test: enable addons test to pass with debug build by danbev · Pull Request #8836 · nodejs/node

danbev · 2016-09-28T19:00:38Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

Currently when running configure with the --debug option in combination
with the tests (./configure --debug && make -j8 test) there are a few
addon tests that fail with error messages similar to this:

=== release test ===
Path: addons/load-long-path/test
fs.js:558
return binding.open(pathModule._makeLong(path), stringToFlags(flags),
mode);
^

Error: ENOENT: no such file or directory, open
'/nodejs/node/test/addons/load-long-path/build/Release/binding.node'
at Object.fs.openSync (fs.js:558:18)
at Object.fs.readFileSync (fs.js:468:33)
at Object.
(/nodejs/node/test/addons/load-long-path/test.js:28:19)
at Module._compile (module.js:560:32)
at Object.Module._extensions..js (module.js:569:10)
at Module.load (module.js:477:32)
at tryModuleLoad (module.js:436:12)
at Function.Module._load (module.js:428:3)
at Module.runMain (module.js:594:10)
at run (bootstrap_node.js:382:7)
Command: out/Release/node
/nodejs/node/test/addons/load-long-path/test.js

This commit allows for the tests to pass even if the configured build
type is of type debug.

Currently when running configure with the --debug option in combination with the tests (./configure --debug && make -j8 test) there are a few addon tests that fail with error messages similar to this: === release test === Path: addons/load-long-path/test fs.js:558 return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode); ^ Error: ENOENT: no such file or directory, open '/nodejs/node/test/addons/load-long-path/build/Release/binding.node' at Object.fs.openSync (fs.js:558:18) at Object.fs.readFileSync (fs.js:468:33) at Object.<anonymous> (/nodejs/node/test/addons/load-long-path/test.js:28:19) at Module._compile (module.js:560:32) at Object.Module._extensions..js (module.js:569:10) at Module.load (module.js:477:32) at tryModuleLoad (module.js:436:12) at Function.Module._load (module.js:428:3) at Module.runMain (module.js:594:10) at run (bootstrap_node.js:382:7) Command: out/Release/node /nodejs/node/test/addons/load-long-path/test.js This commit allows for the tests to pass even if the configured build type is of type debug.

danbev · 2016-09-28T19:01:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4314/

addaleax

LGTM and thank you!! :)

cjihrig

Saw this the other day and meant to LGTM it. Sorry for missing it. LGTM, and the CI seems reasonably happy.

jasnell

LGTM

lpinca

LGTM

When running: $ rm test/addons/.docbuildstamp $ make test/addons/.docbuildstamp A numder of JavaScript test files are generated. These have hard codes paths to build/Release/addons. This commit updates these paths to consider the type of build being performed.

danbev · 2016-10-01T19:54:49Z

Another CI run as I completely missed the paths in addons.md:
https://ci.nodejs.org/job/node-test-pull-request/4352/

imyller

LGTM

imyller · 2016-10-02T21:00:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4358/

danbev · 2016-10-03T11:18:30Z

@addaleax @cjihrig @jasnell @lpinca I just wanted to check if your LGTM still stands with the lastest commit, as I made it afterwards.

bnoordhuis · 2016-10-03T11:28:09Z

I don't think the change to addon.md is a good one. To a third-party add-on, it doesn't matter (much) if node is built in release or debug mode. You can use a debug mode add-on with a release binary and vice versa.

I assume you made the change because node-gyp defaults to the build mode of the node binary? That's actually one of the things I like least about node-gyp; it's confusing to users and it frequently does the wrong thing, like when the node binary that is used to build the add-on is not the binary that actually loads the add-on.

addaleax · 2016-10-03T11:32:42Z

@danbev Could the addons.md modifications also be accomplished by modifying the wrapper in tools/doc/addon-verify.js? If so, I think I’d prefer that when compared to using process.config in the docs itself.

danbev · 2016-10-04T08:35:57Z

I assume you made the change because node-gyp defaults to the build mode of the node binary?

The issue I was trying to work around was that the generated tests assume the the build is of type Release :

// hello.js
const addon = require('./build/Release/addon');

console.log(addon.hello()); // 'world'

and changing this to Debug would cause those generated test to fail. Hope I did not misunderstand you here, that might have been what you meant. This is the first time I've dealt with addons.md and how things are generated, and think I need to look into this a little closer.

I don't think the change to addon.md is a good one.

I can take a shot tonight at adding this to tools/doc/addon-verify.js, like @addaleax suggested, as that would leave addons.md unchanged. Or let me know if there is a better option here that I might not have understood?

bnoordhuis · 2016-10-04T08:49:27Z

The issue I was trying to work around was that the generated tests assume the the build is of type Release

Yes, that's what I mean. node-gyp looks at process.config, the tests don't. It's okay to update the tests but I'd rather not encumber addon.md with it.

danbev · 2016-10-04T08:52:17Z

Yes, that's what I mean. node-gyp looks at process.config, the tests don't. It's okay to update the tests but I'd rather not encumber addon.md with it.

Ah great, then I'll take a stab at @addaleax suggestion and leave addons.md unchanged. Thanks

This reverts commit 0c3ddf8.

danbev · 2016-10-04T20:03:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/4386/

bnoordhuis

LGTM, thanks.

addaleax

Thanks for updating this, still LGTM!

imyller · 2016-10-05T16:22:03Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/4396/

jasnell · 2016-10-06T17:23:49Z

CI failures look unrelated.

Currently when running configure with the --debug option in combination with the tests (./configure --debug && make -j8 test) there are a few addon tests that fail with error messages similar to this: === release test === Path: addons/load-long-path/test fs.js:558 return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode); ^ Error: ENOENT: no such file or directory, open '/nodejs/node/test/addons/load-long-path/build/Release/binding.node' at Object.fs.openSync (fs.js:558:18) at Object.fs.readFileSync (fs.js:468:33) at Object.<anonymous> (/nodejs/node/test/addons/load-long-path/test.js:28:19) at Module._compile (module.js:560:32) at Object.Module._extensions..js (module.js:569:10) at Module.load (module.js:477:32) at tryModuleLoad (module.js:436:12) at Function.Module._load (module.js:428:3) at Module.runMain (module.js:594:10) at run (bootstrap_node.js:382:7) Command: out/Release/node /nodejs/node/test/addons/load-long-path/test.js This commit allows for the tests to pass even if the configured build type is of type debug. PR-URL: #8836 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell · 2016-10-06T17:31:04Z

Landed in fdca79f

Currently when running configure with the --debug option in combination with the tests (./configure --debug && make -j8 test) there are a few addon tests that fail with error messages similar to this: === release test === Path: addons/load-long-path/test fs.js:558 return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode); ^ Error: ENOENT: no such file or directory, open '/nodejs/node/test/addons/load-long-path/build/Release/binding.node' at Object.fs.openSync (fs.js:558:18) at Object.fs.readFileSync (fs.js:468:33) at Object.<anonymous> (/nodejs/node/test/addons/load-long-path/test.js:28:19) at Module._compile (module.js:560:32) at Object.Module._extensions..js (module.js:569:10) at Module.load (module.js:477:32) at tryModuleLoad (module.js:436:12) at Function.Module._load (module.js:428:3) at Module.runMain (module.js:594:10) at run (bootstrap_node.js:382:7) Command: out/Release/node /nodejs/node/test/addons/load-long-path/test.js This commit allows for the tests to pass even if the configured build type is of type debug. PR-URL: #8836 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2016-10-06T23:58:11Z

lts?

jasnell · 2016-10-07T02:33:16Z

I'd say it's likely low priority but it likely wouldn't hurt if it applies cleanly. I'd mark it as an optional backport if it does not apply clean.

Currently when running configure with the --debug option in combination with the tests (./configure --debug && make -j8 test) there are a few addon tests that fail with error messages similar to this: === release test === Path: addons/load-long-path/test fs.js:558 return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode); ^ Error: ENOENT: no such file or directory, open '/nodejs/node/test/addons/load-long-path/build/Release/binding.node' at Object.fs.openSync (fs.js:558:18) at Object.fs.readFileSync (fs.js:468:33) at Object.<anonymous> (/nodejs/node/test/addons/load-long-path/test.js:28:19) at Module._compile (module.js:560:32) at Object.Module._extensions..js (module.js:569:10) at Module.load (module.js:477:32) at tryModuleLoad (module.js:436:12) at Function.Module._load (module.js:428:3) at Module.runMain (module.js:594:10) at run (bootstrap_node.js:382:7) Command: out/Release/node /nodejs/node/test/addons/load-long-path/test.js This commit allows for the tests to pass even if the configured build type is of type debug. PR-URL: #8836 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2016-11-18T20:56:25Z

@jasnell it is not landing cleanly but the diff is something like 2 lines... might be worth doing manually if you have the bandwidth

Currently when running configure with the --debug option in combination with the tests (./configure --debug && make -j8 test) there are a few addon tests that fail with error messages similar to this: === release test === Path: addons/load-long-path/test fs.js:558 return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode); ^ Error: ENOENT: no such file or directory, open '/nodejs/node/test/addons/load-long-path/build/Release/binding.node' at Object.fs.openSync (fs.js:558:18) at Object.fs.readFileSync (fs.js:468:33) at Object.<anonymous> (/nodejs/node/test/addons/load-long-path/test.js:28:19) at Module._compile (module.js:560:32) at Object.Module._extensions..js (module.js:569:10) at Module.load (module.js:477:32) at tryModuleLoad (module.js:436:12) at Function.Module._load (module.js:428:3) at Module.runMain (module.js:594:10) at run (bootstrap_node.js:382:7) Command: out/Release/node /nodejs/node/test/addons/load-long-path/test.js This commit allows for the tests to pass even if the configured build type is of type debug. PR-URL: #8836 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 28, 2016

mscdex added the addons Issues and PRs related to native addons. label Sep 28, 2016

addaleax approved these changes Sep 30, 2016

View reviewed changes

cjihrig approved these changes Sep 30, 2016

View reviewed changes

jasnell approved these changes Sep 30, 2016

View reviewed changes

lpinca approved these changes Oct 1, 2016

View reviewed changes

imyller approved these changes Oct 2, 2016

View reviewed changes

danbev added 2 commits October 4, 2016 21:56

Revert "update addons.md to consider the build type"

79c407c

This reverts commit 0c3ddf8.

update addon-verify.js to consider build type

c991295

bnoordhuis approved these changes Oct 5, 2016

View reviewed changes

addaleax approved these changes Oct 5, 2016

View reviewed changes

jasnell closed this Oct 6, 2016

MylesBorins added the lts-watch-v4.x label Oct 6, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 20, 2016

MylesBorins mentioned this pull request Dec 21, 2016

V4.7.1 proposal #10395

Merged

danbev deleted the fix-debug-testing branch January 17, 2017 07:17

snyk-bot mentioned this pull request Apr 24, 2023

[Snyk] Security upgrade jest from 24.9.0 to 25.0.0 aliscco/alisco-node#95

Open

aliscco mentioned this pull request Apr 28, 2023

[Snyk] Fix for 5 vulnerabilities aliscco/alisco-node#348

Open

aliscco mentioned this pull request Jun 23, 2023

[Snyk] Security upgrade jest from 24.9.0 to 25.0.0 aliscco/alisco-node#565

Open

Uh oh!

Conversation

danbev commented Sep 28, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

danbev commented Sep 28, 2016

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

danbev commented Oct 1, 2016

Uh oh!

imyller left a comment

Choose a reason for hiding this comment

Uh oh!

imyller commented Oct 2, 2016

Uh oh!

danbev commented Oct 3, 2016

Uh oh!

bnoordhuis commented Oct 3, 2016

Uh oh!

addaleax commented Oct 3, 2016

Uh oh!

danbev commented Oct 4, 2016

Uh oh!

bnoordhuis commented Oct 4, 2016

Uh oh!

danbev commented Oct 4, 2016

Uh oh!

danbev commented Oct 4, 2016

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

imyller commented Oct 5, 2016

Uh oh!

jasnell commented Oct 6, 2016

Uh oh!

jasnell commented Oct 6, 2016

Uh oh!

MylesBorins commented Oct 6, 2016

Uh oh!

jasnell commented Oct 7, 2016

Uh oh!

MylesBorins commented Nov 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants