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: support forced exit #52038

Merged
merged 1 commit into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -1866,6 +1866,15 @@ added:
The maximum number of test files that the test runner CLI will execute
concurrently. The default value is `os.availableParallelism() - 1`.

### `--test-force-exit`

<!-- YAML
added: REPLACEME
-->

Configures the test runner to exit the process once all known tests have
finished executing even if the event loop would otherwise remain active.

### `--test-name-pattern`

<!-- YAML
Expand Down
6 changes: 6 additions & 0 deletions doc/api/test.md
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,9 @@ added:
- v18.9.0
- v16.19.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/52038
description: Added the `forceExit` option.
- version:
- v20.1.0
- v18.17.0
Expand All @@ -1137,6 +1140,9 @@ changes:
**Default:** `false`.
* `files`: {Array} An array containing the list of files to run.
**Default** matching files from [test runner execution model][].
* `forceExit`: {boolean} Configures the test runner to exit the process once
all known tests have finished executing even if the event loop would
otherwise remain active. **Default:** `false`.
* `inspectPort` {number|Function} Sets inspector port of test child process.
This can be a number, or a function that takes no arguments and returns a
number. If a nullish value is provided, each process gets its own port,
Expand Down
4 changes: 4 additions & 0 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,10 @@ Starts the Node.js command line test runner.
The maximum number of test files that the test runner CLI will execute
concurrently.
.
.It Fl -test-force-exit
Configures the test runner to exit the process once all known tests have
finished executing even if the event loop would otherwise remain active.
.
.It Fl -test-name-pattern
A regular expression that configures the test runner to only execute tests
whose name matches the provided pattern.
Expand Down
1 change: 1 addition & 0 deletions lib/internal/test_runner/harness.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ function setup(root) {
suites: 0,
},
shouldColorizeTestFiles: false,
teardown: exitHandler,
};
root.startTime = hrtime();
return root;
Expand Down
36 changes: 33 additions & 3 deletions lib/internal/test_runner/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,11 @@ 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, { forceExit, inspectPort, testNamePatterns, only }) {
const argv = ArrayPrototypeFilter(process.execArgv, filterExecArgv);
if (forceExit === true) {
ArrayPrototypePush(argv, '--test-force-exit');
}
if (isUsingInspector()) {
ArrayPrototypePush(argv, `--inspect-port=${getInspectPort(inspectPort)}`);
}
Expand Down Expand Up @@ -440,14 +443,33 @@ function run(options = kEmptyObject) {
validateObject(options, 'options');

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

if (files != null) {
validateArray(files, 'options.files');
}
if (watch != null) {
validateBoolean(watch, 'options.watch');
}
if (forceExit != null) {
validateBoolean(forceExit, 'options.forceExit');

if (forceExit && watch) {
throw new ERR_INVALID_ARG_VALUE(
'options.forceExit', watch, 'is not supported with watch mode',
);
}
}
if (only != null) {
validateBoolean(only, 'options.only');
}
Expand Down Expand Up @@ -501,7 +523,15 @@ 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,
forceExit,
};
if (watch) {
filesWatcher = watchFiles(testFiles, opts);
postRun = undefined;
Expand Down
28 changes: 21 additions & 7 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,12 @@ const kHookNames = ObjectSeal(['before', 'after', 'beforeEach', 'afterEach']);
const kUnwrapErrors = new SafeSet()
.add(kTestCodeFailure).add(kHookFailure)
.add('uncaughtException').add('unhandledRejection');
const { sourceMaps, testNamePatterns, testOnlyFlag } = parseCommandLine();
const {
forceExit,
sourceMaps,
testNamePatterns,
testOnlyFlag,
} = parseCommandLine();
let kResistStopPropagation;
let findSourceMap;

Expand Down Expand Up @@ -748,6 +753,16 @@ class Test extends AsyncResource {
// This helps catch any asynchronous activity that occurs after the tests
// have finished executing.
this.postRun();
} else if (forceExit) {
// This is the root test, and all known tests and hooks have finished
// executing. If the user wants to force exit the process regardless of
// any remaining ref'ed handles, then do that now. It is theoretically
// possible that a ref'ed handle could asynchronously create more tests,
// but the user opted into this behavior.
this.reporter.once('close', () => {
process.exit();
Copy link
Member

Choose a reason for hiding this comment

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

  1. what if the reporter is already closed, should we check if the reporter is already closed?
  2. I think we should check if there are active handles before exiting so maybe the force exit is not needed
  3. maybe print a warning that some handles are refed and it's best to clean them up

Copy link
Member

Choose a reason for hiding this comment

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

what if the reporter is already closed

in this case, there is no need for forced exit. the reporter closes on a call to root.postRun

});
this.harness.teardown();
}
}

Expand Down Expand Up @@ -798,12 +813,11 @@ class Test extends AsyncResource {
if (this.parent === this.root &&
this.root.activeSubtests === 0 &&
this.root.pendingSubtests.length === 0 &&
this.root.readySubtests.size === 0 &&
this.root.hooks.after.length > 0) {
// This is done so that any global after() hooks are run. At this point
// all of the tests have finished running. However, there might be
// ref'ed handles keeping the event loop alive. This gives the global
// after() hook a chance to clean them up.
this.root.readySubtests.size === 0) {
// At this point all of the tests have finished running. However, there
// might be ref'ed handles keeping the event loop alive. This gives the
// global after() hook a chance to clean them up. The user may also
// want to force the test runner to exit despite ref'ed handles.
this.root.run();
}
} else if (!this.reported) {
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/test_runner/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ function parseCommandLine() {

const isTestRunner = getOptionValue('--test');
const coverage = getOptionValue('--experimental-test-coverage');
const forceExit = getOptionValue('--test-force-exit');
const sourceMaps = getOptionValue('--enable-source-maps');
const isChildProcess = process.env.NODE_TEST_CONTEXT === 'child';
const isChildProcessV8 = process.env.NODE_TEST_CONTEXT === 'child-v8';
Expand Down Expand Up @@ -245,6 +246,7 @@ function parseCommandLine() {
__proto__: null,
isTestRunner,
coverage,
forceExit,
sourceMaps,
testOnlyFlag,
testNamePatterns,
Expand Down
6 changes: 6 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,9 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors,
} else if (force_repl) {
errors->push_back("either --watch or --interactive "
"can be used, not both");
} else if (test_runner_force_exit) {
errors->push_back("either --watch or --test-force-exit "
"can be used, not both");
Comment on lines +182 to +184
Copy link
Member

Choose a reason for hiding this comment

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

This is becoming a repetitive check (if AAA -> push_back("either --watch or AAA can be used, not both")),
Might be worth extracting to some function 🤔

} else if (!test_runner && (argv->size() < 1 || (*argv)[1].empty())) {
errors->push_back("--watch requires specifying a file");
}
Expand Down Expand Up @@ -616,6 +619,9 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
AddOption("--test-concurrency",
"specify test runner concurrency",
&EnvironmentOptions::test_runner_concurrency);
AddOption("--test-force-exit",
"force test runner to exit upon completion",
&EnvironmentOptions::test_runner_force_exit);
AddOption("--test-timeout",
"specify test runner timeout",
&EnvironmentOptions::test_runner_timeout);
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ class EnvironmentOptions : public Options {
uint64_t test_runner_concurrency = 0;
uint64_t test_runner_timeout = 0;
bool test_runner_coverage = false;
bool test_runner_force_exit = false;
std::vector<std::string> test_name_pattern;
std::vector<std::string> test_reporter;
std::vector<std::string> test_reporter_destination;
Expand Down
27 changes: 27 additions & 0 deletions test/fixtures/test-runner/output/force_exit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Flags: --test-force-exit --test-reporter=spec
'use strict';
const { after, afterEach, before, beforeEach, test } = require('node:test');

before(() => {
console.log('BEFORE');
});

beforeEach(() => {
console.log('BEFORE EACH');
});

after(() => {
console.log('AFTER');
});

afterEach(() => {
console.log('AFTER EACH');
});

test('passes but oops', () => {
setTimeout(() => {
throw new Error('this should not have a chance to be thrown');
}, 1000);
});

test('also passes');
16 changes: 16 additions & 0 deletions test/fixtures/test-runner/output/force_exit.snapshot
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
BEFORE
BEFORE EACH
AFTER EACH
BEFORE EACH
AFTER EACH
AFTER
✔ passes but oops (*ms)
✔ also passes (*ms)
ℹ tests 2
ℹ suites 0
ℹ pass 2
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms *
10 changes: 10 additions & 0 deletions test/fixtures/test-runner/throws_sync_and_async.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
'use strict';
const { test } = require('node:test');

test('fails and schedules more work', () => {
setTimeout(() => {
throw new Error('this should not have a chance to be thrown');
}, 1000);

throw new Error('fails');
});
15 changes: 15 additions & 0 deletions test/parallel/test-runner-force-exit-failure.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
'use strict';
require('../common');
const { match, doesNotMatch, strictEqual } = require('node:assert');
const { spawnSync } = require('node:child_process');
const fixtures = require('../common/fixtures');
const fixture = fixtures.path('test-runner/throws_sync_and_async.js');
const r = spawnSync(process.execPath, ['--test', '--test-force-exit', fixture]);

strictEqual(r.status, 1);
strictEqual(r.signal, null);
strictEqual(r.stderr.toString(), '');

const stdout = r.stdout.toString();
match(stdout, /error: 'fails'/);
doesNotMatch(stdout, /this should not have a chance to be thrown/);
1 change: 1 addition & 0 deletions test/parallel/test-runner-output.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ const tests = [
{ name: 'test-runner/output/global-hooks-with-no-tests.js' },
{ name: 'test-runner/output/before-and-after-each-too-many-listeners.js' },
{ name: 'test-runner/output/before-and-after-each-with-timeout-too-many-listeners.js' },
{ name: 'test-runner/output/force_exit.js', transform: specTransform },
cjihrig marked this conversation as resolved.
Show resolved Hide resolved
{ name: 'test-runner/output/global_after_should_fail_the_test.js' },
{ name: 'test-runner/output/no_refs.js' },
{ name: 'test-runner/output/no_tests.js' },
Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-runner-run.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -513,3 +513,21 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
for await (const _ of stream);
});
});

describe('forceExit', () => {
it('throws for non-boolean values', () => {
[Symbol(), {}, 0, 1, '1', Promise.resolve([])].forEach((forceExit) => {
assert.throws(() => run({ forceExit }), {
code: 'ERR_INVALID_ARG_TYPE',
message: /The "options\.forceExit" property must be of type boolean\./
});
});
});

it('throws if enabled with watch mode', () => {
assert.throws(() => run({ forceExit: true, watch: true }), {
code: 'ERR_INVALID_ARG_VALUE',
message: /The property 'options\.forceExit' is not supported with watch mode\./
});
});
});