Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runner: allow running test files in single process #51579

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -912,6 +912,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 @@ -2505,6 +2517,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `isolation` {string} If `'process'`, each test file is run in
* `isolation` {'process' | 'none'} 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
3 changes: 3 additions & 0 deletions lib/internal/main/test_runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ 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;

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
22 changes: 22 additions & 0 deletions lib/internal/main/tests_entry.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
'use strict';
const {
prepareMainThreadExecution,
markBootstrapComplete,
} = require('internal/process/pre_execution');
const { getOptionValue } = require('internal/options');
const { getCWDURL } = require('internal/util');
const { pathToFileURL } = require('internal/url');
const { initGlobalRoot } = require('internal/test_runner/harness');

prepareMainThreadExecution(false);
markBootstrapComplete();

const concurrency = getOptionValue('--test-concurrency') || true;
const parentURL = getCWDURL().href;
const files = process.env.NODE_TEST_FILES.split(',');

const { esmLoader } = require('internal/process/esm_loader');
initGlobalRoot({ __proto__: null, concurrency });
for (const path of files) {
esmLoader.import(pathToFileURL(path), parentURL, { __proto__: null });
}
11 changes: 6 additions & 5 deletions lib/internal/test_runner/harness.js
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,9 @@ function setup(root) {

let globalRoot;
let reportersSetup;
function getGlobalRoot() {
function initGlobalRoot(options = kEmptyObject) {
if (!globalRoot) {
globalRoot = createTestTree();
globalRoot = createTestTree(options);
globalRoot.reporter.on('test:fail', (data) => {
if (data.todo === undefined || data.todo === false) {
process.exitCode = kGenericUserError;
Expand All @@ -214,13 +214,13 @@ function getGlobalRoot() {

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

function runInParentContext(Factory) {
function run(name, options, fn, overrides) {
const parent = testResources.get(executionAsyncId()) || getGlobalRoot();
const parent = testResources.get(executionAsyncId()) || initGlobalRoot();
const subtest = parent.createSubtest(Factory, name, options, fn, overrides);
if (!(parent instanceof Suite)) {
return startSubtest(subtest);
Expand Down Expand Up @@ -252,7 +252,7 @@ function runInParentContext(Factory) {

function hook(hook) {
return (fn, options) => {
const parent = testResources.get(executionAsyncId()) || getGlobalRoot();
const parent = testResources.get(executionAsyncId()) || initGlobalRoot();
parent.createHook(hook, fn, {
__proto__: null,
...options,
Expand All @@ -265,6 +265,7 @@ function hook(hook) {

module.exports = {
createTestTree,
initGlobalRoot,
test: runInParentContext(Test),
describe: runInParentContext(Suite),
it: runInParentContext(Test),
Expand Down
69 changes: 49 additions & 20 deletions lib/internal/test_runner/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const {
validateBoolean,
validateFunction,
validateObject,
validateOneOf,
validateInteger,
} = require('internal/validators');
const { getInspectPort, isUsingInspector, isInspectorMessage } = require('internal/util/inspector');
Expand Down Expand Up @@ -80,8 +81,9 @@ const {
} = internalBinding('errors');

const kFilterArgs = ['--test', '--experimental-test-coverage', '--watch'];
const kFilterArgValues = ['--test-reporter', '--test-reporter-destination'];
const kFilterArgValues = ['--test-reporter', '--test-reporter-destination', '--experimental-test-isolation'];
const kDiagnosticsFilterArgs = ['tests', 'suites', 'pass', 'fail', 'cancelled', 'skipped', 'todo', 'duration_ms'];
const kIsolatedProcessName = '<root>';

const kCanceledTests = new SafeSet()
.add(kCancelledByParent).add(kAborted).add(kTestTimeoutFailure);
Expand Down Expand Up @@ -112,7 +114,7 @@ function filterExecArgv(arg, i, arr) {
!ArrayPrototypeSome(kFilterArgValues, (p) => arg === p || (i > 0 && arr[i - 1] === p) || StringPrototypeStartsWith(arg, `${p}=`));
}

function getRunArgs(path, { inspectPort, testNamePatterns, only }) {
function getRunArgs(path, { inspectPort, testNamePatterns, only, isolation }) {
const argv = ArrayPrototypeFilter(process.execArgv, filterExecArgv);
if (isUsingInspector()) {
ArrayPrototypePush(argv, `--inspect-port=${getInspectPort(inspectPort)}`);
Expand All @@ -123,7 +125,9 @@ function getRunArgs(path, { inspectPort, testNamePatterns, only }) {
if (only === true) {
ArrayPrototypePush(argv, '--test-only');
}
ArrayPrototypePush(argv, path);
if (isolation !== 'none') {
ArrayPrototypePush(argv, path);
}

return argv;
}
Expand All @@ -144,11 +148,12 @@ class FileTest extends Test {

constructor(options) {
super(options);
const file = this.name === kIsolatedProcessName ? resolve('') : resolve(this.name);
this.loc ??= {
__proto__: null,
line: 1,
column: 1,
file: resolve(this.name),
file,
};
}

Expand Down Expand Up @@ -330,6 +335,10 @@ function runTestFile(path, filesWatcher, opts) {
if (opts.root.harness.shouldColorizeTestFiles) {
env.FORCE_COLOR = '1';
}
if (opts.isolation === 'none') {
args.push('--experimental-test-isolation=none');
env.NODE_TEST_FILES = ArrayPrototypeJoin(opts.testFiles, ',');
}

const child = spawn(process.execPath, args, { __proto__: null, signal: t.signal, encoding: 'utf8', env, stdio });
if (watchMode) {
Expand Down Expand Up @@ -397,29 +406,40 @@ function runTestFile(path, filesWatcher, opts) {
return subtest.start();
}

function watchFiles(testFiles, opts) {
function watchFiles(opts) {
const runningProcesses = new SafeMap();
const runningSubtests = new SafeMap();
const watcher = new FilesWatcher({ __proto__: null, debounce: 200, mode: 'filter', signal: opts.signal });
const filesWatcher = { __proto__: null, watcher, runningProcesses, runningSubtests };

async function stopRunning(file) {
const runningProcess = runningProcesses.get(file);
if (runningProcess) {
runningProcess.kill();
await once(runningProcess, 'exit');
}
if (!runningSubtests.size) {
// Reset the topLevel counter
opts.root.harness.counters.topLevel = 0;
}
await runningSubtests.get(file);
runningSubtests.set(file, runTestFile(file, filesWatcher, opts));
}

watcher.on('changed', ({ owners }) => {
if (opts.isolation === 'none') {
PromisePrototypeThen(stopRunning(kIsolatedProcessName), undefined, (error) => {
triggerUncaughtException(error, true /* fromPromise */);
});
return;
}
watcher.unfilterFilesOwnedBy(owners);
PromisePrototypeThen(SafePromiseAllReturnVoid(testFiles, async (file) => {
PromisePrototypeThen(SafePromiseAllReturnVoid(opts.testFiles, async (file) => {
console.log(owners, opts.testFiles, opts.isolation);
if (!owners.has(file)) {
return;
}
const runningProcess = runningProcesses.get(file);
if (runningProcess) {
runningProcess.kill();
await once(runningProcess, 'exit');
}
if (!runningSubtests.size) {
// Reset the topLevel counter
opts.root.harness.counters.topLevel = 0;
}
await runningSubtests.get(file);
runningSubtests.set(file, runTestFile(file, filesWatcher, opts));
await stopRunning(file);
}, undefined, (error) => {
triggerUncaughtException(error, true /* fromPromise */);
}));
Expand All @@ -439,7 +459,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 +490,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,13 +525,18 @@ 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, testFiles };
if (watch) {
filesWatcher = watchFiles(testFiles, opts);
filesWatcher = watchFiles(opts);
postRun = undefined;
}
const runFiles = () => {
root.harness.bootstrapComplete = true;
if (isolation === 'none') {
const subtest = runTestFile(kIsolatedProcessName, filesWatcher, opts);
filesWatcher?.runningSubtests.set(kIsolatedProcessName, subtest);
return subtest;
}
return SafePromiseAllSettledReturnVoid(testFiles, (path) => {
const subtest = runTestFile(path, filesWatcher, opts);
filesWatcher?.runningSubtests.set(path, subtest);
Expand Down
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
5 changes: 5 additions & 0 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,11 @@ MaybeLocal<Value> StartExecution(Environment* env, StartExecutionCallback cb) {
return StartExecution(env, "internal/main/check_syntax");
}

if (env->options()->test_isolation == "none" &&
env->env_vars()->Get("NODE_TEST_FILES").IsJust()) {
return StartExecution(env, "internal/main/tests_entry");
}

if (env->options()->test_runner) {
return StartExecution(env, "internal/main/test_runner");
}
Expand Down
4 changes: 4 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,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;
});