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: handle missing V8 tests in n-api test #14123

Merged
merged 1 commit into from
Jul 13, 2017
Merged

test: handle missing V8 tests in n-api test #14123

merged 1 commit into from
Jul 13, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 7, 2017

The N-API test testInstanceOf.js relies on several V8 test files which may not exist in downloadable archives. If the files are missing, this commit causes a warning to be emitted rather than failing the test.

Refs: #14113
Fixes: #13344

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Jul 7, 2017
targos
targos reviewed Jul 7, 2017
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

Would we see the warning with make test if a V8 upgrade removed one of the files? If so, LGTM.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 7, 2017

No we wouldn't see it in make test. You'd need to run the test in a way that displays the test's output.

@targos
Copy link
Member

targos commented Jul 7, 2017

I don't think it is likely to happen but I'm afraid that we never realize if a file disappears from the source. Maybe throw if deps/v8/test exists?

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 7, 2017

Wouldn't that put us back in the current situation for people who run the tests via downloaded archive (not git clone)? See #13344

targos
targos reviewed Jul 7, 2017
View reviewed changes
test/addons-napi/test_general/testInstanceOf.js Outdated
} catch (err) {
// This test depends on V8 test files, which may not exist in downloaded
// archives. Emit a warning if the tests cannot be found instead of failing.
if (err.code === 'ENOENT')
Copy link
Member

Choose a reason for hiding this comment

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

@cjihrig I propose to add && !fs.existsSync(path.resolve(__dirname, '../../../deps/v8/test')).
If the file is not found and the test directory exists, this is unexpected because the test directory should be absent from the source tarball.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what you mean now. Thanks.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 7, 2017

@targos updated!

targos
targos approved these changes Jul 7, 2017
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

mhdawson
mhdawson approved these changes Jul 7, 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

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 11, 2017 •
edited
Loading

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

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

@cjihrig
test: handle missing V8 tests in n-api test
598a128 
The N-API test testInstanceOf.js relies on several V8 test files
which may not exist in downloadable archives. If the files are
missing, this commit causes a warning to be emitted rather than
failing the test.

Refs: nodejs#14113
Fixes: nodejs#13344
PR-URL: nodejs#14123
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cjihrig cjihrig merged commit 598a128 into nodejs:master Jul 13, 2017
@cjihrig cjihrig deleted the napi-test branch July 13, 2017 15:55
@addaleax addaleax mentioned this pull request Jul 18, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@cjihrig @addaleax
test: handle missing V8 tests in n-api test
3fcc7e6 
The N-API test testInstanceOf.js relies on several V8 test files
which may not exist in downloadable archives. If the files are
missing, this commit causes a warning to be emitted rather than
failing the test.

Refs: #14113
Fixes: #13344
PR-URL: #14123
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@cjihrig @Fishrock123
test: handle missing V8 tests in n-api test
3ffd938 
The N-API test testInstanceOf.js relies on several V8 test files
which may not exist in downloadable archives. If the files are
missing, this commit causes a warning to be emitted rather than
failing the test.

Refs: #14113
Fixes: #13344
PR-URL: #14123
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
@cjihrig
test: handle missing V8 tests in n-api test
fb572c4 
The N-API test testInstanceOf.js relies on several V8 test files
which may not exist in downloadable archives. If the files are
missing, this commit causes a warning to be emitted rather than
failing the test.

Refs: nodejs#14113
Fixes: nodejs#13344
PR-URL: nodejs#14123
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@cjihrig @MylesBorins
test: handle missing V8 tests in n-api test
4827421 
The N-API test testInstanceOf.js relies on several V8 test files
which may not exist in downloadable archives. If the files are
missing, this commit causes a warning to be emitted rather than
failing the test.

Refs: #14113
Fixes: #13344
Backport-PR-URL: #19447
PR-URL: #14123
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
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

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@cjihrig @targos @bnoordhuis @danbev @jasnell @mhdawson @MylesBorins @nodejs-github-bot