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.

Sign up for GitHub

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

Jump to bottom

test: add check on an addon that doesn't register #13954

Closed
wants to merge 1 commit into from
Closed

test: add check on an addon that doesn't register #13954

wants to merge 1 commit into from

Conversation

ezequielgarcia
Copy link
Contributor

@ezequielgarcia ezequielgarcia commented Jun 28, 2017 •
edited
Loading

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
cjihrig
cjihrig reviewed Jun 28, 2017
View reviewed changes
test/addons/not-a-binding/test.js Outdated
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.

bnoordhuis
bnoordhuis approved these changes Jun 29, 2017
View reviewed changes
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.

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

test/addons/not-a-binding/test.js Outdated
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.

@ezequielgarcia
test: add check on an addon that doesn't register
1db7836 
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 <ezequiel@vanguardiasur.com.ar>
@ezequielgarcia
Copy link
Contributor Author

Agreed. Changed to literal.

mhdawson
mhdawson approved these changes Jul 6, 2017
View reviewed changes
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

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

@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 •
edited
Loading

@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
@ezequielgarcia @tniessen
test: add check on an addon that does not register
13d6eae 
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 <ezequiel@vanguardiasur.com.ar>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax pushed a commit that referenced this pull request Aug 1, 2017
@ezequielgarcia @addaleax
test: add check on an addon that does not register
f40b906 
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 <ezequiel@vanguardiasur.com.ar>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@addaleax addaleax mentioned this pull request Aug 2, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@ezequielgarcia @MylesBorins
test: add check on an addon that does not register
ba89673 
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 <ezequiel@vanguardiasur.com.ar>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@ezequielgarcia @MylesBorins
test: add check on an addon that does not register
9132f37 
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 <ezequiel@vanguardiasur.com.ar>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Sep 3, 2017
@ezequielgarcia @MylesBorins
test: add check on an addon that does not register
0e95742 
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 <ezequiel@vanguardiasur.com.ar>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
@ezequielgarcia @MylesBorins
test: add check on an addon that does not register
e5375a9 
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 <ezequiel@vanguardiasur.com.ar>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees

@tniessen tniessen

Labels
addons Issues and PRs related to native addons. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@ezequielgarcia @jasnell @gibfahn @tniessen @bnoordhuis @refack @cjihrig @mhdawson @MylesBorins @nodejs-github-bot