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: conditional test experimental features with NODE_MAJOR_VERSION #545

Merged
merged 5 commits into from
Nov 1, 2019
Merged

test: conditional test experimental features with NODE_MAJOR_VERSION #545

merged 5 commits into from
Nov 1, 2019

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Sep 18, 2019 •
edited
Loading

test: conditionally test experimental features with NODE_MAJOR_VERSION

NAPI_EXPERIMENTAL would be expand to NAPI_VERSION=2147483647 if
NAPI_VERSION is not defined. It would be not compatible with various
Node.js versions if we use NAPI_VERSION > 2147483646 mixed with
definition of NAPI_EXPERIMENTAL. This fix introduced NODE_MAJOR_VERSION
to allow more precise control over test sets that which set should be
enabled on a Node.js release.

Refs: #534 (comment)

build: remove unnecessary NAPI_VERSION pre-definition on travis

Since experimental api tests were conditioned by NODE_MAJOR_VERSION, we could simply run tests with npm test on every Node.js LTS version.

  • npm test passed
    • Node.js v8.16.1
    • Node.js v10.16.3
    • Node.js v12.10.0
src: enabling BigInt API with NAPI_EXPERIMENTAL

Strictly aligning with Node.js header js_native_api.h in which
napi_bigint related apis were nested in NAPI_EXPERIMENTAL.

@legendecas legendecas force-pushed the experiment-feature-test branch from 080913a to d098649 Compare September 18, 2019 12:51
@mhdawson
Copy link
Member

I'll have to find time to look more carefully, but I don't think we should have checks based on NODE_MAJOR_VERSION. It should be that either you need to enable experimental or guard against a specific N-API version. I could be wrong but I think adding checks against the NODE_MAJOR_VERSION will go counter to us wanting to provide support across versions.

@legendecas
Copy link
Member Author

IMO, NODE_MAJOR_VERSION should only be used on the condition of experimental new apis like napi_bigint and previously napi_date which has different backport status so we could distinguish if the related test could be ran on that Node.js version hardly with NAPI_EXPERIMENTAL definition.

Out of experimental api should be guarded with NAPI_VERSION normally.

@legendecas legendecas force-pushed the experiment-feature-test branch 2 times, most recently from a9acd95 to 8757704 Compare September 22, 2019 02:27
@legendecas legendecas mentioned this pull request Oct 22, 2019
Closed
15 tasks
@legendecas legendecas force-pushed the experiment-feature-test branch from 8757704 to d198d14 Compare October 22, 2019 16:30
@legendecas legendecas mentioned this pull request Oct 22, 2019
Closed
@legendecas
Copy link
Member Author

For the issue of test not been run, check out here https://travis-ci.com/nodejs/node-addon-api/jobs/248367006#L296, the tests of bigint and date have been run on Node.js v10 and Node.js v12.

@gabrielschulhof
Copy link
Contributor

@mhdawson I believe that using NODE_MAJOR_VERSION should be OK in the tests. Nevertheless, it must not make its way into napi.h or napi-inl.h.

@legendecas legendecas changed the title NAPI_EXPERIMENTAL mixed with various Node.js Major Version test: conditional test experimental features with NODE_MAJOR_VERSION Oct 29, 2019
@legendecas legendecas force-pushed the experiment-feature-test branch from 797d3d5 to e7b4eea Compare October 29, 2019 05:11
@legendecas
Copy link
Member Author

legendecas commented Oct 29, 2019 •
edited
Loading

Just updated the descriptions of the changelist to better indicate the NODE_MAJOR_VERSION was only used in tests.

@mhdawson please take another look :)

@legendecas legendecas requested review from mhdawson and NickNaso and removed request for mhdawson October 29, 2019 05:13
@legendecas legendecas force-pushed the experiment-feature-test branch from e7b4eea to a1cb729 Compare October 29, 2019 05:15
@legendecas
test: conditionally test experimental features with NODE_MAJOR_VERSION
93348b1 
NAPI_EXPERIMENTAL would be expand to NAPI_VERSION=2147483647 if
NAPI_VERSION is not defined. It would be not compatible with various
Node.js versions if we use NAPI_VERSION > 2147483646 mixed with
definition of NAPI_EXPERIMENTAL. This fix introduced NODE_MAJOR_VERSION
to allow more precise control over test sets that which set should be
enabled on a Node.js release.
@legendecas
build: remove unnecessary NAPI_VERSION pre-definition on travis
890d904
@legendecas
src: enabling BigInt API with NAPI_EXPERIMENTAL
988e5c2 
Strictly aligning with Node.js header js_native_api.h in which
napi_bigint related apis were nested in NAPI_EXPERIMENTAL.
@legendecas legendecas force-pushed the experiment-feature-test branch from a1cb729 to 988e5c2 Compare October 30, 2019 02:35
mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
napi-inl.h Outdated
@@ -613,9 +613,9 @@ inline double Number::DoubleValue() const {
return result;
}

// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
// currently experimental guard with version of NAPI_VERSION that it is

Copy link
Member Author

Choose a reason for hiding this comment

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

With commit src: enabling BigInt API with NAPI_EXPERIMENTAL, BigInt would only be enabled with the definition of NAPI_EXPERIMENTAL rather than a very big NAPI_VERSION, which is what has been done in js_native_api.h.

Copy link
Member

Choose a reason for hiding this comment

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

What the comment is trying to say is:

  • Currently it is experimental
  • When it becomes non-experimental is should be guarded with the NAPI_VERSION in which it became non-experimental (for example it might be version 6).

Maybe it comment should be

// currently experimental, for now guard with definition of NAPI_EXPERIMENTAL. Once it is no
// longer experimental guard with the NAPI_VERSION in which it is released.

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
napi-inl.h Outdated
@@ -376,9 +376,9 @@ inline bool Value::IsNumber() const {
return Type() == napi_number;
}

// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
// currently experimental guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
napi.h Outdated
@@ -111,9 +111,9 @@ namespace Napi {
class Value;
class Boolean;
class Number;
// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
// currently experimental guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
napi.h Outdated
@@ -139,9 +139,9 @@ namespace Napi {
typedef TypedArrayOf<uint32_t> Uint32Array; ///< Typed-array of unsigned 32-bit integers
typedef TypedArrayOf<float> Float32Array; ///< Typed-array of 32-bit floating-point values
typedef TypedArrayOf<double> Float64Array; ///< Typed-array of 64-bit floating-point values
// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
// currently experimental guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
napi.h Outdated
@@ -244,9 +244,9 @@ namespace Napi {
bool IsNull() const; ///< Tests if a value is a null JavaScript value.
bool IsBoolean() const; ///< Tests if a value is a JavaScript boolean.
bool IsNumber() const; ///< Tests if a value is a JavaScript number.
// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
// currently experimental guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
napi.h Outdated
@@ -321,9 +321,9 @@ namespace Napi {
double DoubleValue() const; ///< Converts a Number value to a 64-bit floating-point value.
};

// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
// currently experimental guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
napi.h Outdated
@@ -851,9 +851,9 @@ namespace Napi {
: std::is_same<T, uint32_t>::value ? napi_uint32_array
: std::is_same<T, float>::value ? napi_float32_array
: std::is_same<T, double>::value ? napi_float64_array
// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with definition of NAPI_EXPERIMENTAL that it is
// currently experimental guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
test/bigint.cc Outdated
@@ -1,11 +1,12 @@
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
Copy link
Member

@mhdawson mhdawson Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
// currently experimental, but tests can't tell which version of Node.js supports
// which experimental features. Temporarily guard with NODE_MAJOR_VERISION.
// Guard with version of NAPI_VERSION that it is

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 think experimental feature tests do tell which Node.js versions it could be run on. And we should continuously update the test table on each backport.

And experimental features were always temporary status, IMO. It is not necessary to point out that an experimental guard flag is temporary, IMO.

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 it is important to point out that we should NOT use NODE_MAJOR_VERSION once it is not experimental.

I'd be ok with:

// currently experimental temporarily guard with NODE_MAJOR_VERISION.
// Once its no longer experimental guard with version of NAPI_VERSION that it is release in.

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
test/binding.cc Outdated
@@ -11,9 +10,9 @@ Object InitBasicTypesArray(Env env);
Object InitBasicTypesBoolean(Env env);
Object InitBasicTypesNumber(Env env);
Object InitBasicTypesValue(Env env);
// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
// currently experimental, but tests can't tell which version of Node.js supports
// which experimental features. Temporarily guard with NODE_MAJOR_VERISION.
// Guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
test/binding.cc Outdated
@@ -56,9 +55,9 @@ Object Init(Env env, Object exports) {
exports.Set("basic_types_boolean", InitBasicTypesBoolean(env));
exports.Set("basic_types_number", InitBasicTypesNumber(env));
exports.Set("basic_types_value", InitBasicTypesValue(env));
// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
// currently experimental, but tests can't tell which version of Node.js supports
// which experimental features. Temporarily guard with NODE_MAJOR_VERISION.
// Guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
test/typedarray.cc Outdated
@@ -1,4 +1,8 @@
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
// currently experimental, but tests can't tell which version of Node.js supports
// which experimental features. Temporarily guard with NODE_MAJOR_VERISION.
// Guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
test/typedarray.cc Outdated
@@ -65,9 +69,9 @@ Value CreateTypedArray(const CallbackInfo& info) {
NAPI_TYPEDARRAY_NEW(Float64Array, info.Env(), length, napi_float64_array) :
NAPI_TYPEDARRAY_NEW_BUFFER(Float64Array, info.Env(), length, buffer, bufferOffset,
napi_float64_array);
// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
// currently experimental, but tests can't tell which version of Node.js supports
// which experimental features. Temporarily guard with NODE_MAJOR_VERISION.
// Guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
test/typedarray.cc Outdated
@@ -101,9 +105,9 @@ Value GetTypedArrayType(const CallbackInfo& info) {
case napi_uint32_array: return String::New(info.Env(), "uint32");
case napi_float32_array: return String::New(info.Env(), "float32");
case napi_float64_array: return String::New(info.Env(), "float64");
// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
// currently experimental, but tests can't tell which version of Node.js supports
// which experimental features. Temporarily guard with NODE_MAJOR_VERISION.
// Guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
test/typedarray.cc Outdated
@@ -143,9 +147,9 @@ Value GetTypedArrayElement(const CallbackInfo& info) {
return Number::New(info.Env(), array.As<Float32Array>()[index]);
case napi_float64_array:
return Number::New(info.Env(), array.As<Float64Array>()[index]);
// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
// currently experimental, but tests can't tell which version of Node.js supports
// which experimental features. Temporarily guard with NODE_MAJOR_VERISION.
// Guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
test/typedarray.cc Outdated
@@ -189,9 +193,9 @@ void SetTypedArrayElement(const CallbackInfo& info) {
case napi_float64_array:
array.As<Float64Array>()[index] = value.DoubleValue();
break;
// currently experimental guard with version of NAPI_VERSION that it is
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
// currently experimental, but tests can't tell which version of Node.js supports
// which experimental features. Temporarily guard with NODE_MAJOR_VERISION.
// Guard with version of NAPI_VERSION that it is

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
test/index.js Outdated

if (nodeMajorVersion < 10) {
// currently experimental only test if node major version
// is set to experimental. We can't use napi_experimental here
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// is set to experimental. We can't use napi_experimental here
is >= were it exists. We can't use NAPI_EXPERIMENTAL here
as the tests can't tell which version of Node.js support which
experimental versions.

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
test/index.js Outdated

if (nodeMajorVersion < 10) {
// currently experimental only test if node major version
// is set to experimental. We can't use napi_experimental here
// as that is not supported as a number on earlier
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// as that is not supported as a number on earlier

mhdawson
mhdawson reviewed Oct 31, 2019
View reviewed changes
test/index.js Outdated

if (nodeMajorVersion < 10) {
// currently experimental only test if node major version
// is set to experimental. We can't use napi_experimental here
// as that is not supported as a number on earlier
// Node.js versions. Once bigint is in a release
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Node.js versions. Once bigint is in a release
Once bigint is in a release

@mhdawson
Copy link
Member

@legendecas having looked at this carefully I agree that we need to guard the tests with the NODE_MAJOR_VERSION but the comment need to say that when the feature is no longer experimental we'll guard with with the NAPI_VERSION instead. I've made suggested comments.

If you think those make sense please accept them and let me know and then I'll run some CI tests to validate and we can get this landed.

@legendecas
Copy link
Member Author

There are many comments with similar content. Posting what I've replied as links:

Also, I've updated the comment in test/index.js to indicate when the feature is no longer experimental we'll guard with the NAPI_VERSION instead.

@legendecas legendecas force-pushed the experiment-feature-test branch from 34af5b3 to 5fae20a Compare November 1, 2019 16:46
@legendecas
Copy link
Member Author

@mhdawson I've updated all the comments that mentioned experimental guard with your concern of note on future works. PTAL :)

@mhdawson
Copy link
Member

mhdawson commented Nov 1, 2019

@legendecas thanks. Looking now.

mhdawson
mhdawson approved these changes Nov 1, 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 295e560 into nodejs:master Nov 1, 2019
@mhdawson
Copy link
Member

mhdawson commented Nov 1, 2019

Landed as 295e560

@legendecas legendecas deleted the experiment-feature-test branch November 2, 2019 01:51
@legendecas
Copy link
Member Author

@mhdawson @gabrielschulhof I think the test script of https://ci.nodejs.org/job/node-test-node-addon-api-new could be simplified as same as what's done to .travis.yml in this PR. How do we make changes to it?

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
test: improve guards for experimental features
d9dbfae 
NAPI_EXPERIMENTAL would be expand to NAPI_VERSION=2147483647 if
NAPI_VERSION is not defined. It would be not compatible with various
Node.js versions if we use NAPI_VERSION > 2147483646 mixed with
definition of NAPI_EXPERIMENTAL. This fix introduced NODE_MAJOR_VERSION
to allow more precise control over test sets that which set should be
enabled on a Node.js release.

PR-URL: nodejs/node-addon-api#545
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
test: improve guards for experimental features
4481f6f 
NAPI_EXPERIMENTAL would be expand to NAPI_VERSION=2147483647 if
NAPI_VERSION is not defined. It would be not compatible with various
Node.js versions if we use NAPI_VERSION > 2147483646 mixed with
definition of NAPI_EXPERIMENTAL. This fix introduced NODE_MAJOR_VERSION
to allow more precise control over test sets that which set should be
enabled on a Node.js release.

PR-URL: nodejs/node-addon-api#545
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
test: improve guards for experimental features
e1b8b62 
NAPI_EXPERIMENTAL would be expand to NAPI_VERSION=2147483647 if
NAPI_VERSION is not defined. It would be not compatible with various
Node.js versions if we use NAPI_VERSION > 2147483646 mixed with
definition of NAPI_EXPERIMENTAL. This fix introduced NODE_MAJOR_VERSION
to allow more precise control over test sets that which set should be
enabled on a Node.js release.

PR-URL: nodejs/node-addon-api#545
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
test: improve guards for experimental features
63f9d06 
NAPI_EXPERIMENTAL would be expand to NAPI_VERSION=2147483647 if
NAPI_VERSION is not defined. It would be not compatible with various
Node.js versions if we use NAPI_VERSION > 2147483646 mixed with
definition of NAPI_EXPERIMENTAL. This fix introduced NODE_MAJOR_VERSION
to allow more precise control over test sets that which set should be
enabled on a Node.js release.

PR-URL: nodejs/node-addon-api#545
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
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

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@NickNaso NickNaso Awaiting requested review from NickNaso

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

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