Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test failing in Debug mode #7860

Closed
fhinkel opened this issue Jul 24, 2016 · 8 comments
Closed

Test failing in Debug mode #7860

fhinkel opened this issue Jul 24, 2016 · 8 comments
Labels
addons Issues and PRs related to native addons. test Issues and PRs related to the tests.

Comments

@fhinkel
Copy link
Member

fhinkel commented Jul 24, 2016

  • Version: master and v6.3.0, since 1fe0708
  • Platform: x86_64 GNU/Linux
  • Subsystem: test

The test for addons/repl-domain-abort is failing, when configure was run in debug mode:

./configure --debug && make -j48 test

The assertion in the test test/addons/repl-domain-abort/test.js fails:

/usr/bin/python tools/test.py --mode=release -J \
        addons doctool known_issues message pseudo-tty parallel sequential
=== release test ===                                                           
Path: addons/repl-domain-abort/test
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: false == true
    at process.<anonymous> (/usr/local/google/home/franzih/node/test/addons/repl-domain-abort/test.js:15:3)
    at emitOne (events.js:101:20)
    at process.emit (events.js:188:7)
Command: out/Release/node /usr/local/google/home/franzih/node/test/addons/repl-domain-abort/test.js

With some debug statements, turns out the binding module cannot be found:

Error: Cannot find module '/usr/local/google/home/franzih/node/test/addons/repl-domain-abort/build/Debug/binding'

Should build/Debug be created or should the correct path be build/Release/binding?

@bnoordhuis bnoordhuis added test Issues and PRs related to the tests. addons Issues and PRs related to native addons. labels Jul 24, 2016
@bnoordhuis
Copy link
Member

I think the issue with that particular test is that it looks at the default_configuration whereas most other add-on tests simply load the the binding.node from a hard-coded path. Does this patch fix the issue for you?

diff --git a/test/addons/repl-domain-abort/test.js b/test/addons/repl-domain-abort/test.js
index 5591b4f..057af4f 100644
--- a/test/addons/repl-domain-abort/test.js
+++ b/test/addons/repl-domain-abort/test.js
@@ -4,8 +4,7 @@ var assert = require('assert');
 var repl = require('repl');
 var stream = require('stream');
 var path = require('path');
-var buildType = process.config.target_defaults.default_configuration;
-var buildPath = path.join(__dirname, 'build', buildType, 'binding');
+var buildPath = path.join(__dirname, 'build/Release/binding');
 // On Windows, escape backslashes in the path before passing it to REPL.
 if (common.isWindows)
   buildPath = buildPath.replace(/\\/g, '/');

Should build/Debug be created or should the correct path be build/Release/binding?

As mentioned, most tests use a hard-coded path but that is arguably wrong: node-gyp builds add-ons in debug mode when a) --debug is passed, or b) the node binary used to build it is a debug build; the latter is something of a historical artifact.

IOW, build/Debug is the correct path some of the time but the tests don't currently respect it.

@fhinkel
Copy link
Member Author

fhinkel commented Jul 24, 2016

Thanks, that fixes it. I can't figure out how to generate build/Debug. I thought BUILDTYPE=Debug would do that.

@bnoordhuis
Copy link
Member

You have to pass --debug on the command line or npm_config_debug=1 in the environment.

@danbev
Copy link
Contributor

danbev commented Jul 26, 2016

I don't mean to high jack this issue but it is somewhat related. I had an issue getting the add-on tests to run again after configured with --debug. What I mean is that I re-ran ./configure without the --debug option to generate a Release build.

My issue was that when running the tests, the build-addons target would not update the addon's config.gypi which still referred to "default_configuration": "Debug". To force an update I deleted test/addons/.buildstamp, and this allowed me to run the tests.

Should the removal of test/addons/.buildstamp perhaps be part of a target like clean or distclean?

@fhinkel
Copy link
Member Author

fhinkel commented Jul 27, 2016

@danbev I'm having similar issues. Especially after switching back from older branches.

@danbev
Copy link
Contributor

danbev commented Jul 27, 2016

@fhinkel Good to know that it is not just me :) I'll create a PR with a suggestion for handling.

danbev added a commit to danbev/node that referenced this issue Jul 30, 2016
Currently the build-addons target is called as part of the
test target, and it has test/addons/.buildstamp as a dependency.
When running ./configure --debug and later switching/
updating branches you can be in a situation where the config.gypi
files in the addons build directories are using an incorrect
value for default_configuration. Currently this can happen and you
will get test errors as there are a few test that depend on the
value of default_configuration to be Release.

Ben Noordhuis provided the solution for this by adding a dependency to
config.gypi, which is generated when running ./configure, and will
correct this situation.

This commit is related to nodejs#7860. Please see the dicussion in that
issue regarding the test using hard-coded paths.
jasnell pushed a commit that referenced this issue Aug 1, 2016
Currently the build-addons target is called as part of the
test target, and it has test/addons/.buildstamp as a dependency.
When running ./configure --debug and later switching/
updating branches you can be in a situation where the config.gypi
files in the addons build directories are using an incorrect
value for default_configuration. Currently this can happen and you
will get test errors as there are a few test that depend on the
value of default_configuration to be Release.

Ben Noordhuis provided the solution for this by adding a dependency to
config.gypi, which is generated when running ./configure, and will
correct this situation.

This commit is related to #7860. Please see the dicussion in that
issue regarding the test using hard-coded paths.

PR-URL: #7893
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
cjihrig pushed a commit that referenced this issue Aug 10, 2016
Currently the build-addons target is called as part of the
test target, and it has test/addons/.buildstamp as a dependency.
When running ./configure --debug and later switching/
updating branches you can be in a situation where the config.gypi
files in the addons build directories are using an incorrect
value for default_configuration. Currently this can happen and you
will get test errors as there are a few test that depend on the
value of default_configuration to be Release.

Ben Noordhuis provided the solution for this by adding a dependency to
config.gypi, which is generated when running ./configure, and will
correct this situation.

This commit is related to #7860. Please see the dicussion in that
issue regarding the test using hard-coded paths.

PR-URL: #7893
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
danbev added a commit to danbev/node that referenced this issue Oct 6, 2016
Currently the build-addons target is called as part of the
test target, and it has test/addons/.buildstamp as a dependency.
When running ./configure --debug and later switching/
updating branches you can be in a situation where the config.gypi
files in the addons build directories are using an incorrect
value for default_configuration. Currently this can happen and you
will get test errors as there are a few test that depend on the
value of default_configuration to be Release.

Ben Noordhuis provided the solution for this by adding a dependency to
config.gypi, which is generated when running ./configure, and will
correct this situation.

This commit is related to nodejs#7860. Please see the dicussion in that
issue regarding the test using hard-coded paths.

Ref: nodejs#7893
MylesBorins pushed a commit that referenced this issue Oct 7, 2016
Currently the build-addons target is called as part of the
test target, and it has test/addons/.buildstamp as a dependency.
When running ./configure --debug and later switching/
updating branches you can be in a situation where the config.gypi
files in the addons build directories are using an incorrect
value for default_configuration. Currently this can happen and you
will get test errors as there are a few test that depend on the
value of default_configuration to be Release.

Ben Noordhuis provided the solution for this by adding a dependency to
config.gypi, which is generated when running ./configure, and will
correct this situation.

This commit is related to #7860. Please see the dicussion in that
issue regarding the test using hard-coded paths.

Ref: #8905
PR-URL: #7893
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 10, 2016
Currently the build-addons target is called as part of the
test target, and it has test/addons/.buildstamp as a dependency.
When running ./configure --debug and later switching/
updating branches you can be in a situation where the config.gypi
files in the addons build directories are using an incorrect
value for default_configuration. Currently this can happen and you
will get test errors as there are a few test that depend on the
value of default_configuration to be Release.

Ben Noordhuis provided the solution for this by adding a dependency to
config.gypi, which is generated when running ./configure, and will
correct this situation.

This commit is related to #7860. Please see the dicussion in that
issue regarding the test using hard-coded paths.

Ref: #8905
PR-URL: #7893
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this issue Oct 18, 2016
Currently the build-addons target is called as part of the
test target, and it has test/addons/.buildstamp as a dependency.
When running ./configure --debug and later switching/
updating branches you can be in a situation where the config.gypi
files in the addons build directories are using an incorrect
value for default_configuration. Currently this can happen and you
will get test errors as there are a few test that depend on the
value of default_configuration to be Release.

Ben Noordhuis provided the solution for this by adding a dependency to
config.gypi, which is generated when running ./configure, and will
correct this situation.

This commit is related to #7860. Please see the dicussion in that
issue regarding the test using hard-coded paths.

Ref: #8905
PR-URL: #7893
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 26, 2016
Currently the build-addons target is called as part of the
test target, and it has test/addons/.buildstamp as a dependency.
When running ./configure --debug and later switching/
updating branches you can be in a situation where the config.gypi
files in the addons build directories are using an incorrect
value for default_configuration. Currently this can happen and you
will get test errors as there are a few test that depend on the
value of default_configuration to be Release.

Ben Noordhuis provided the solution for this by adding a dependency to
config.gypi, which is generated when running ./configure, and will
correct this situation.

This commit is related to #7860. Please see the dicussion in that
issue regarding the test using hard-coded paths.

Ref: #8905
PR-URL: #7893
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@Trott
Copy link
Member

Trott commented Jul 9, 2017

@fhinkel Should this issue remain open?

@fhinkel
Copy link
Member Author

fhinkel commented Mar 19, 2018

No 🤣

@fhinkel fhinkel closed this as completed Mar 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

4 participants