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: add bail out #56490

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
12 changes: 12 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -2254,6 +2254,17 @@ Starts the Node.js command line test runner. This flag cannot be combined with
See the documentation on [running tests from the command line][]
for more details.

### `--test-bail`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

Instructs the test runner to bail out if a test failure occurs.
See the documentation on [test bailout][] for more details.

### `--test-concurrency`

<!-- YAML
Expand Down Expand Up @@ -3714,6 +3725,7 @@ node --stack-trace-limit=12 -p -e "Error.stackTraceLimit" # prints 12
[single executable application]: single-executable-applications.md
[snapshot testing]: test.md#snapshot-testing
[syntax detection]: packages.md#syntax-detection
[test bailout]: test.md#bailing-out
[test reporters]: test.md#test-reporters
[test runner execution model]: test.md#test-runner-execution-model
[timezone IDs]: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
Expand Down
51 changes: 51 additions & 0 deletions doc/api/test.md
Original file line number Diff line number Diff line change
Expand Up @@ -990,6 +990,23 @@ exports[`suite of snapshot tests > snapshot test 2`] = `
Once the snapshot file is created, run the tests again without the
`--test-update-snapshots` flag. The tests should pass now.

## Bailing out

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

> Stability: 1 - Experimental

The `--test-bail` flag provides a way to stop the test execution
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `--test-bail` flag provides a way to stop the test execution
The `--test-bail` flag provides a way to stop test execution

as soon as a test fails.
By enabling this flag, the test runner will exit the test suite early
when it encounters the first failing test, preventing
the execution of subsequent tests.
Already running tests will be canceled, and no further tests will be started.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we need to get into this small of a detail, but technically there will be a window of time where new tests may be started before being cancelled. Maybe we can say something like "The test runner will cancel all remaining tests."

**Default:** `false`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This default seems out of place.


## Test reporters

<!-- YAML
Expand Down Expand Up @@ -1077,6 +1094,9 @@ const customReporter = new Transform({
case 'test:fail':
callback(null, `test ${event.data.name} failed`);
break;
case 'test:bail':
callback(null, `test ${event.data.name} bailed out`);
break;
case 'test:plan':
callback(null, 'test plan');
break;
Expand Down Expand Up @@ -1122,6 +1142,9 @@ const customReporter = new Transform({
case 'test:fail':
callback(null, `test ${event.data.name} failed`);
break;
case 'test:bail':
callback(null, `test ${event.data.name} bailed out`);
break;
case 'test:plan':
callback(null, 'test plan');
break;
Expand Down Expand Up @@ -1166,6 +1189,9 @@ export default async function * customReporter(source) {
case 'test:fail':
yield `test ${event.data.name} failed\n`;
break;
case 'test:bail':
yield `test ${event.data.name} bailed out\n`;
break;
case 'test:plan':
yield 'test plan\n';
break;
Expand Down Expand Up @@ -1206,6 +1232,9 @@ module.exports = async function * customReporter(source) {
case 'test:fail':
yield `test ${event.data.name} failed\n`;
break;
case 'test:bail':
yield `test ${event.data.name} bailed out\n`;
break;
case 'test:plan':
yield 'test plan\n';
break;
Expand Down Expand Up @@ -1483,6 +1512,11 @@ changes:
does not have a name.
* `options` {Object} Configuration options for the test. The following
properties are supported:
* `bail` {boolean}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be an option to test(). Should it be part of run() instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, my bad, the bail option it's part of run. I'll update the doc ASAP !

If `true`, it will exit the test suite early
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If `true`, it will exit the test suite early
If `true`, the test runner will exit

when it encounters the first failing test, preventing
the execution of subsequent tests and canceling already running tests.
**Default:** `false`.
* `concurrency` {number|boolean} If a number is provided,
then that many tests would run in parallel within the application thread.
If `true`, all scheduled asynchronous tests run concurrently within the
Expand Down Expand Up @@ -3124,6 +3158,22 @@ generated for each test file in addition to a final cumulative summary.

Emitted when no more tests are queued for execution in watch mode.

### Event: `'test:bail'`

* `data` {Object}
* `column` {number|undefined} The column number where the test is defined, or
`undefined` if the test was run through the REPL.
* `file` {string|undefined} The path of the test file,
`undefined` if test was run through the REPL.
* `line` {number|undefined} The line number where the test is defined, or
`undefined` if the test was run through the REPL.
* `name` {string} The test name.
* `nesting` {number} The nesting level of the test.

Emitted when the test runner stops executing tests due to the [`--test-bail`][] flag.
This event signals that the first failing test caused the suite to bail out,
canceling all pending and currently running tests.

## Class: `TestContext`

<!-- YAML
Expand Down Expand Up @@ -3618,6 +3668,7 @@ Can be used to abort test subtasks when the test has been aborted.
[`--experimental-test-module-mocks`]: cli.md#--experimental-test-module-mocks
[`--import`]: cli.md#--importmodule
[`--no-experimental-strip-types`]: cli.md#--no-experimental-strip-types
[`--test-bail`]: cli.md#--test-bail
[`--test-concurrency`]: cli.md#--test-concurrency
[`--test-coverage-exclude`]: cli.md#--test-coverage-exclude
[`--test-coverage-include`]: cli.md#--test-coverage-include
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 @@ -76,6 +76,7 @@ function createTestTree(rootTestOptions, globalOptions) {

buildPhaseDeferred.resolve();
},
testsProcesses: new SafeMap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? It seems to only be used in one file.

};

harness.resetCounters();
Expand Down
5 changes: 4 additions & 1 deletion lib/internal/test_runner/reporter/spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const {
const assert = require('assert');
const Transform = require('internal/streams/transform');
const colors = require('internal/util/colors');
const { kSubtestsFailed } = require('internal/test_runner/test');
const { kSubtestsFailed, kTestBailedOut } = require('internal/test_runner/test');
const { getCoverageReport } = require('internal/test_runner/utils');
const { relative } = require('path');
const {
Expand Down Expand Up @@ -57,6 +57,7 @@ class SpecReporter extends Transform {
#handleEvent({ type, data }) {
switch (type) {
case 'test:fail':
if (data.details?.error?.failureType === kTestBailedOut) break;
if (data.details?.error?.failureType !== kSubtestsFailed) {
ArrayPrototypePush(this.#failedTests, data);
}
Expand All @@ -74,6 +75,8 @@ class SpecReporter extends Transform {
case 'test:coverage':
return getCoverageReport(indent(data.nesting), data.summary,
reporterUnicodeSymbolMap['test:coverage'], colors.blue, true);
case 'test:bail':
return `${reporterColorMap[type]}${reporterUnicodeSymbolMap[type]}Bail out!${colors.white}\n`;
}
}
_transform({ type, data }, encoding, callback) {
Expand Down
4 changes: 4 additions & 0 deletions lib/internal/test_runner/reporter/tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ async function * tapReporter(source) {
for await (const { type, data } of source) {
switch (type) {
case 'test:fail': {
if (data.details?.error?.failureType === lazyLoadTest().kTestBailedOut) break;
yield reportTest(data.nesting, data.testNumber, 'not ok', data.name, data.skip, data.todo);
const location = data.file ? `${data.file}:${data.line}:${data.column}` : null;
yield reportDetails(data.nesting, data.details, location);
Expand Down Expand Up @@ -61,6 +62,9 @@ async function * tapReporter(source) {
case 'test:coverage':
yield getCoverageReport(indent(data.nesting), data.summary, '# ', '', true);
break;
case 'test:bail':
yield `${indent(data.nesting)}Bail out!\n`;
break;
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions lib/internal/test_runner/reporter/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const reporterUnicodeSymbolMap = {
'test:coverage': '\u2139 ',
'arrow:right': '\u25B6 ',
'hyphen:minus': '\uFE63 ',
'test:bail': '\u2716 ',
};

const reporterColorMap = {
Expand All @@ -37,6 +38,9 @@ const reporterColorMap = {
get 'test:diagnostic'() {
return colors.blue;
},
get 'test:bail'() {
return colors.red;
},
};

function indent(nesting) {
Expand Down
41 changes: 39 additions & 2 deletions lib/internal/test_runner/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ const {
kSubtestsFailed,
kTestCodeFailure,
kTestTimeoutFailure,
kTestBailedOut,
Test,
} = require('internal/test_runner/test');

Expand All @@ -101,7 +102,10 @@ const kFilterArgValues = ['--test-reporter', '--test-reporter-destination'];
const kDiagnosticsFilterArgs = ['tests', 'suites', 'pass', 'fail', 'cancelled', 'skipped', 'todo', 'duration_ms'];

const kCanceledTests = new SafeSet()
.add(kCancelledByParent).add(kAborted).add(kTestTimeoutFailure);
.add(kCancelledByParent)
.add(kAborted)
.add(kTestTimeoutFailure)
.add(kTestBailedOut);
Comment on lines +105 to +108
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you just added to it here, but can we initialize the set without repeatedly calling add().


let kResistStopPropagation;

Expand Down Expand Up @@ -137,7 +141,8 @@ function getRunArgs(path, { forceExit,
only,
argv: suppliedArgs,
execArgv,
cwd }) {
cwd,
bail }) {
const argv = ArrayPrototypeFilter(process.execArgv, filterExecArgv);
if (forceExit === true) {
ArrayPrototypePush(argv, '--test-force-exit');
Expand All @@ -154,6 +159,9 @@ function getRunArgs(path, { forceExit,
if (only === true) {
ArrayPrototypePush(argv, '--test-only');
}
if (bail === true) {
ArrayPrototypePush(argv, '--test-bail');
}

ArrayPrototypePushApply(argv, execArgv);

Expand Down Expand Up @@ -216,6 +224,14 @@ class FileTest extends Test {
if (item.data.details?.error) {
item.data.details.error = deserializeError(item.data.details.error);
}
if (item.type === 'test:bail') {
// <-- here we need to stop all the pending test files (aka subprocesses)
// To be replaced, just for poc
this.root.harness.testsProcesses.forEach((child) => {
child.kill();
});
return;
Comment on lines +230 to +233
Copy link
Member

Choose a reason for hiding this comment

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

replace with an abort signal?

Copy link
Member Author

Choose a reason for hiding this comment

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

hey @atlowChemi, sure!

Copy link
Member

Choose a reason for hiding this comment

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

We already have an existing signal passed to the spawned process, but that is a signal at the Test level IIRC. perhaps we can add a new controller on root.harness, and spawn with a AbortSignal.any

}
if (item.type === 'test:pass' || item.type === 'test:fail') {
item.data.testNumber = isTopLevel ? (this.root.harness.counters.topLevel + 1) : item.data.testNumber;
countCompletedTest({
Expand Down Expand Up @@ -362,7 +378,12 @@ function runTestFile(path, filesWatcher, opts) {
const watchMode = filesWatcher != null;
const testPath = path === kIsolatedProcessName ? '' : path;
const testOpts = { __proto__: null, signal: opts.signal };
const subtestProcesses = opts.root.harness.testsProcesses;
const subtest = opts.root.createSubtest(FileTest, testPath, testOpts, async (t) => {
if (opts.root.bailed) {
// TODO(pmarchini): this is a temporary solution to avoid running tests after bailing
return; // No-op in order to avoid running tests after bailing
}
const args = getRunArgs(path, opts);
const stdio = ['pipe', 'pipe', 'pipe'];
const env = { __proto__: null, ...process.env, NODE_TEST_CONTEXT: 'child-v8' };
Expand All @@ -389,6 +410,7 @@ function runTestFile(path, filesWatcher, opts) {
filesWatcher.runningProcesses.set(path, child);
filesWatcher.watcher.watchChildProcessModules(child, path);
}
subtestProcesses.set(path, child);

let err;

Expand Down Expand Up @@ -422,6 +444,7 @@ function runTestFile(path, filesWatcher, opts) {
finished(child.stdout, { __proto__: null, signal: t.signal }),
]);

subtestProcesses.delete(path);
if (watchMode) {
filesWatcher.runningProcesses.delete(path);
filesWatcher.runningSubtests.delete(path);
Expand Down Expand Up @@ -478,6 +501,8 @@ function watchFiles(testFiles, opts) {
// Reset the topLevel counter
opts.root.harness.counters.topLevel = 0;
}
// TODO(pmarchini): Reset the bailed flag to rerun the tests.
// This must be added only when we add support for bail in watch mode.
await runningSubtests.get(file);
runningSubtests.set(file, runTestFile(file, filesWatcher, opts));
}
Expand Down Expand Up @@ -564,6 +589,7 @@ function run(options = kEmptyObject) {
execArgv = [],
argv = [],
cwd = process.cwd(),
bail = false,
} = options;

if (files != null) {
Expand Down Expand Up @@ -663,6 +689,15 @@ function run(options = kEmptyObject) {

validateStringArray(argv, 'options.argv');
validateStringArray(execArgv, 'options.execArgv');
validateBoolean(bail, 'options.bail');
// TODO(pmarchini): watch mode with bail needs to be implemented
if (bail && watch) {
throw new ERR_INVALID_ARG_VALUE(
'options.bail',
watch,
'bail not supported while watch mode is enabled',
);
}

const rootTestOptions = { __proto__: null, concurrency, timeout, signal };
const globalOptions = {
Expand All @@ -678,6 +713,7 @@ function run(options = kEmptyObject) {
branchCoverage: branchCoverage,
functionCoverage: functionCoverage,
cwd,
bail,
};
const root = createTestTree(rootTestOptions, globalOptions);
let testFiles = files ?? createTestFileList(globPatterns, cwd);
Expand Down Expand Up @@ -705,6 +741,7 @@ function run(options = kEmptyObject) {
isolation,
argv,
execArgv,
bail,
};

if (isolation === 'process') {
Expand Down
Loading
Loading