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

bigint tests running/failing on earlier Node.js versions #349

Closed
mhdawson opened this issue Sep 18, 2018 · 11 comments
Closed

bigint tests running/failing on earlier Node.js versions #349

mhdawson opened this issue Sep 18, 2018 · 11 comments

Comments

@mhdawson
Copy link
Member

The CI is broken for Node.js versions lower than master because the bigint tests are running even though the methods don't exist in those earlier versions.

We need to guard execution of those tests to only run on 11.x and later.
@mhdawson
Copy link
Member Author

@devsnek, @gabrielschulhof

I don't see a quick/easy way to just make the bigint tests to build/run on 11.x or later where they exist.

I think we should turn off the bigint tests, for the next few days until we can figure out how to only have them run in the right versions? Any objections ?

@mhdawson
Copy link
Member Author

Looks like also run ok on 10.x as well, but fail to build on 8.x and 6.x

@mhdawson
Copy link
Member Author

I think we'll need to figure out how to guard which tests build/run by supported napi version. I think we've not run into this before because so far pretty much everything used by node-addon-api has been available in all versions.

@devsnek
Copy link
Member

devsnek commented Sep 18, 2018

@mhdawson sounds okay. fwiw i don't really know how your versioning system works so you do you.

@NickNaso
Copy link
Member

I can confirm that the problems are only for Node.js version 6.x and 8.x.

@NickNaso
Copy link
Member

NickNaso commented Sep 19, 2018 •
edited
Loading

We have the problem also for Napi::TypedArray, I tried to disable by hand the test for Napi::BigInt and Napi::TypedArray and all work fine. For the moment I don't know how to dinamically do the same thing with a little effort.
I tested on Node.js v8.x and 6.x on macOS.
Is there a plan to port the api we need on 6.x and 8x?

@NickNaso
Copy link
Member

I worked a little on this issue and the only way I thought is to dynamically create the following files:

  • binding.gyp
  • binding.cc
  • List of modules in the file test/index.js

It's something like a pre-test process where we need to execute the following steps:

  • Check if binding.gyp, binding.cc and the list of modules to test exists and in that case delete them.
  • Get the version of Node.js and based on that build binding.gyp, binding.cc and the lists of modules to test.

We can do that creating a file where to specify the test we want execute on the specific version of Node.js like reported below:

{
    6: [
     // List of test for Node.js 6
    ],
    8: [
    // List of test for Node.js 8
   ]
   // ...
}

For now this is my idea maybe there is a better way.

mhdawson added a commit to mhdawson/node-addon-api that referenced this issue Sep 19, 2018
@mhdawson
test: add ability to control experimental tests
615d54f 
Add the ability to specify a NAPI_VERSION which
limits the tests executed to those supported by that
version.

As an example tests can be built/run as:
  npm test --NAPI_VERSION=3

Fixes: nodejs#349
@mhdawson mhdawson mentioned this issue Sep 19, 2018
Closed
romandev added a commit to romandev/node-addon-api that referenced this issue Sep 20, 2018
@romandev
Fix a syntax error in test
989953e 
`npm test` command has been failed in old version node.js since
introducing `bitint` tests. The problem is caused that old version V8
doesn't support the bigint syntax(e.g. 11n, 22n). So, we just wrap the
syntax using `eval()` method to avoid the problem.

/home/jinho_bang/up/test/typedarray.js:81
      t[3] = 11n;
             ^^

SyntaxError: Invalid or unexpected token
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:607:28)
    at Object.Module._extensions..js (module.js:654:10)
    at Module.load (module.js:556:32)
    at tryModuleLoad (module.js:499:12)
    at Function.Module._load (module.js:491:3)
    at Module.require (module.js:587:17)
    at require (internal/module.js:11:18)
    at testModules.forEach.name (/home/jinho_bang/up/test/index.js:54:5)

Fixes: nodejs#349
@romandev romandev mentioned this issue Sep 20, 2018
Closed
romandev added a commit to romandev/node-addon-api that referenced this issue Sep 20, 2018
@romandev
Fix a syntax error in test
eaf4e7e 
`npm test` command has been failed in old version node.js since
introducing `bitint` tests. The problem is caused that old version V8
doesn't support the bigint syntax(e.g. 11n, 22n). So, we just wrap the
syntax using `eval()` method to avoid the problem.

/home/jinho_bang/up/test/typedarray.js:81
      t[3] = 11n;
             ^^

SyntaxError: Invalid or unexpected token
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:607:28)
    at Object.Module._extensions..js (module.js:654:10)
    at Module.load (module.js:556:32)
    at tryModuleLoad (module.js:499:12)
    at Function.Module._load (module.js:491:3)
    at Module.require (module.js:587:17)
    at require (internal/module.js:11:18)
    at testModules.forEach.name (/home/jinho_bang/up/test/index.js:54:5)

Fixes: nodejs#349
@mhdawson
Copy link
Member Author

Should be fixed by #350

@mhdawson
Copy link
Member Author

I've recently pushed a final set of changes and have CI runs going across 6.x, 8.x, 10.x and master. They have all completed except for AIX so I think it looks good.

@romandev, @NickNaso, @devsnek can you review #350

@mhdawson
Copy link
Member Author

FYI, All CI runs have now completed and were good on my branch.

@NickNaso
Copy link
Member

@mhdawson I downloaded your work from https://github.com/mhdawson/node-addon-api/tree/fixci-bigint and tried to execute the test but I get an error like reported below:

screen shot 2018-09-20 at 14 22 45

I'm working on Node.js version8.12.0 and macOs 10.13.6. Maybe the problem is in nvm because i'm using it to switch between different version of Node.js.

mhdawson added a commit that referenced this issue Sep 20, 2018
@mhdawson
test: add ability to control experimental tests
73fed84 
Add the ability to specify a NAPI_VERSION which
limits the tests executed to those supported by that
version.

As an example tests can be built/run as:
  npm test --NAPI_VERSION=3

PR-URL: #350
Fixes: #349
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jinho Bang <zino@chromium.org>
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this issue Aug 24, 2022
@kevindavies8
test: add ability to control experimental tests
70f9ed3 
Add the ability to specify a NAPI_VERSION which
limits the tests executed to those supported by that
version.

As an example tests can be built/run as:
  npm test --NAPI_VERSION=3

PR-URL: nodejs/node-addon-api#350
Fixes: nodejs/node-addon-api#349
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jinho Bang <zino@chromium.org>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this issue Aug 26, 2022
@Marlyfleitas
test: add ability to control experimental tests
6ef2f4b 
Add the ability to specify a NAPI_VERSION which
limits the tests executed to those supported by that
version.

As an example tests can be built/run as:
  npm test --NAPI_VERSION=3

PR-URL: nodejs/node-addon-api#350
Fixes: nodejs/node-addon-api#349
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jinho Bang <zino@chromium.org>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this issue Sep 19, 2022
@wroy7860
test: add ability to control experimental tests
1c6bd82 
Add the ability to specify a NAPI_VERSION which
limits the tests executed to those supported by that
version.

As an example tests can be built/run as:
  npm test --NAPI_VERSION=3

PR-URL: nodejs/node-addon-api#350
Fixes: nodejs/node-addon-api#349
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jinho Bang <zino@chromium.org>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this issue Aug 11, 2023
test: add ability to control experimental tests
92ed325 
Add the ability to specify a NAPI_VERSION which
limits the tests executed to those supported by that
version.

As an example tests can be built/run as:
  npm test --NAPI_VERSION=3

PR-URL: nodejs/node-addon-api#350
Fixes: nodejs/node-addon-api#349
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Jinho Bang <zino@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@NickNaso @devsnek @mhdawson