bigint tests running/failing on earlier Node.js versions

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.

@devsnek

@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 ?

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

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.

@mhdawson

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

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

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?

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.

test: add ability to control experimental tests

Fix a syntax error in test

Should be fixed by #350

@romandev

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

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

@mhdawson

@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:

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.

test: add ability to control experimental tests

Navigation Menu

Repository navigation

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

mhdawson commented on Sep 18, 2018

mhdawson commented on Sep 18, 2018

mhdawson commented on Sep 18, 2018

devsnek commented on Sep 18, 2018

NickNaso commented on Sep 18, 2018

NickNaso commented on Sep 19, 2018

NickNaso commented on Sep 19, 2018

mhdawson commented on Sep 20, 2018

mhdawson commented on Sep 20, 2018

mhdawson commented on Sep 20, 2018

NickNaso commented on Sep 20, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions