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: bail #48919

Closed
wants to merge 25 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
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
13 changes: 13 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -1507,6 +1507,18 @@ 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`
marco-ippolito marked this conversation as resolved.
Show resolved Hide resolved

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

marco-ippolito marked this conversation as resolved.
Show resolved Hide resolved
> 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-name-pattern`

<!-- YAML
Expand Down Expand Up @@ -2711,6 +2723,7 @@ done
[security warning]: #warning-binding-inspector-to-a-public-ipport-combination-is-insecure
[semi-space]: https://www.memorymanagement.org/glossary/s.html#semi.space
[single executable application]: single-executable-applications.md
[test bailout]: test.md#bailing-out
[test reporters]: test.md#test-reporters
[timezone IDs]: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
[tracking issue for user-land snapshots]: https://github.com/nodejs/node/issues/44014
Expand Down
21 changes: 21 additions & 0 deletions doc/api/test.md
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,22 @@ test('mocks setTimeout to be executed synchronously without having to actually w
});
```

## Bailing out

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

> Stability: 1 - Experimental

The `--test-bail` flag provides a way to stop the 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.
**Default:** `false`.

## Test reporters

<!-- YAML
Expand Down Expand Up @@ -870,6 +886,11 @@ changes:

* `options` {Object} Configuration options for running tests. The following
properties are supported:
* `bail` {boolean}
If `true`, it will exit the test suite early
when it encounters the first failing test, preventing
the execution of subsequent tests.
**Default:** `false`.
* `concurrency` {number|boolean} If a number is provided,
then that many test processes would run in parallel, where each process
corresponds to one test file.
Expand Down
3 changes: 3 additions & 0 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,9 @@ Specify the minimum allocation from the OpenSSL secure heap. The default is 2. T
.It Fl -test
Starts the Node.js command line test runner.
.
.It Fl -test-bail
Instructs the test runner to bail out if a test failure occurs.
.
.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
7 changes: 6 additions & 1 deletion lib/internal/main/test_runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,12 @@ if (shardOption) {
};
}

run({ concurrency, inspectPort, watch: getOptionValue('--watch'), setup: setupTestReporters, shard })
run({ concurrency,
inspectPort,
watch: getOptionValue('--watch'),
setup: setupTestReporters,
shard,
bail: getOptionValue('--test-bail') })
.on('test:fail', (data) => {
if (data.todo === undefined || data.todo === false) {
process.exitCode = kGenericUserError;
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/test_runner/reporter/tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const {
const { inspectWithNoCustomRetry } = require('internal/errors');
const { isError, kEmptyObject } = require('internal/util');
const { getCoverageReport } = require('internal/test_runner/utils');
const { kBailedOut } = require('internal/test_runner/test');
const kDefaultIndent = ' '; // 4 spaces
const kFrameStartRegExp = /^ {4}at /;
const kLineBreakRegExp = /\n|\r\n/;
Expand All @@ -32,6 +33,8 @@ async function * tapReporter(source) {
for await (const { type, data } of source) {
switch (type) {
case 'test:fail': {
// If test bailed out do not report
Copy link
Member

Choose a reason for hiding this comment

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

so a test can run without us reporting it? I am not sure this is good

Copy link
Member Author

@marco-ippolito marco-ippolito Aug 30, 2023

Choose a reason for hiding this comment

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

The test has error kBailedOut because has been canceled, I dont want to see canceled tests in the reporter because they bailedout

if (data.details?.error?.failureType === kBailedOut) break;
yield reportTest(data.nesting, data.testNumber, 'not ok', data.name, data.skip, data.todo);
const location = `${data.file}:${data.line}:${data.column}`;
yield reportDetails(data.nesting, data.details, location);
Expand Down
22 changes: 14 additions & 8 deletions lib/internal/test_runner/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,17 @@ function filterExecArgv(arg, i, arr) {
!ArrayPrototypeSome(kFilterArgValues, (p) => arg === p || (i > 0 && arr[i - 1] === p) || StringPrototypeStartsWith(arg, `${p}=`));
}

function getRunArgs({ path, inspectPort, testNamePatterns }) {
function getRunArgs({ path, inspectPort, testNamePatterns, bail }) {
const argv = ArrayPrototypeFilter(process.execArgv, filterExecArgv);
if (isUsingInspector()) {
ArrayPrototypePush(argv, `--inspect-port=${getInspectPort(inspectPort)}`);
}
if (testNamePatterns) {
ArrayPrototypeForEach(testNamePatterns, (pattern) => ArrayPrototypePush(argv, `--test-name-pattern=${pattern}`));
}
if (bail) {
ArrayPrototypePush(argv, '--test-bail');
}
ArrayPrototypePush(argv, path);

return argv;
Expand Down Expand Up @@ -301,10 +304,10 @@ class FileTest extends Test {
}
}

function runTestFile(path, root, inspectPort, filesWatcher, testNamePatterns) {
function runTestFile(path, root, inspectPort, filesWatcher, testNamePatterns, bail) {
const watchMode = filesWatcher != null;
const subtest = root.createSubtest(FileTest, path, async (t) => {
const args = getRunArgs({ __proto__: null, path, inspectPort, testNamePatterns });
const args = getRunArgs({ __proto__: null, path, inspectPort, testNamePatterns, bail });
const stdio = ['pipe', 'pipe', 'pipe'];
const env = { __proto__: null, ...process.env, NODE_TEST_CONTEXT: 'child-v8' };
if (watchMode) {
Expand Down Expand Up @@ -381,7 +384,7 @@ function runTestFile(path, root, inspectPort, filesWatcher, testNamePatterns) {
return subtest.start();
}

function watchFiles(testFiles, root, inspectPort, signal, testNamePatterns) {
function watchFiles(testFiles, root, inspectPort, signal, testNamePatterns, bail) {
const runningProcesses = new SafeMap();
const runningSubtests = new SafeMap();
const watcher = new FilesWatcher({ __proto__: null, debounce: 200, mode: 'filter', signal });
Expand All @@ -403,7 +406,7 @@ function watchFiles(testFiles, root, inspectPort, signal, testNamePatterns) {
root.harness.counters.topLevel = 0;
}
await runningSubtests.get(file);
runningSubtests.set(file, runTestFile(file, root, inspectPort, filesWatcher, testNamePatterns));
runningSubtests.set(file, runTestFile(file, root, inspectPort, filesWatcher, testNamePatterns, bail));
}, undefined, (error) => {
triggerUncaughtException(error, true /* fromPromise */);
}));
Expand All @@ -425,14 +428,17 @@ function run(options) {
options = kEmptyObject;
}
let { testNamePatterns, shard } = options;
const { concurrency, timeout, signal, files, inspectPort, watch, setup } = options;
const { concurrency, timeout, signal, files, inspectPort, watch, setup, bail } = options;

if (files != null) {
validateArray(files, 'options.files');
}
if (watch != null) {
validateBoolean(watch, 'options.watch');
}
if (bail != null) {
validateBoolean(bail, 'options.bail');
}
if (shard != null) {
validateObject(shard, 'options.shard');
// Avoid re-evaluating the shard object in case it's a getter
Expand Down Expand Up @@ -479,13 +485,13 @@ function run(options) {
let postRun = () => root.postRun();
let filesWatcher;
if (watch) {
filesWatcher = watchFiles(testFiles, root, inspectPort, signal, testNamePatterns);
filesWatcher = watchFiles(testFiles, root, inspectPort, signal, testNamePatterns, bail);
marco-ippolito marked this conversation as resolved.
Show resolved Hide resolved
postRun = undefined;
}
const runFiles = () => {
root.harness.bootstrapComplete = true;
return SafePromiseAllSettledReturnVoid(testFiles, (path) => {
const subtest = runTestFile(path, root, inspectPort, filesWatcher, testNamePatterns);
const subtest = runTestFile(path, root, inspectPort, filesWatcher, testNamePatterns, bail);
filesWatcher?.runningSubtests.set(path, subtest);
return subtest;
});
Expand Down
27 changes: 18 additions & 9 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ const kParentAlreadyFinished = 'parentAlreadyFinished';
const kSubtestsFailed = 'subtestsFailed';
const kTestCodeFailure = 'testCodeFailure';
const kTestTimeoutFailure = 'testTimeoutFailure';
const kBailedOut = 'testBailedOut';
const kHookFailure = 'hookFailed';
const kDefaultTimeout = null;
const noop = FunctionPrototype;
Expand All @@ -75,7 +76,7 @@ const kHookNames = ObjectSeal(['before', 'after', 'beforeEach', 'afterEach']);
const kUnwrapErrors = new SafeSet()
.add(kTestCodeFailure).add(kHookFailure)
.add('uncaughtException').add('unhandledRejection');
const { testNamePatterns, testOnlyFlag } = parseCommandLine();
const { testNamePatterns, testOnlyFlag, bail } = parseCommandLine();
let kResistStopPropagation;

function stopTest(timeout, signal) {
Expand Down Expand Up @@ -204,6 +205,7 @@ class Test extends AsyncResource {
abortController;
outerSignal;
#reportedSubtest;
bailedOut;

constructor(options) {
super('Test');
Expand Down Expand Up @@ -469,13 +471,14 @@ class Test extends AsyncResource {
if (this.endTime !== null) {
return;
}

this.fail(error ||
new ERR_TEST_FAILURE(
'test did not finish before its parent and was cancelled',
kCancelledByParent,
),
const cancelledError = this.root.bailedOut ? new ERR_TEST_FAILURE(
'test bailed out',
kBailedOut,
) : new ERR_TEST_FAILURE(
'test did not finish before its parent and was cancelled',
kCancelledByParent,
);
this.fail(error || cancelledError);
this.startTime = this.startTime || this.endTime; // If a test was canceled before it was started, e.g inside a hook
this.cancelled = true;
this.abortController.abort();
Expand All @@ -500,6 +503,11 @@ class Test extends AsyncResource {
this.endTime = hrtime();
this.passed = false;
this.error = err;
if (bail && !this.root.bailedOut) {
this.bailedOut = true;
this.root.bailedOut = true;
this.root.postRun();
}
}

pass() {
Expand Down Expand Up @@ -811,7 +819,7 @@ class Test extends AsyncResource {

if (this.passed) {
this.reporter.ok(this.nesting, this.loc, this.testNumber, this.name, details, directive);
} else {
} else if (this.error.failureType !== kBailedOut) {
details.error = this.error;
this.reporter.fail(this.nesting, this.loc, this.testNumber, this.name, details, directive);
}
Expand All @@ -822,7 +830,7 @@ class Test extends AsyncResource {
}

reportStarted() {
if (this.#reportedSubtest || this.parent === null) {
if (this.#reportedSubtest || this.parent === null || this.root.bailedOut) {
marco-ippolito marked this conversation as resolved.
Show resolved Hide resolved
return;
}
this.#reportedSubtest = true;
Expand Down Expand Up @@ -968,6 +976,7 @@ module.exports = {
kSubtestsFailed,
kTestCodeFailure,
kTestTimeoutFailure,
kBailedOut,
kAborted,
kUnwrapErrors,
Suite,
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 @@ -179,6 +179,7 @@ function parseCommandLine() {
}

const isTestRunner = getOptionValue('--test');
const bail = getOptionValue('--test-bail');
const coverage = getOptionValue('--experimental-test-coverage');
const isChildProcess = process.env.NODE_TEST_CONTEXT === 'child';
const isChildProcessV8 = process.env.NODE_TEST_CONTEXT === 'child-v8';
Expand Down Expand Up @@ -230,6 +231,7 @@ function parseCommandLine() {
globalTestOptions = {
__proto__: null,
isTestRunner,
bail,
coverage,
testOnlyFlag,
testNamePatterns,
Expand Down
3 changes: 3 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,9 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
"run test at specific shard",
&EnvironmentOptions::test_shard,
kAllowedInEnvvar);
AddOption("--test-bail",
"stop test runner execution on the first test failure",
&EnvironmentOptions::test_bail);
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 @@ -161,6 +161,7 @@ class EnvironmentOptions : public Options {
std::string env_file;
bool has_env_file_string = false;
bool test_runner = false;
bool test_bail = false;
bool test_runner_coverage = false;
std::vector<std::string> test_name_pattern;
std::vector<std::string> test_reporter;
Expand Down
18 changes: 18 additions & 0 deletions test/fixtures/test-runner/bail/bail.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';
const common = require('../../../common');
const test = require('node:test');
marco-ippolito marked this conversation as resolved.
Show resolved Hide resolved

test('nested', (t) => {
t.test('first', () => {});
t.test('second', () => {
throw new Error();
});
t.test('third', common.mustNotCall(() => {}));
});

test('top level', (t) => {
t.test('forth', () => {});
t.test('fifth', () => {
throw new Error();
});
});
12 changes: 12 additions & 0 deletions test/fixtures/test-runner/bail/error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
'use strict';
const common = require('../../../common');
marco-ippolito marked this conversation as resolved.
Show resolved Hide resolved
const assert = require('assert');
const test = require('node:test');

test('keep error', (t) => {
assert.strictEqual(0, 1);
});

test('dont show', common.mustNotCall((t) => {
assert.strictEqual(0, 2);
}));
13 changes: 13 additions & 0 deletions test/fixtures/test-runner/output/bail-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Flags: --test-bail --test-reporter=tap
'use strict';
const common = require('../../../common');
const assert = require('assert');
const test = require('node:test');

test('keep error', (t) => {
assert.strictEqual(0, 1);
});

test('dont show', common.mustNotCall((t) => {
assert.strictEqual(0, 2);
}));
32 changes: 32 additions & 0 deletions test/fixtures/test-runner/output/bail-error.snapshot
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
TAP version 13
not ok 1 - keep error
---
duration_ms: *
location: '/test/fixtures/test-runner/output/bail-error.js:(LINE):1'
failureType: 'testCodeFailure'
error: |-
Expected values to be strictly equal:

0 !== 1

code: 'ERR_ASSERTION'
name: 'AssertionError'
expected: 1
actual: 0
operator: 'strictEqual'
stack: |-
*
*
*
*
*
...
1..2
# tests 2
# suites 0
# pass 0
# fail 1
# cancelled 1
# skipped 0
# todo 0
# duration_ms *
Loading