Make tests work on all Node versions (#44)

 - Make GC tests (arraybuffer, buffer, external) async, to account for
   different GC behavior with different versions of V8 and ChakraCore,
   similar to nodejs/node#13121
 - In test/index.js, use the --napi-modules and --expose-gc
   command-line flags automatically.
 - Add missing entry for object tests in index.js.
 - Remove check for writable attribute on accessor property
   descriptors; it should not be there according to the JS spec.
 - Remove the explicit dependency on node-gyp in package.json.
   (NPM carries its own copy of node-gyp.)
 - Fix improper rethrow of an Error caught by reference that caused
   a double napi_ref delete, which failed in release builds of
   Node-ChakraCore.

package.json

@@ -15,9 +15,7 @@
   ],
   "dependencies": {},
   "description": "Node.js API (N-API)",
-  "devDependencies": {
-    "node-gyp": "^3.6.0"
-  },
+  "devDependencies": {},
   "directories": {},
   "homepage": "https://github.com/nodejs/node-addon-api",
   "license": "MIT",
@@ -31,7 +29,7 @@
   },
   "scripts": {
     "pretest": "node-gyp rebuild -C test",
-    "test": "node --expose-gc test"
+    "test": "node test"
   },
   "version": "0.3.0"
 }

test/arraybuffer.cc

@@ -35,6 +35,8 @@ Value CreateBuffer(const CallbackInfo& info) {
 }
 
 Value CreateExternalBuffer(const CallbackInfo& info) {
+  finalizeCount = 0;
+
   ArrayBuffer buffer = ArrayBuffer::New(info.Env(), testData, testLength);
 
   if (buffer.ByteLength() != testLength) {
@@ -50,6 +52,8 @@ Value CreateExternalBuffer(const CallbackInfo& info) {
 }
 
 Value CreateExternalBufferWithFinalize(const CallbackInfo& info) {
+  finalizeCount = 0;
+
   uint8_t* data = new uint8_t[testLength];
 
   ArrayBuffer buffer = ArrayBuffer::New(
@@ -74,6 +78,8 @@ Value CreateExternalBufferWithFinalize(const CallbackInfo& info) {
 }
 
 Value CreateExternalBufferWithFinalizeHint(const CallbackInfo& info) {
+  finalizeCount = 0;
+
   uint8_t* data = new uint8_t[testLength];
 
   char* hint = nullptr;

test/arraybuffer.js

@@ -2,30 +2,52 @@
 const buildType = process.config.target_defaults.default_configuration;
 const binding = require(`./build/${buildType}/binding.node`);
 const assert = require('assert');
+const testUtil = require('./testUtil');
 
-let test = binding.arraybuffer.createBuffer();
-binding.arraybuffer.checkBuffer(test);
-assert.ok(test instanceof ArrayBuffer);
+testUtil.runGCTests([
+  'Internal ArrayBuffer',
+  () => {
+    const test = binding.arraybuffer.createBuffer();
+    binding.arraybuffer.checkBuffer(test);
+    assert.ok(test instanceof ArrayBuffer);
 
-let test2 = test.slice(0);
-binding.arraybuffer.checkBuffer(test2);
+    const test2 = test.slice(0);
+    binding.arraybuffer.checkBuffer(test2);
+  },
 
-test = binding.arraybuffer.createExternalBuffer();
-binding.arraybuffer.checkBuffer(test);
-assert.ok(test instanceof ArrayBuffer);
+  'External ArrayBuffer',
+  () => {
+    const test = binding.arraybuffer.createExternalBuffer();
+    binding.arraybuffer.checkBuffer(test);
+    assert.ok(test instanceof ArrayBuffer);
+    assert.strictEqual(0, binding.arraybuffer.getFinalizeCount());
+  },
+  () => {
+    global.gc();
+    assert.strictEqual(0, binding.arraybuffer.getFinalizeCount());
+  },
 
-test = binding.arraybuffer.createExternalBufferWithFinalize();
-binding.arraybuffer.checkBuffer(test);
-assert.ok(test instanceof ArrayBuffer);
-assert.strictEqual(0, binding.arraybuffer.getFinalizeCount());
-test = null;
-global.gc();
-assert.strictEqual(1, binding.arraybuffer.getFinalizeCount());
+  'External ArrayBuffer with finalizer',
+  () => {
+    const test = binding.arraybuffer.createExternalBufferWithFinalize();
+    binding.arraybuffer.checkBuffer(test);
+    assert.ok(test instanceof ArrayBuffer);
+    assert.strictEqual(0, binding.arraybuffer.getFinalizeCount());
+  },
+  () => {
+    global.gc();
+    assert.strictEqual(1, binding.arraybuffer.getFinalizeCount());
+  },
 
-test = binding.arraybuffer.createExternalBufferWithFinalizeHint();
-binding.arraybuffer.checkBuffer(test);
-assert.ok(test instanceof ArrayBuffer);
-assert.strictEqual(1, binding.arraybuffer.getFinalizeCount());
-test = null;
-global.gc();
-assert.strictEqual(2, binding.arraybuffer.getFinalizeCount());
+  'External ArrayBuffer with finalizer hint',
+  () => {
+    const test = binding.arraybuffer.createExternalBufferWithFinalizeHint();
+    binding.arraybuffer.checkBuffer(test);
+    assert.ok(test instanceof ArrayBuffer);
+    assert.strictEqual(0, binding.arraybuffer.getFinalizeCount());
+  },
+  () => {
+    global.gc();
+    assert.strictEqual(1, binding.arraybuffer.getFinalizeCount());
+  },
+]);

test/buffer.cc

@@ -37,6 +37,8 @@ Value CreateBuffer(const CallbackInfo& info) {
 }
 
 Value CreateExternalBuffer(const CallbackInfo& info) {
+  finalizeCount = 0;
+
   Buffer<uint16_t> buffer = Buffer<uint16_t>::New(
     info.Env(),
     testData,
@@ -55,6 +57,8 @@ Value CreateExternalBuffer(const CallbackInfo& info) {
 }
 
 Value CreateExternalBufferWithFinalize(const CallbackInfo& info) {
+  finalizeCount = 0;
+
   uint16_t* data = new uint16_t[testLength];
 
   Buffer<uint16_t> buffer = Buffer<uint16_t>::New(
@@ -79,6 +83,8 @@ Value CreateExternalBufferWithFinalize(const CallbackInfo& info) {
 }
 
 Value CreateExternalBufferWithFinalizeHint(const CallbackInfo& info) {
+  finalizeCount = 0;
+
   uint16_t* data = new uint16_t[testLength];
 
   char* hint = nullptr;

test/buffer.js

@@ -2,35 +2,60 @@
 const buildType = process.config.target_defaults.default_configuration;
 const binding = require(`./build/${buildType}/binding.node`);
 const assert = require('assert');
+const testUtil = require('./testUtil');
 
-let test = binding.buffer.createBuffer();
-binding.buffer.checkBuffer(test);
-assert.ok(test instanceof Buffer);
+testUtil.runGCTests([
+  'Internal Buffer',
+  () => {
+    const test = binding.buffer.createBuffer();
+    binding.buffer.checkBuffer(test);
+    assert.ok(test instanceof Buffer);
 
-let test2 = Buffer.alloc(test.length);
-test.copy(test2);
-binding.buffer.checkBuffer(test2);
+    const test2 = Buffer.alloc(test.length);
+    test.copy(test2);
+    binding.buffer.checkBuffer(test2);
+  },
 
-test = binding.buffer.createBufferCopy();
-binding.buffer.checkBuffer(test);
-assert.ok(test instanceof Buffer);
+  'Buffer copy',
+  () => {
+    const test = binding.buffer.createBufferCopy();
+    binding.buffer.checkBuffer(test);
+    assert.ok(test instanceof Buffer);
+  },
 
-test = binding.buffer.createExternalBuffer();
-binding.buffer.checkBuffer(test);
-assert.ok(test instanceof Buffer);
+  'External Buffer',
+  () => {
+    const test = binding.buffer.createExternalBuffer();
+    binding.buffer.checkBuffer(test);
+    assert.ok(test instanceof Buffer);
+    assert.strictEqual(0, binding.buffer.getFinalizeCount());
+  },
+  () => {
+      global.gc();
+      assert.strictEqual(0, binding.buffer.getFinalizeCount());
+  },
 
-test = binding.buffer.createExternalBufferWithFinalize();
-binding.buffer.checkBuffer(test);
-assert.ok(test instanceof Buffer);
-assert.strictEqual(0, binding.buffer.getFinalizeCount());
-test = null;
-global.gc();
-assert.strictEqual(1, binding.buffer.getFinalizeCount());
+  'External Buffer with finalizer',
+  () => {
+    const test = binding.buffer.createExternalBufferWithFinalize();
+    binding.buffer.checkBuffer(test);
+    assert.ok(test instanceof Buffer);
+    assert.strictEqual(0, binding.buffer.getFinalizeCount());
+  },
+  () => {
+      global.gc();
+      assert.strictEqual(1, binding.buffer.getFinalizeCount());
+  },
 
-test = binding.buffer.createExternalBufferWithFinalizeHint();
-binding.buffer.checkBuffer(test);
-assert.ok(test instanceof Buffer);
-assert.strictEqual(1, binding.buffer.getFinalizeCount());
-test = null;
-global.gc();
-assert.strictEqual(2, binding.buffer.getFinalizeCount());
+  'External Buffer with finalizer hint',
+  () => {
+      const test = binding.buffer.createExternalBufferWithFinalizeHint();
+      binding.buffer.checkBuffer(test);
+      assert.ok(test instanceof Buffer);
+      assert.strictEqual(0, binding.buffer.getFinalizeCount());
+  },
+  () => {
+      global.gc();
+      assert.strictEqual(1, binding.buffer.getFinalizeCount());
+  },
+]);

test/error.cc

@@ -51,7 +51,7 @@ void CatchAndRethrowError(const CallbackInfo& info) {
     thrower({});
   } catch (Error& e) {
      e.Set("caught", Boolean::New(info.Env(), true));
-     throw e;
+     throw;
   }
 }
 
@@ -68,7 +68,7 @@ void CatchAndRethrowErrorThatEscapesScope(const CallbackInfo& info) {
     ThrowErrorThatEscapesScope(info);
   } catch (Error& e) {
     e.Set("caught", Boolean::New(info.Env(), true));
-    throw e;
+    throw;
   }
 }
 

test/external.cc

@@ -8,10 +8,12 @@ int testData = 1;
 int finalizeCount = 0;
 
 Value CreateExternal(const CallbackInfo& info) {
+  finalizeCount = 0;
   return External<int>::New(info.Env(), &testData);
 }
 
 Value CreateExternalWithFinalize(const CallbackInfo& info) {
+  finalizeCount = 0;
   return External<int>::New(info.Env(), new int(1),
     [](Env env, int* data) {
       delete data;
@@ -20,6 +22,7 @@ Value CreateExternalWithFinalize(const CallbackInfo& info) {
 }
 
 Value CreateExternalWithFinalizeHint(const CallbackInfo& info) {
+  finalizeCount = 0;
   char* hint = nullptr;
   return External<int>::New(info.Env(), new int(1),
     [](Env env, int* data, char* hint) {

test/external.js

@@ -2,26 +2,39 @@
 const buildType = process.config.target_defaults.default_configuration;
 const binding = require(`./build/${buildType}/binding.node`);
 const assert = require('assert');
+const testUtil = require('./testUtil');
 
-assert.strictEqual(0, binding.external.getFinalizeCount());
+testUtil.runGCTests([
+  'External without finalizer',
+  () => {
+    const test = binding.external.createExternal();
+    assert.strictEqual(typeof test, 'object');
+    binding.external.checkExternal(test);
+    assert.strictEqual(0, binding.external.getFinalizeCount());
+  },
+  () => {
+    assert.strictEqual(0, binding.external.getFinalizeCount());
+  },
 
-let test = binding.external.createExternal();
-assert.strictEqual(typeof test, 'object');
-binding.external.checkExternal(test);
-test = null;
-global.gc();
-assert.strictEqual(0, binding.external.getFinalizeCount());
+  'External with finalizer',
+  () => {
+    const test = binding.external.createExternalWithFinalize();
+    assert.strictEqual(typeof test, 'object');
+    binding.external.checkExternal(test);
+    assert.strictEqual(0, binding.external.getFinalizeCount());
+  },
+  () => {
+    assert.strictEqual(1, binding.external.getFinalizeCount());
+  },
 
-test = binding.external.createExternalWithFinalize();
-assert.strictEqual(typeof test, 'object');
-binding.external.checkExternal(test);
-test = null;
-global.gc();
-assert.strictEqual(1, binding.external.getFinalizeCount());
-
-test = binding.external.createExternalWithFinalizeHint();
-assert.strictEqual(typeof test, 'object');
-binding.external.checkExternal(test);
-test = null;
-global.gc();
-assert.strictEqual(2, binding.external.getFinalizeCount());
+  'External with finalizer hint',
+  () => {
+    const test = binding.external.createExternalWithFinalizeHint();
+    assert.strictEqual(typeof test, 'object');
+    binding.external.checkExternal(test);
+    assert.strictEqual(0, binding.external.getFinalizeCount());
+  },
+  () => {
+    assert.strictEqual(1, binding.external.getFinalizeCount());
+  },
+]);

test/index.js

@@ -1,25 +1,30 @@
 'use strict';
 
-if (typeof global.gc !== 'function') {
-   throw new Error('Tests require --expose-gc flag.')
-}
-
 let testModules = [
-   'arraybuffer',
-   'asyncworker',
-   'buffer',
-   'error',
-   'external',
-   'function',
-   'name',
-   'typedarray',
+  'arraybuffer',
+  'asyncworker',
+  'buffer',
+  'error',
+  'external',
+  'function',
+  'name',
+  'object',
+  'typedaray',
 ];
 
-testModules.forEach(name => {
-   try {
-      require('./' + name);
-   }
-   catch (e) {
-      console.error(e);
-   }
-});
+if (typeof global.gc === 'function') {
+  // Requiring each module runs tests in the module.
+  testModules.forEach(name => {
+    require('./' + name);
+  });
+} else {
+  // Make it easier to run with the correct (version-dependent) command-line args.
+  const args = [ '--expose-gc', __filename ];
+  if (require('../index').isNodeApiBuiltin) {
+    args.splice(0, 0, '--napi-modules');
+  }
+  const child = require('child_process').spawnSync(process.argv[0], args, {
+    stdio: 'inherit',
+  });
+  process.exitCode = child.status;
+}

test/object.js

@@ -19,12 +19,10 @@ function testDefineProperties(nameType) {
   const obj = {};
   binding.object.defineProperties(obj, nameType);
 
-  assertPropertyIsNot(obj, 'readonlyAccessor', 'writable');
   assertPropertyIsNot(obj, 'readonlyAccessor', 'enumerable');
   assertPropertyIsNot(obj, 'readonlyAccessor', 'configurable');
   assert.strictEqual(obj.readonlyAccessor, true);
 
-  assertPropertyIs(obj, 'readwriteAccessor', 'writable');
   assertPropertyIsNot(obj, 'readwriteAccessor', 'enumerable');
   assertPropertyIsNot(obj, 'readwriteAccessor', 'configurable');
   obj.readwriteAccessor = false;