Skip to content

Commit 414be9e

Browse files
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>
1 parent afa494e commit 414be9e

File tree

11 files changed

+157
-84
lines changed

11 files changed

+157
-84
lines changed

test/addon.cc

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ class TestAddon : public Napi::Addon<TestAddon> {
1717
{InstanceMethod("decrement", &TestAddon::Decrement)}))});
1818
}
1919

20+
~TestAddon() { fprintf(stderr, "TestAddon::~TestAddon\n"); }
21+
2022
private:
2123
Napi::Value Increment(const Napi::CallbackInfo& info) {
2224
return Napi::Number::New(info.Env(), ++value);
@@ -29,10 +31,14 @@ class TestAddon : public Napi::Addon<TestAddon> {
2931
uint32_t value = 42;
3032
};
3133

34+
Napi::Value CreateAddon(const Napi::CallbackInfo& info) {
35+
return TestAddon::Init(info.Env(), Napi::Object::New(info.Env()));
36+
}
37+
3238
} // end of anonymous namespace
3339

3440
Napi::Object InitAddon(Napi::Env env) {
35-
return TestAddon::Init(env, Napi::Object::New(env));
41+
return Napi::Function::New<CreateAddon>(env, "CreateAddon");
3642
}
3743

3844
#endif // (NAPI_VERSION > 5)

test/addon.js

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,7 @@
11
'use strict';
22

3-
const assert = require('assert');
4-
5-
module.exports = require('./common').runTest(test);
6-
7-
function test (binding) {
8-
assert.strictEqual(binding.addon.increment(), 43);
9-
assert.strictEqual(binding.addon.increment(), 44);
10-
assert.strictEqual(binding.addon.subObject.decrement(), 43);
11-
}
3+
module.exports = require('./common').runTestInChildProcess({
4+
suite: 'addon',
5+
testName: 'workingCode',
6+
expectedStderr: ['TestAddon::~TestAddon']
7+
});

test/addon_data.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@
44
#include "test_helper.h"
55

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

test/addon_data.js

Lines changed: 16 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,46 +1,24 @@
11
'use strict';
22

3-
const assert = require('assert');
4-
const { spawn } = require('child_process');
5-
const readline = require('readline');
3+
const common = require('./common');
64

7-
module.exports = require('./common').runTestWithBindingPath(test);
5+
module.exports = common.runTest(test);
86

9-
// Make sure the instance data finalizer is called at process exit. If the hint
10-
// is non-zero, it will be printed out by the child process.
11-
function testFinalizer (bindingName, hint, expected) {
12-
return new Promise((resolve) => {
13-
bindingName = bindingName.split('\\').join('\\\\');
14-
const child = spawn(process.execPath, [
15-
'-e',
16-
`require('${bindingName}').addon_data(${hint}).verbose = true;`
17-
]);
18-
const actual = [];
19-
readline
20-
.createInterface({ input: child.stderr })
21-
.on('line', (line) => {
22-
if (expected.indexOf(line) >= 0) {
23-
actual.push(line);
24-
}
25-
})
26-
.on('close', () => {
27-
assert.deepStrictEqual(expected, actual);
28-
resolve();
29-
});
7+
async function test () {
8+
await common.runTestInChildProcess({
9+
suite: 'addon_data',
10+
testName: 'workingCode'
3011
});
31-
}
32-
33-
async function test (bindingName) {
34-
const binding = require(bindingName).addon_data(0);
3512

36-
// Make sure it is possible to get/set instance data.
37-
assert.strictEqual(binding.verbose.verbose, false);
38-
binding.verbose = true;
39-
assert.strictEqual(binding.verbose.verbose, true);
40-
binding.verbose = false;
41-
assert.strictEqual(binding.verbose.verbose, false);
13+
await common.runTestInChildProcess({
14+
suite: 'addon_data',
15+
testName: 'cleanupWithoutHint',
16+
expectedStderr: ['addon_data: Addon::~Addon']
17+
});
4218

43-
await testFinalizer(bindingName, 0, ['addon_data: Addon::~Addon']);
44-
await testFinalizer(bindingName, 42,
45-
['addon_data: Addon::~Addon', 'hint: 42']);
19+
await common.runTestInChildProcess({
20+
suite: 'addon_data',
21+
testName: 'cleanupWithHint',
22+
expectedStderr: ['addon_data: Addon::~Addon', 'hint: 42']
23+
});
4624
}

test/child_processes/addon.js

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
'use strict';
2+
const assert = require('assert');
3+
4+
module.exports = {
5+
workingCode: binding => {
6+
const addon = binding.addon();
7+
assert.strictEqual(addon.increment(), 43);
8+
assert.strictEqual(addon.increment(), 44);
9+
assert.strictEqual(addon.subObject.decrement(), 43);
10+
}
11+
};

test/child_processes/addon_data.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
'use strict';
2+
3+
const assert = require('assert');
4+
5+
// Make sure the instance data finalizer is called at process exit. If the hint
6+
// is non-zero, it will be printed out by the child process.
7+
const cleanupTest = (binding, hint) => {
8+
binding.addon_data(hint).verbose = true;
9+
};
10+
11+
module.exports = {
12+
workingCode: binding => {
13+
const addonData = binding.addon_data(0);
14+
15+
// Make sure it is possible to get/set instance data.
16+
assert.strictEqual(addonData.verbose.verbose, false);
17+
addonData.verbose = true;
18+
assert.strictEqual(addonData.verbose.verbose, true);
19+
addonData.verbose = false;
20+
assert.strictEqual(addonData.verbose.verbose, false);
21+
},
22+
cleanupWithHint: binding => cleanupTest(binding, 42),
23+
cleanupWithoutHint: binding => cleanupTest(binding, 0)
24+
};
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
'use strict';
2+
3+
const assert = require('assert');
4+
const testUtil = require('../testUtil');
5+
6+
module.exports = {
7+
runTest: function (binding) {
8+
return testUtil.runGCTests([
9+
'objectwrap function',
10+
() => {
11+
const { FunctionTest } = binding.objectwrap_function();
12+
const newConstructed = new FunctionTest();
13+
const functionConstructed = FunctionTest();
14+
assert(newConstructed instanceof FunctionTest);
15+
assert(functionConstructed instanceof FunctionTest);
16+
assert.throws(() => (FunctionTest(true)), /an exception/);
17+
},
18+
// Do one gc before returning.
19+
() => {}
20+
]);
21+
}
22+
};

test/common/index.js

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,11 @@
33
const assert = require('assert');
44
const path = require('path');
55
const { access } = require('node:fs/promises');
6+
const { spawn } = require('child_process');
7+
const { EOL } = require('os');
8+
const readline = require('readline');
9+
10+
const escapeBackslashes = (pathString) => pathString.split('\\').join('\\\\');
611

712
const noop = () => {};
813

@@ -26,7 +31,7 @@ function runCallChecks (exitCode) {
2631
context.name,
2732
context.messageSegment,
2833
context.actual);
29-
console.log(context.stack.split('\n').slice(2).join('\n'));
34+
console.log(context.stack.split(EOL).slice(2).join(EOL));
3035
});
3136

3237
if (failed.length) process.exit(1);
@@ -190,3 +195,51 @@ exports.runTestWithBuildType = async function (test, buildType) {
190195
await Promise.resolve(test(buildType))
191196
.finally(exports.mustCall());
192197
};
198+
199+
// Some tests have to run in their own process, otherwise they would interfere
200+
// with each other. Such tests export a factory function rather than the test
201+
// itself so as to avoid automatic instantiation, and therefore interference,
202+
// in the main process. Two examples are addon and addon_data, both of which
203+
// use Napi::Env::SetInstanceData(). This helper function provides a common
204+
// approach for running such tests.
205+
exports.runTestInChildProcess = function ({ suite, testName, expectedStderr }) {
206+
return exports.runTestWithBindingPath((bindingName) => {
207+
return new Promise((resolve) => {
208+
bindingName = escapeBackslashes(bindingName);
209+
// Test suites are assumed to be located here.
210+
const suitePath = escapeBackslashes(path.join(__dirname, '..', 'child_processes', suite));
211+
const child = spawn(process.execPath, [
212+
'--expose-gc',
213+
'-e',
214+
`require('${suitePath}').${testName}(require('${bindingName}'))`
215+
]);
216+
const resultOfProcess = { stderr: [] };
217+
218+
// Capture the exit code and signal.
219+
child.on('close', (code, signal) => resolve(Object.assign(resultOfProcess, { code, signal })));
220+
221+
// Capture the stderr as an array of lines.
222+
readline
223+
.createInterface({ input: child.stderr })
224+
.on('line', (line) => {
225+
resultOfProcess.stderr.push(line);
226+
});
227+
}).then(actual => {
228+
// Back up the stderr in case the assertion fails.
229+
const fullStderr = actual.stderr.map(item => `from child process: ${item}`);
230+
const expected = { stderr: expectedStderr, code: 0, signal: null };
231+
232+
if (!expectedStderr) {
233+
// If we don't care about stderr, delete it.
234+
delete actual.stderr;
235+
delete expected.stderr;
236+
} else {
237+
// Otherwise we only care about expected lines in the actual stderr, so
238+
// filter out everything else.
239+
actual.stderr = actual.stderr.filter(line => expectedStderr.includes(line));
240+
}
241+
242+
assert.deepStrictEqual(actual, expected, `Assertion for child process test ${suite}.${testName} failed:${EOL}` + fullStderr.join(EOL));
243+
});
244+
});
245+
};

test/index.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ function loadTestModules (currentDirectory = __dirname, pre = '') {
6060
file === 'binding.gyp' ||
6161
file === 'build' ||
6262
file === 'common' ||
63+
file === 'child_processes' ||
6364
file === 'napi_child.js' ||
6465
file === 'testUtil.js' ||
6566
file === 'thunking_manual.cc' ||

test/objectwrap_function.cc

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,28 +18,26 @@ class FunctionTest : public Napi::ObjectWrap<FunctionTest> {
1818
return MaybeUnwrap(GetConstructor(info.Env()).New(args));
1919
}
2020

21-
// Constructor-per-env map in a static member because env.SetInstanceData()
22-
// would interfere with Napi::Addon<T>
23-
static std::unordered_map<napi_env, Napi::FunctionReference> constructors;
24-
2521
static void Initialize(Napi::Env env, Napi::Object exports) {
2622
const char* name = "FunctionTest";
2723
Napi::Function func = DefineClass(env, name, {});
28-
constructors[env] = Napi::Persistent(func);
29-
env.AddCleanupHook([env] { constructors.erase(env); });
24+
Napi::FunctionReference* ctor = new Napi::FunctionReference();
25+
*ctor = Napi::Persistent(func);
26+
env.SetInstanceData(ctor);
3027
exports.Set(name, func);
3128
}
3229

3330
static Napi::Function GetConstructor(Napi::Env env) {
34-
return constructors[env].Value();
31+
return env.GetInstanceData<Napi::FunctionReference>()->Value();
3532
}
3633
};
3734

38-
std::unordered_map<napi_env, Napi::FunctionReference>
39-
FunctionTest::constructors;
35+
Napi::Value ObjectWrapFunctionFactory(const Napi::CallbackInfo& info) {
36+
Napi::Object exports = Napi::Object::New(info.Env());
37+
FunctionTest::Initialize(info.Env(), exports);
38+
return exports;
39+
}
4040

4141
Napi::Object InitObjectWrapFunction(Napi::Env env) {
42-
Napi::Object exports = Napi::Object::New(env);
43-
FunctionTest::Initialize(env, exports);
44-
return exports;
42+
return Napi::Function::New<ObjectWrapFunctionFactory>(env, "FunctionFactory");
4543
}

test/objectwrap_function.js

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,6 @@
11
'use strict';
22

3-
const assert = require('assert');
4-
const testUtil = require('./testUtil');
5-
6-
function test (binding) {
7-
return testUtil.runGCTests([
8-
'objectwrap function',
9-
() => {
10-
const { FunctionTest } = binding.objectwrap_function;
11-
const newConstructed = new FunctionTest();
12-
const functionConstructed = FunctionTest();
13-
assert(newConstructed instanceof FunctionTest);
14-
assert(functionConstructed instanceof FunctionTest);
15-
assert.throws(() => (FunctionTest(true)), /an exception/);
16-
},
17-
// Do on gc before returning.
18-
() => {}
19-
]);
20-
}
21-
22-
module.exports = require('./common').runTest(test);
3+
module.exports = require('./common').runTestInChildProcess({
4+
suite: 'objectwrap_function',
5+
testName: 'runTest'
6+
});

0 commit comments

Comments
 (0)