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

Merged
merged 5 commits into from
Nov 1, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,7 @@ install:
script:
# Travis CI sets NVM_NODEJS_ORG_MIRROR, but it makes node-gyp fail to download headers for nightly builds.
- unset NVM_NODEJS_ORG_MIRROR
- NODEJS_MAJOR_VERSION=$(node -p "process.versions.node.match(/\d+/)[0]")

- |
if [ ${NODEJS_MAJOR_VERSION} -gt 11 ]; then
npm test
else
npm test $NPMOPT --NAPI_VERSION=$(node -p "process.versions.napi")
fi
- npm test
after_success:
- cpp-coveralls --gcov-options '\-lp' --build-root test/build --exclude test
8 changes: 4 additions & 4 deletions napi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#ifdef NAPI_EXPERIMENTAL
inline bool Value::IsBigInt() const {
return Type() == napi_bigint;
}
Expand Down Expand Up @@ -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.

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#ifdef NAPI_EXPERIMENTAL
////////////////////////////////////////////////////////////////////////////////
// BigInt Class
////////////////////////////////////////////////////////////////////////////////
Expand Down
20 changes: 10 additions & 10 deletions napi.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#ifdef NAPI_EXPERIMENTAL
class BigInt;
#endif // NAPI_EXPERIMENTAL
#if (NAPI_VERSION > 4)
Expand All @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#ifdef NAPI_EXPERIMENTAL
typedef TypedArrayOf<int64_t> BigInt64Array; ///< Typed array of signed 64-bit integers
typedef TypedArrayOf<uint64_t> BigUint64Array; ///< Typed array of unsigned 64-bit integers
#endif // NAPI_EXPERIMENTAL
Expand Down Expand Up @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#ifdef NAPI_EXPERIMENTAL
bool IsBigInt() const; ///< Tests if a value is a JavaScript bigint.
#endif // NAPI_EXPERIMENTAL
#if (NAPI_VERSION > 4)
Expand Down Expand Up @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#ifdef NAPI_EXPERIMENTAL
/// A JavaScript bigint value.
class BigInt : public Value {
public:
Expand Down Expand Up @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#ifdef NAPI_EXPERIMENTAL
: std::is_same<T, int64_t>::value ? napi_bigint64_array
: std::is_same<T, uint64_t>::value ? napi_biguint64_array
#endif // NAPI_EXPERIMENTAL
Expand Down
7 changes: 4 additions & 3 deletions test/bigint.cc
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
// currently experimental guard with version of NODE_MAJOR_VERSION that it is
Copy link
Member

@mhdawson mhdawson Oct 31, 2019

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.

// released in once it is no longer experimental
#if (NODE_MAJOR_VERSION >= 10)

#define NAPI_EXPERIMENTAL
#include "napi.h"

using namespace Napi;

// currently experimental guard with version of NAPI_VERSION that it is
// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
namespace {

Value IsLossless(const CallbackInfo& info) {
Expand Down
9 changes: 4 additions & 5 deletions test/binding.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#define NAPI_EXPERIMENTAL
#include "napi.h"

using namespace Napi;
Expand All @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#if (NODE_MAJOR_VERSION >= 10)
Object InitBigInt(Env env);
#endif
Object InitBuffer(Env env);
Expand Down Expand Up @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#if (NODE_MAJOR_VERSION >= 10)
exports.Set("bigint", InitBigInt(env));
#endif
#if (NAPI_VERSION > 4)
Expand Down
4 changes: 3 additions & 1 deletion test/binding.gyp
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
'variables': {
'NAPI_VERSION%': "",
'NAPI_VERSION%': "<!(node -p \"process.versions.napi\")",
'NODE_MAJOR_VERSION%': "<!(node -p \"process.versions.node.match(/\\d+/)[0]\")",
'disable_deprecated': "<!(node -p \"process.env['npm_config_disable_deprecated']\")"
},
'target_defaults': {
Expand Down Expand Up @@ -57,6 +58,7 @@
}
}]
],
'defines': ['NODE_MAJOR_VERSION=<@(NODE_MAJOR_VERSION)'],
'include_dirs': ["<!@(node -p \"require('../').include\")"],
'dependencies': ["<!(node -p \"require('../').gyp\")"],
'cflags': [ '-Werror', '-Wall', '-Wextra', '-Wpedantic', '-Wunused-parameter' ],
Expand Down
1 change: 0 additions & 1 deletion test/date.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#define NAPI_EXPERIMENTAL
#include "napi.h"

using namespace Napi;
Expand Down
22 changes: 12 additions & 10 deletions test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,12 @@ let testModules = [
'version_management'
];

if ((process.env.npm_config_NAPI_VERSION !== undefined) &&
(process.env.npm_config_NAPI_VERSION < 50000)) {
// currently experimental only test if NAPI_VERSION
// is set to experimental. We can't use C max int
const napiVersion = Number(process.versions.napi)
const nodeMajorVersion = Number(process.versions.node.match(/\d+/)[0])

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.

// 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

// 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

// this should be guarded on the napi version
Expand All @@ -60,25 +62,25 @@ if ((process.env.npm_config_NAPI_VERSION !== undefined) &&
testModules.splice(testModules.indexOf('typedarray-bigint'), 1);
}

if ((process.env.npm_config_NAPI_VERSION !== undefined) &&
(process.env.npm_config_NAPI_VERSION < 3)) {
if (napiVersion < 3) {
testModules.splice(testModules.indexOf('callbackscope'), 1);
testModules.splice(testModules.indexOf('version_management'), 1);
}

if ((process.env.npm_config_NAPI_VERSION !== undefined) &&
(process.env.npm_config_NAPI_VERSION < 4)) {
if (napiVersion < 4) {
testModules.splice(testModules.indexOf('threadsafe_function/threadsafe_function_ptr'), 1);
testModules.splice(testModules.indexOf('threadsafe_function/threadsafe_function_unref'), 1);
testModules.splice(testModules.indexOf('threadsafe_function/threadsafe_function'), 1);
}

if ((process.env.npm_config_NAPI_VERSION !== undefined) &&
(process.env.npm_config_NAPI_VERSION < 5)) {
if (napiVersion < 5) {
testModules.splice(testModules.indexOf('date'), 1);
}

if (typeof global.gc === 'function') {
console.log(`Testing with N-API Version '${napiVersion}'.`);
console.log(`Testing with Node.js Major Version '${nodeMajorVersion}'.\n`);

console.log('Starting test suite\n');

// Requiring each module runs tests in the module.
Expand Down
20 changes: 12 additions & 8 deletions test/typedarray.cc
Original file line number Diff line number Diff line change
@@ -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

// released in once it is no longer experimental
#if (NODE_MAJOR_VERSION >= 10)
#define NAPI_EXPERIMENTAL
#endif
#include "napi.h"

using namespace Napi;
Expand Down Expand Up @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#if (NODE_MAJOR_VERSION >= 10)
} else if (arrayType == "bigint64") {
return buffer.IsUndefined() ?
NAPI_TYPEDARRAY_NEW(BigInt64Array, info.Env(), length, napi_bigint64_array) :
Expand Down Expand Up @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#if (NODE_MAJOR_VERSION >= 10)
case napi_bigint64_array: return String::New(info.Env(), "bigint64");
case napi_biguint64_array: return String::New(info.Env(), "biguint64");
#endif
Expand Down Expand Up @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#if (NODE_MAJOR_VERSION >= 10)
case napi_bigint64_array:
return BigInt::New(info.Env(), array.As<BigInt64Array>()[index]);
case napi_biguint64_array:
Expand Down Expand Up @@ -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

// released in once it is no longer experimental
#if (NAPI_VERSION > 2147483646)
#if (NODE_MAJOR_VERSION >= 10)
case napi_bigint64_array: {
bool lossless;
array.As<BigInt64Array>()[index] = value.As<BigInt>().Int64Value(&lossless);
Expand Down