Skip to content

Commit

Permalink
test_runner: allow running test files in single process
Browse files Browse the repository at this point in the history
  • Loading branch information
MoLow committed Jan 29, 2024
1 parent f3fcad2 commit 244aa35
Show file tree
Hide file tree
Showing 16 changed files with 139 additions and 27 deletions.
13 changes: 13 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,18 @@ generated as part of the test runner output. If no tests are run, a coverage
report is not generated. See the documentation on
[collecting code coverage from tests][] for more details.

### `--experimental-test-isolation`

<!-- YAML
added:
- REPLACEME
-->

When running tests with the `node:test` module,
each test file is run in its own process.
running `--experimental-test-isolation=none` will run all
files in the same process.

### `--experimental-vm-modules`

<!-- YAML
Expand Down Expand Up @@ -2489,6 +2501,7 @@ Node.js options that are allowed are:
* `--experimental-policy`
* `--experimental-shadow-realm`
* `--experimental-specifier-resolution`
* `--experimental-test-isolation`
* `--experimental-top-level-await`
* `--experimental-vm-modules`
* `--experimental-wasi-unstable-preview1`
Expand Down
3 changes: 3 additions & 0 deletions doc/api/test.md
Original file line number Diff line number Diff line change
Expand Up @@ -1125,6 +1125,9 @@ changes:
number. If a nullish value is provided, each process gets its own port,
incremented from the primary's `process.debugPort`.
**Default:** `undefined`.
* `isolation` {string} If `'process'`, each test file is run in
its own process. If `'none'`, all test files run in the same proccess.
**Default:** `'process'`.
* `only`: {boolean} If truthy, the test context will only run tests that
have the `only` option set
* `setup` {Function} A function that accepts the `TestsStream` instance
Expand Down
5 changes: 4 additions & 1 deletion lib/internal/main/test_runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@ let debug = require('internal/util/debuglog').debuglog('test_runner', (fn) => {
prepareMainThreadExecution(false);
markBootstrapComplete();


const isolation = getOptionValue('--experimental-test-isolation');
let concurrency = getOptionValue('--test-concurrency') || true;
let inspectPort;

if (isUsingInspector()) {
if (isUsingInspector() && isolation !== 'none') {
process.emitWarning('Using the inspector with --test forces running at a concurrency of 1. ' +
'Use the inspectPort option to run with concurrency');
concurrency = 1;
Expand Down Expand Up @@ -65,6 +67,7 @@ const timeout = getOptionValue('--test-timeout') || Infinity;
const options = {
concurrency,
inspectPort,
isolation,
watch: getOptionValue('--watch'),
setup: setupTestReporters,
timeout,
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/test_runner/harness.js
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ function getGlobalRoot() {

async function startSubtest(subtest) {
await reportersSetup;
getGlobalRoot().harness.bootstrapComplete = true;
subtest.root.harness.bootstrapComplete = true;
await subtest.start();
}

Expand Down
54 changes: 48 additions & 6 deletions lib/internal/test_runner/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ const {
const { spawn } = require('child_process');
const { finished } = require('internal/streams/end-of-stream');
const { resolve } = require('path');
const { pathToFileURL } = require('internal/url');
const { DefaultDeserializer, DefaultSerializer } = require('v8');
// TODO(aduh95): switch to internal/readline/interface when backporting to Node.js 16.x is no longer a concern.
const { createInterface } = require('readline');
Expand All @@ -50,11 +51,12 @@ const {
validateBoolean,
validateFunction,
validateObject,
validateOneOf,
validateInteger,
} = require('internal/validators');
const { getInspectPort, isUsingInspector, isInspectorMessage } = require('internal/util/inspector');
const { isRegExp } = require('internal/util/types');
const { kEmptyObject } = require('internal/util');
const { kEmptyObject, getCWDURL } = require('internal/util');
const { kEmitMessage } = require('internal/test_runner/tests_stream');
const { createTestTree } = require('internal/test_runner/harness');
const {
Expand All @@ -64,6 +66,7 @@ const {
kTestCodeFailure,
kTestTimeoutFailure,
Test,
Suite,
} = require('internal/test_runner/test');

const {
Expand Down Expand Up @@ -134,7 +137,34 @@ const v8Header = serializer.releaseBuffer();
const kV8HeaderLength = TypedArrayPrototypeGetLength(v8Header);
const kSerializedSizeHeader = 4 + kV8HeaderLength;

class FileTest extends Test {

class InProcessFileTest extends Suite {
constructor(options) {
super(options);
this.loc ??= {
__proto__: null,
line: 1,
column: 1,
file: resolve(this.name),
};
this.nesting = -1;
this.reportedType = 'test';
}

#reported = false;
reportStarted() {}
report() {
const skipReporting = this.subtests.length > 0;
if (!skipReporting && !this.#reported) {
this.nesting = 0;
this.#reported = true;
super.reportStarted();
super.report();
}
}
}

class SpawnFileTest extends Test {
// This class maintains two buffers:
#reportBuffer = []; // Parsed items waiting for this.isClearToSend()
#rawBuffer = []; // Raw data waiting to be parsed
Expand Down Expand Up @@ -319,7 +349,15 @@ class FileTest extends Test {

function runTestFile(path, filesWatcher, opts) {
const watchMode = filesWatcher != null;
const subtest = opts.root.createSubtest(FileTest, path, { __proto__: null, signal: opts.signal }, async (t) => {
const Factory = opts.isolation === 'none' ? InProcessFileTest : SpawnFileTest;
const subtest = opts.root.createSubtest(Factory, path, { __proto__: null, signal: opts.signal }, async (t) => {
if (opts.isolation === 'none') {
const parentURL = getCWDURL().href;
const { esmLoader } = require('internal/process/esm_loader');

await esmLoader.import(pathToFileURL(path), parentURL, { __proto__: null });
return;
}
const args = getRunArgs(path, opts);
const stdio = ['pipe', 'pipe', 'pipe'];
const env = { __proto__: null, ...process.env, NODE_TEST_CONTEXT: 'child-v8' };
Expand Down Expand Up @@ -439,7 +477,7 @@ function watchFiles(testFiles, opts) {
function run(options = kEmptyObject) {
validateObject(options, 'options');

let { testNamePatterns, shard } = options;
let { testNamePatterns, shard, isolation } = options;
const { concurrency, timeout, signal, files, inspectPort, watch, setup, only } = options;

if (files != null) {
Expand Down Expand Up @@ -470,6 +508,10 @@ function run(options = kEmptyObject) {
if (setup != null) {
validateFunction(setup, 'options.setup');
}
isolation ||= 'process';
if (isolation != null) {
validateOneOf(isolation, 'options.isolation', ['process', 'none']);
}
if (testNamePatterns != null) {
if (!ArrayIsArray(testNamePatterns)) {
testNamePatterns = [testNamePatterns];
Expand Down Expand Up @@ -501,7 +543,7 @@ function run(options = kEmptyObject) {

let postRun = () => root.postRun();
let filesWatcher;
const opts = { __proto__: null, root, signal, inspectPort, testNamePatterns, only };
const opts = { __proto__: null, root, signal, inspectPort, testNamePatterns, only, isolation };
if (watch) {
filesWatcher = watchFiles(testFiles, opts);
postRun = undefined;
Expand All @@ -521,6 +563,6 @@ function run(options = kEmptyObject) {
}

module.exports = {
FileTest, // Exported for tests only
SpawnFileTest, // Exported for tests only
run,
};
9 changes: 5 additions & 4 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ const kHookFailure = 'hookFailed';
const kDefaultTimeout = null;
const noop = FunctionPrototype;
const kShouldAbort = Symbol('kShouldAbort');
const kFilename = process.argv?.[1];
const kHookNames = ObjectSeal(['before', 'after', 'beforeEach', 'afterEach']);
const kUnwrapErrors = new SafeSet()
.add(kTestCodeFailure).add(kHookFailure)
Expand Down Expand Up @@ -349,7 +348,7 @@ class Test extends AsyncResource {
this.diagnostic(warning);
}

if (loc === undefined || kFilename === undefined) {
if (loc === undefined) {
this.loc = undefined;
} else {
this.loc = {
Expand Down Expand Up @@ -826,11 +825,13 @@ class Test extends AsyncResource {
details.type = this.reportedType;
}

const isTopLevel = this.nesting === 0;
const testNumber = isTopLevel ? this.root.harness.counters.topLevel : this.testNumber;
if (this.passed) {
this.reporter.ok(this.nesting, this.loc, this.testNumber, this.name, details, directive);
this.reporter.ok(this.nesting, this.loc, testNumber, this.name, details, directive);
} else {
details.error = this.error;
this.reporter.fail(this.nesting, this.loc, this.testNumber, this.name, details, directive);
this.reporter.fail(this.nesting, this.loc, testNumber, this.name, details, directive);
}

for (let i = 0; i < this.diagnostics.length; i++) {
Expand Down
12 changes: 2 additions & 10 deletions lib/internal/test_runner/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@ const {
const { AsyncResource } = require('async_hooks');
const { relative } = require('path');
const { createWriteStream } = require('fs');
const { pathToFileURL } = require('internal/url');
const { createDeferredPromise } = require('internal/util');
const { createDeferredPromise, getCWDURL } = require('internal/util');
const { getOptionValue } = require('internal/options');
const { green, yellow, red, white, shouldColorize } = require('internal/util/colors');

Expand Down Expand Up @@ -146,14 +145,7 @@ async function getReportersMap(reporters, destinations) {
let reporter = tryBuiltinReporter(name);

if (reporter === undefined) {
let parentURL;

try {
parentURL = pathToFileURL(process.cwd() + '/').href;
} catch {
parentURL = 'file:///';
}

const parentURL = getCWDURL().href;
const { esmLoader } = require('internal/process/esm_loader');
reporter = await esmLoader.import(name, parentURL, { __proto__: null });
}
Expand Down
3 changes: 2 additions & 1 deletion src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,8 @@ inline bool Environment::owns_inspector() const {

inline bool Environment::should_create_inspector() const {
return (flags_ & EnvironmentFlags::kNoCreateInspector) == 0 &&
!options_->test_runner && !options_->watch_mode;
(!options_->test_runner || options_->test_isolation == "none") &&
!options_->watch_mode;
}

inline bool Environment::tracks_unmanaged_fds() const {
Expand Down
11 changes: 8 additions & 3 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,10 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors,
"--watch-path cannot be used in combination with --test");
}

#ifndef ALLOW_ATTACHING_DEBUGGER_IN_TEST_RUNNER
debug_options_.allow_attaching_debugger = false;
#endif
if (watch_mode && test_isolation == "none") {
errors->push_back(
"--watch cannot be used with --experimental-test-isolation=none");
}
}

if (watch_mode) {
Expand Down Expand Up @@ -641,6 +642,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
"run test at specific shard",
&EnvironmentOptions::test_shard,
kAllowedInEnvvar);
AddOption("--experimental-test-isolation",
"isolation mode of test runner",
&EnvironmentOptions::test_isolation,
kAllowedInEnvvar);
AddOption("--test-udp-no-try-send", "", // For testing only.
&EnvironmentOptions::test_udp_no_try_send);
AddOption("--throw-deprecation",
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ class EnvironmentOptions : public Options {
bool test_only = false;
bool test_udp_no_try_send = false;
std::string test_shard;
std::string test_isolation;
bool throw_deprecation = false;
bool trace_atomics_wait = false;
bool trace_deprecation = false;
Expand Down
4 changes: 4 additions & 0 deletions test/fixtures/test-runner/no-isolation/a.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
'use strict';
const test = require('node:test');

test('a', (t, cb) => { globalThis.aCb = cb; });
7 changes: 7 additions & 0 deletions test/fixtures/test-runner/no-isolation/b.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';
const test = require('node:test');

test('b', async () => {
globalThis.aCb();
delete globalThis.aCb;
});
14 changes: 14 additions & 0 deletions test/parallel/test-runner-cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,20 @@ const testFixtures = fixtures.path('test-runner');
assert.match(stdout, /ok 6 - this should be executed/);
}


{
// Test test files with --experimental-test-isolation=none
const args = ['--test', '--experimental-test-isolation=none'];
const child = spawnSync(process.execPath, args, { cwd: join(testFixtures, 'no-isolation') });

assert.strictEqual(child.status, 0);
assert.strictEqual(child.signal, null);
assert.strictEqual(child.stderr.toString(), '');
const stdout = child.stdout.toString();
assert.match(stdout, /ok 1 - a/);
assert.match(stdout, /ok 2 - b/);
}

{
// Same but with a prototype mutation in require scripts.
const args = ['--require', join(testFixtures, 'protoMutation.js'), '--test'];
Expand Down
12 changes: 12 additions & 0 deletions test/parallel/test-runner-concurrency.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,15 @@ test('--test multiple files', { skip: os.availableParallelism() < 3 }, async ()
assert.strictEqual(stderr, '');
assert.strictEqual(code, 0);
});

test('--test multiple files with --test-isolation=none', { skip: os.availableParallelism() < 3 }, async () => {
await fs.writeFile(tmpdir.resolve('test-runner-concurrency'), '');
const { code, stderr } = await common.spawnPromisified(process.execPath, [
'--test',
'--experimental-test-isolation=none',
fixtures.path('test-runner', 'concurrency', 'a.mjs'),
fixtures.path('test-runner', 'concurrency', 'b.mjs'),
]);
assert.strictEqual(stderr, '');
assert.strictEqual(code, 0);
});
14 changes: 14 additions & 0 deletions test/parallel/test-runner-run.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,20 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
});
});

it('should run multiple files in same process', async () => {
const stream = run({
files: [
join(testFixtures, 'no-isolation/a.test.js'),
join(testFixtures, 'no-isolation/b.test.js')],
concurrency: 2,
isolation: 'none'
});
stream.on('test:fail', common.mustNotCall());
stream.on('test:pass', common.mustCall(2));
// eslint-disable-next-line no-unused-vars
for await (const _ of stream);
});

describe('sharding', () => {
const shardsTestsFixtures = fixtures.path('test-runner', 'shards');
const shardsTestsFiles = [
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-runner-v8-deserializer.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe('v8 deserializer', () => {
let reported;
beforeEach(() => {
reported = [];
fileTest = new runner.FileTest({ name: 'filetest' });
fileTest = new runner.SpawnFileTest({ name: 'filetest' });
fileTest.reporter.on('data', (data) => reported.push(data));
assert(fileTest.isClearToSend());
});
Expand Down

0 comments on commit 244aa35

Please sign in to comment.