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

ci: enable travis for fast PR check #534

Merged
merged 2 commits into from
Sep 20, 2019
Merged

ci: enable travis for fast PR check #534

merged 2 commits into from
Sep 20, 2019

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Sep 4, 2019 •
edited
Loading

test: remove unnecessary NAPI_EXPERIMENTAL

As ci.nodejs.org doesn't automatically run for PRs, travis is still the easiest way to determine if the PR is test passed on multiple node variants and platforms.

travis: remove chakracore on travis
  1. node-chakracore v8 doesn’t expose napi version
  2. node-chakracore v10 doesn’t throws on handlescope double escape
  3. set npm_config_NAPI_VERSION to override NAPI_EXPERIMENTAL expansion
    on Node.js version lower than 11 for napi_date_* not back ported to v10
  • npm test passed

mhdawson
mhdawson reviewed Sep 4, 2019
View reviewed changes
test/bigint.cc Outdated
@@ -1,4 +1,3 @@
#define NAPI_EXPERIMENTAL
Copy link
Member

Choose a reason for hiding this comment

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

I think this will cause the tests not to run due to this later check #if (NAPI_VERSION > 2147483646)

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 maybe an option to control whether NAPI_EXPERIMENTAL should be defined or not might be needed.

Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly we had it enabled by default so that we'd run the tests in the most common use case (running on master) and then then CI sets the env variable so that the tests don't run when they run on earlier node version (ex Node.js v8)

Copy link
Member Author

@legendecas legendecas Sep 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

But if NAPI_EXPERIMENTAL is defined the binding.node would mark napi_bigint related symbol as required, which should probably fail on Node.js v8, which I found on travis builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed npm_config_NAPI_VERSION in test/index.js which would disable bigint tests. AFAIKT, it doesn't affect compilation right? Then requiring binding.node would fail on Node versions that don't have bigint NAPIs.

Copy link
Member Author

@legendecas legendecas Sep 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

https://travis-ci.com/legendecas/node-addon-api/builds/125931273

Used node major version to determine if experimental tests should be ran.

In Node.js v8 bigint tests were ignored.
In Node.js v10 bigint tests were ran.

@legendecas legendecas force-pushed the travis branch 10 times, most recently from f81685d to 8415aef Compare September 5, 2019 03:56
@legendecas
Copy link
Member Author

@mhdawson hi, just updated the PR to use npm_config_NAPI_VERSION with node-gyp on travis which should be aligned with ci.nodejs.org. Please take a look :)

mhdawson
mhdawson reviewed Sep 5, 2019
View reviewed changes
test/index.js Outdated
@@ -45,8 +47,10 @@ let testModules = [
'version_management'
];

if ((process.env.npm_config_NAPI_VERSION !== undefined) &&
(process.env.npm_config_NAPI_VERSION < 50000)) {
const napiVersion = Number(process.env.npm_config_NAPI_VERSION || process.versions.napi);
Copy link
Member

Choose a reason for hiding this comment

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

I think this will still disable testing the experimental tests by default right? I think we want to make sure that if you just do a checkout that they will run unless you specifically do something to prevent that. Otherwise It will be too easy for us to miss running the experimental tests when we land PRs. Previously if npm_config_NAPI_VERSION was not set then all tests would run, afterwards the experimental tests will never run unless npm_config_NAPI_VERSION is set to run then....

Copy link
Member Author

@legendecas legendecas Sep 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, I noticed that with your mention.

Also I found out that napi_date related API was landed on Node.js v12 as experimental yet NAPI v5 on master. And bigint related API was landed on Node.js v8, v10, v12 and master as experimental. It might be not enough to control experimental features merely by NAPI_VERSION if we'd like to address failure of old Node.js version running node-addon-api tests locally with simple npm test.

So maybe NAPI_EXPERIMENTAL features should be filtered with Node.js major version.

Nevertheless, I'd like to submit another PR to address this issue. For this PR, we could enable travis for fast PR test check?

Copy link
Member

Choose a reason for hiding this comment

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

Seems reasonable to me. Will wait until Monday to see if we have any other comments, otherwise will land.

@legendecas legendecas force-pushed the travis branch 5 times, most recently from 49887b2 to e985975 Compare September 6, 2019 06:02
@legendecas
test: remove unnecessary NAPI_EXPERIMENTAL
598ab9a
@legendecas
travis: remove chakracore on travis
64d87ee 
1. node-chakracore v8 doesn’t expose napi version
2. node-chakracore v10 doesn’t throws on handlescope double escape
3. set npm_config_NAPI_VERSION to override NAPI_EXPERIMENTAL expansion
  on Node.js version lower than 11 for napi_date_* not back ported to
  v10
@legendecas legendecas changed the title test: fix failures on node variants CI: enable travis for fast PR check Sep 6, 2019
@legendecas legendecas changed the title CI: enable travis for fast PR check ci: enable travis for fast PR check Sep 6, 2019
mhdawson
mhdawson approved these changes Sep 6, 2019
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

@legendecas legendecas mentioned this pull request Sep 18, 2019
Merged
4 tasks
mhdawson
mhdawson approved these changes Sep 20, 2019
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

@mhdawson mhdawson merged commit 5d6aeae into nodejs:master Sep 20, 2019
@mhdawson
Copy link
Member

Landed as 5d6aeae

@mhdawson mhdawson mentioned this pull request Sep 23, 2019
Closed
mhdawson added a commit to mhdawson/node-addon-api that referenced this pull request Sep 23, 2019
@mhdawson
test: fix up test failures from PR nodejs#534
f84b9cc 
https://github.com/nodejs/node-addon-api/pulls?q=is%3Apr+is%3Aclosed
remove the EXPERIMENATAL define. This messed up some of the
testing. Adding back until we investigate why.
@legendecas legendecas mentioned this pull request Oct 4, 2019
Closed
@legendecas legendecas deleted the travis branch October 4, 2019 17:11
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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@legendecas @mhdawson