test: run interfering tests in their own process

Tests such as addon and addon_data both use SetInstanceData. Thus, they
overwrite each other's instance data. We change them both to run in a
child process such that they do not call SetInstanceData on init, but
rather they return a JS function on init which, when called, produces
the actual binding which needs SetInstanceData.

We also introduce a utility function for launching a test in its own
child process for the purpose of running tests that would otherwise
interfere with each other.

Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: #1325
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

test/addon.cc

@@ -17,6 +17,8 @@ class TestAddon : public Napi::Addon<TestAddon> {
                  {InstanceMethod("decrement", &TestAddon::Decrement)}))});
   }
 
+  ~TestAddon() { fprintf(stderr, "TestAddon::~TestAddon\n"); }
+
  private:
   Napi::Value Increment(const Napi::CallbackInfo& info) {
     return Napi::Number::New(info.Env(), ++value);
@@ -29,10 +31,14 @@ class TestAddon : public Napi::Addon<TestAddon> {
   uint32_t value = 42;
 };
 
+Napi::Value CreateAddon(const Napi::CallbackInfo& info) {
+  return TestAddon::Init(info.Env(), Napi::Object::New(info.Env()));
+}
+
 }  // end of anonymous namespace
 
 Napi::Object InitAddon(Napi::Env env) {
-  return TestAddon::Init(env, Napi::Object::New(env));
+  return Napi::Function::New<CreateAddon>(env, "CreateAddon");
 }
 
 #endif  // (NAPI_VERSION > 5)

test/addon.js

@@ -1,11 +1,7 @@
 'use strict';
 
-const assert = require('assert');
-
-module.exports = require('./common').runTest(test);
-
-function test (binding) {
-  assert.strictEqual(binding.addon.increment(), 43);
-  assert.strictEqual(binding.addon.increment(), 44);
-  assert.strictEqual(binding.addon.subObject.decrement(), 43);
-}
+module.exports = require('./common').runTestInChildProcess({
+  suite: 'addon',
+  testName: 'workingCode',
+  expectedStderr: ['TestAddon::~TestAddon']
+});

test/addon_data.cc

@@ -4,10 +4,10 @@
 #include "test_helper.h"
 
 // An overly elaborate way to get/set a boolean stored in the instance data:
-// 0. A boolean named "verbose" is stored in the instance data. The constructor
-//    for JS `VerboseIndicator` instances is also stored in the instance data.
+// 0. The constructor for JS `VerboseIndicator` instances, which have a private
+//    member named "verbose", is stored in the instance data.
 // 1. Add a property named "verbose" onto exports served by a getter/setter.
-// 2. The getter returns a object of type VerboseIndicator, which itself has a
+// 2. The getter returns an object of type VerboseIndicator, which itself has a
 //    property named "verbose", also served by a getter/setter:
 //    * The getter returns a boolean, indicating whether "verbose" is set.
 //    * The setter sets "verbose" on the instance data.

test/addon_data.js

@@ -1,46 +1,24 @@
 'use strict';
 
-const assert = require('assert');
-const { spawn } = require('child_process');
-const readline = require('readline');
+const common = require('./common');
 
-module.exports = require('./common').runTestWithBindingPath(test);
+module.exports = common.runTest(test);
 
-// Make sure the instance data finalizer is called at process exit. If the hint
-// is non-zero, it will be printed out by the child process.
-function testFinalizer (bindingName, hint, expected) {
-  return new Promise((resolve) => {
-    bindingName = bindingName.split('\\').join('\\\\');
-    const child = spawn(process.execPath, [
-      '-e',
-      `require('${bindingName}').addon_data(${hint}).verbose = true;`
-    ]);
-    const actual = [];
-    readline
-      .createInterface({ input: child.stderr })
-      .on('line', (line) => {
-        if (expected.indexOf(line) >= 0) {
-          actual.push(line);
-        }
-      })
-      .on('close', () => {
-        assert.deepStrictEqual(expected, actual);
-        resolve();
-      });
+async function test () {
+  await common.runTestInChildProcess({
+    suite: 'addon_data',
+    testName: 'workingCode'
   });
-}
-
-async function test (bindingName) {
-  const binding = require(bindingName).addon_data(0);
 
-  // Make sure it is possible to get/set instance data.
-  assert.strictEqual(binding.verbose.verbose, false);
-  binding.verbose = true;
-  assert.strictEqual(binding.verbose.verbose, true);
-  binding.verbose = false;
-  assert.strictEqual(binding.verbose.verbose, false);
+  await common.runTestInChildProcess({
+    suite: 'addon_data',
+    testName: 'cleanupWithoutHint',
+    expectedStderr: ['addon_data: Addon::~Addon']
+  });
 
-  await testFinalizer(bindingName, 0, ['addon_data: Addon::~Addon']);
-  await testFinalizer(bindingName, 42,
-    ['addon_data: Addon::~Addon', 'hint: 42']);
+  await common.runTestInChildProcess({
+    suite: 'addon_data',
+    testName: 'cleanupWithHint',
+    expectedStderr: ['addon_data: Addon::~Addon', 'hint: 42']
+  });
 }

test/child_processes/addon.js

@@ -0,0 +1,11 @@
+'use strict';
+const assert = require('assert');
+
+module.exports = {
+  workingCode: binding => {
+    const addon = binding.addon();
+    assert.strictEqual(addon.increment(), 43);
+    assert.strictEqual(addon.increment(), 44);
+    assert.strictEqual(addon.subObject.decrement(), 43);
+  }
+};

test/child_processes/addon_data.js

@@ -0,0 +1,24 @@
+'use strict';
+
+const assert = require('assert');
+
+// Make sure the instance data finalizer is called at process exit. If the hint
+// is non-zero, it will be printed out by the child process.
+const cleanupTest = (binding, hint) => {
+  binding.addon_data(hint).verbose = true;
+};
+
+module.exports = {
+  workingCode: binding => {
+    const addonData = binding.addon_data(0);
+
+    // Make sure it is possible to get/set instance data.
+    assert.strictEqual(addonData.verbose.verbose, false);
+    addonData.verbose = true;
+    assert.strictEqual(addonData.verbose.verbose, true);
+    addonData.verbose = false;
+    assert.strictEqual(addonData.verbose.verbose, false);
+  },
+  cleanupWithHint: binding => cleanupTest(binding, 42),
+  cleanupWithoutHint: binding => cleanupTest(binding, 0)
+};

test/child_processes/objectwrap_function.js

@@ -0,0 +1,22 @@
+'use strict';
+
+const assert = require('assert');
+const testUtil = require('../testUtil');
+
+module.exports = {
+  runTest: function (binding) {
+    return testUtil.runGCTests([
+      'objectwrap function',
+      () => {
+        const { FunctionTest } = binding.objectwrap_function();
+        const newConstructed = new FunctionTest();
+        const functionConstructed = FunctionTest();
+        assert(newConstructed instanceof FunctionTest);
+        assert(functionConstructed instanceof FunctionTest);
+        assert.throws(() => (FunctionTest(true)), /an exception/);
+      },
+      // Do one gc before returning.
+      () => {}
+    ]);
+  }
+};

test/common/index.js

@@ -3,6 +3,11 @@
 const assert = require('assert');
 const path = require('path');
 const { access } = require('node:fs/promises');
+const { spawn } = require('child_process');
+const { EOL } = require('os');
+const readline = require('readline');
+
+const escapeBackslashes = (pathString) => pathString.split('\\').join('\\\\');
 
 const noop = () => {};
 
@@ -26,7 +31,7 @@ function runCallChecks (exitCode) {
       context.name,
       context.messageSegment,
       context.actual);
-    console.log(context.stack.split('\n').slice(2).join('\n'));
+    console.log(context.stack.split(EOL).slice(2).join(EOL));
   });
 
   if (failed.length) process.exit(1);
@@ -190,3 +195,51 @@ exports.runTestWithBuildType = async function (test, buildType) {
   await Promise.resolve(test(buildType))
     .finally(exports.mustCall());
 };
+
+// Some tests have to run in their own process, otherwise they would interfere
+// with each other. Such tests export a factory function rather than the test
+// itself so as to avoid automatic instantiation, and therefore interference,
+// in the main process. Two examples are addon and addon_data, both of which
+// use Napi::Env::SetInstanceData(). This helper function provides a common
+// approach for running such tests.
+exports.runTestInChildProcess = function ({ suite, testName, expectedStderr }) {
+  return exports.runTestWithBindingPath((bindingName) => {
+    return new Promise((resolve) => {
+      bindingName = escapeBackslashes(bindingName);
+      // Test suites are assumed to be located here.
+      const suitePath = escapeBackslashes(path.join(__dirname, '..', 'child_processes', suite));
+      const child = spawn(process.execPath, [
+        '--expose-gc',
+        '-e',
+        `require('${suitePath}').${testName}(require('${bindingName}'))`
+      ]);
+      const resultOfProcess = { stderr: [] };
+
+      // Capture the exit code and signal.
+      child.on('close', (code, signal) => resolve(Object.assign(resultOfProcess, { code, signal })));
+
+      // Capture the stderr as an array of lines.
+      readline
+        .createInterface({ input: child.stderr })
+        .on('line', (line) => {
+          resultOfProcess.stderr.push(line);
+        });
+    }).then(actual => {
+      // Back up the stderr in case the assertion fails.
+      const fullStderr = actual.stderr.map(item => `from child process: ${item}`);
+      const expected = { stderr: expectedStderr, code: 0, signal: null };
+
+      if (!expectedStderr) {
+        // If we don't care about stderr, delete it.
+        delete actual.stderr;
+        delete expected.stderr;
+      } else {
+        // Otherwise we only care about expected lines in the actual stderr, so
+        // filter out everything else.
+        actual.stderr = actual.stderr.filter(line => expectedStderr.includes(line));
+      }
+
+      assert.deepStrictEqual(actual, expected, `Assertion for child process test ${suite}.${testName} failed:${EOL}` + fullStderr.join(EOL));
+    });
+  });
+};

test/index.js

@@ -60,6 +60,7 @@ function loadTestModules (currentDirectory = __dirname, pre = '') {
           file === 'binding.gyp' ||
           file === 'build' ||
           file === 'common' ||
+          file === 'child_processes' ||
           file === 'napi_child.js' ||
           file === 'testUtil.js' ||
           file === 'thunking_manual.cc' ||

test/objectwrap_function.cc

@@ -18,28 +18,26 @@ class FunctionTest : public Napi::ObjectWrap<FunctionTest> {
     return MaybeUnwrap(GetConstructor(info.Env()).New(args));
   }
 
-  // Constructor-per-env map in a static member because env.SetInstanceData()
-  // would interfere with Napi::Addon<T>
-  static std::unordered_map<napi_env, Napi::FunctionReference> constructors;
-
   static void Initialize(Napi::Env env, Napi::Object exports) {
     const char* name = "FunctionTest";
     Napi::Function func = DefineClass(env, name, {});
-    constructors[env] = Napi::Persistent(func);
-    env.AddCleanupHook([env] { constructors.erase(env); });
+    Napi::FunctionReference* ctor = new Napi::FunctionReference();
+    *ctor = Napi::Persistent(func);
+    env.SetInstanceData(ctor);
     exports.Set(name, func);
   }
 
   static Napi::Function GetConstructor(Napi::Env env) {
-    return constructors[env].Value();
+    return env.GetInstanceData<Napi::FunctionReference>()->Value();
   }
 };
 
-std::unordered_map<napi_env, Napi::FunctionReference>
-    FunctionTest::constructors;
+Napi::Value ObjectWrapFunctionFactory(const Napi::CallbackInfo& info) {
+  Napi::Object exports = Napi::Object::New(info.Env());
+  FunctionTest::Initialize(info.Env(), exports);
+  return exports;
+}
 
 Napi::Object InitObjectWrapFunction(Napi::Env env) {
-  Napi::Object exports = Napi::Object::New(env);
-  FunctionTest::Initialize(env, exports);
-  return exports;
+  return Napi::Function::New<ObjectWrapFunctionFactory>(env, "FunctionFactory");
 }

test/objectwrap_function.js

@@ -1,22 +1,6 @@
 'use strict';
 
-const assert = require('assert');
-const testUtil = require('./testUtil');
-
-function test (binding) {
-  return testUtil.runGCTests([
-    'objectwrap function',
-    () => {
-      const { FunctionTest } = binding.objectwrap_function;
-      const newConstructed = new FunctionTest();
-      const functionConstructed = FunctionTest();
-      assert(newConstructed instanceof FunctionTest);
-      assert(functionConstructed instanceof FunctionTest);
-      assert.throws(() => (FunctionTest(true)), /an exception/);
-    },
-    // Do on gc before returning.
-    () => {}
-  ]);
-}
-
-module.exports = require('./common').runTest(test);
+module.exports = require('./common').runTestInChildProcess({
+  suite: 'objectwrap_function',
+  testName: 'runTest'
+});