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: add check on an addon that doesn't register #13954

Closed

Conversation

ezequielgarcia
Copy link
Contributor

@ezequielgarcia ezequielgarcia commented Jun 28, 2017

This test require()s a shared library that won't register as a node module.

Other tests would be possible such as loading a file that is not a shared library, or loading a path that doesn't exist. However, these two tests produce platform-dependent error messages, and so I haven't included them.

Checklist
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. labels Jun 28, 2017
const common = require('../../common');
const assert = require('assert');

const re = new RegExp('Module did not self-register');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use ^ and $ to make sure the entire error message is being matched.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjihrig done.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const assert = require('assert');

const re = new RegExp('^Error: Module did not self-register\\.$');
assert.throws(() => require(`./build/${common.buildType}/binding`), re);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, what is the reason for using a regexp object instead of a regexp literal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason, just that I used the node-module-version test as model.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd generally prefer the regexp literal form.

This commit calls require on a shared library that is
not declared as a node module, and therefore does not register
properly.

Signed-off-by: Ezequiel Garcia <[email protected]>
@ezequielgarcia
Copy link
Contributor Author

Agreed. Changed to literal.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jasnell
Copy link
Member

jasnell commented Jul 10, 2017

@jasnell
Copy link
Member

jasnell commented Jul 10, 2017

Looks like there are linting issues that need to be fixed.

@ezequielgarcia
Copy link
Contributor Author

ezequielgarcia commented Jul 10, 2017

@jasnell I don't have read permissions, my user is ezequielgarcia. I'll run the linter and update.

@jasnell
Copy link
Member

jasnell commented Jul 10, 2017

Access to the CI environment is temporarily blocked while testing a number of security fixes.

@gibfahn
Copy link
Member

gibfahn commented Jul 11, 2017

@jasnell I don't have read permissions, my user is ezequielgarcia. I'll run the linter and update.

Try now 😁

@ezequielgarcia
Copy link
Contributor Author

@jasnell according to https://ci.nodejs.org/job/node-test-linter/10386/console, there was some git errors...

git fetch --tags --progress https://github.com/nodejs/node.git +refs/heads/:refs/remotes/origin/ +refs/pull/13954/head:refs/remotes/origin/_jenkins_local_branch # timeout=20
ERROR: Error fetching remote repo 'origin'

or am I looking the wrong place?

@gibfahn
Copy link
Member

gibfahn commented Jul 13, 2017

@ezequielgarcia that one looks like a transient issue, rerunning:

CI 2: https://ci.nodejs.org/job/node-test-commit/11092/

@tniessen
Copy link
Member

tniessen commented Aug 1, 2017

Pre-land CI: https://ci.nodejs.org/job/node-test-pull-request/9432/

Assuming there are no objections from @bnoordhuis @jasnell @cjihrig @mhdawson ... ?

@tniessen tniessen self-assigned this Aug 1, 2017
@bnoordhuis
Copy link
Member

No objections, still LGTM.

@refack
Copy link
Contributor

refack commented Aug 1, 2017

centos fail is most probably unrelated - opened #14568

@jasnell
Copy link
Member

jasnell commented Aug 1, 2017

still lgtm

@tniessen
Copy link
Member

tniessen commented Aug 1, 2017

Landed in 13d6eae, thank you!

@tniessen tniessen closed this Aug 1, 2017
tniessen pushed a commit that referenced this pull request Aug 1, 2017
This commit calls require on a shared library that is not declared
as a node module, and therefore does not register properly.

PR-URL: #13954
Signed-off-by: Ezequiel Garcia <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
addaleax pushed a commit that referenced this pull request Aug 1, 2017
This commit calls require on a shared library that is not declared
as a node module, and therefore does not register properly.

PR-URL: #13954
Signed-off-by: Ezequiel Garcia <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@addaleax addaleax mentioned this pull request Aug 2, 2017
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
This commit calls require on a shared library that is not declared
as a node module, and therefore does not register properly.

PR-URL: #13954
Signed-off-by: Ezequiel Garcia <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
This commit calls require on a shared library that is not declared
as a node module, and therefore does not register properly.

PR-URL: #13954
Signed-off-by: Ezequiel Garcia <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 3, 2017
This commit calls require on a shared library that is not declared
as a node module, and therefore does not register properly.

PR-URL: #13954
Signed-off-by: Ezequiel Garcia <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
This commit calls require on a shared library that is not declared
as a node module, and therefore does not register properly.

PR-URL: #13954
Signed-off-by: Ezequiel Garcia <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
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

Successfully merging this pull request may close these issues.

10 participants