-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
base: main
Are you sure you want to change the base?
test_runner: add bail out #56490
Changes from 11 commits
f1c269c
1d68db4
e691cd6
d8efaec
e213dbf
9514d89
a97c98b
4d85df5
39a21ba
0364d15
c3c7999
eadba47
db86232
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here. Also, why is bailing out only supported in the spec and tap reporters? What about the dot reporter for example? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't implemented all the reporters yet, as I'm still not convinced by the bail implementation itself. |
||
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); | ||
|
@@ -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; | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,7 @@ const { | |
kSubtestsFailed, | ||
kTestCodeFailure, | ||
kTestTimeoutFailure, | ||
kTestBailedOut, | ||
Test, | ||
} = require('internal/test_runner/test'); | ||
|
||
|
@@ -100,8 +101,12 @@ const kFilterArgs = ['--test', '--experimental-test-coverage', '--watch']; | |
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); | ||
const kCanceledTests = new SafeSet([ | ||
kCancelledByParent, | ||
kAborted, | ||
kTestTimeoutFailure, | ||
kTestBailedOut, | ||
]); | ||
|
||
let kResistStopPropagation; | ||
|
||
|
@@ -137,7 +142,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'); | ||
|
@@ -154,6 +160,9 @@ function getRunArgs(path, { forceExit, | |
if (only === true) { | ||
ArrayPrototypePush(argv, '--test-only'); | ||
} | ||
if (bail === true) { | ||
ArrayPrototypePush(argv, '--test-bail'); | ||
} | ||
|
||
ArrayPrototypePushApply(argv, execArgv); | ||
|
||
|
@@ -194,7 +203,14 @@ class FileTest extends Test { | |
} | ||
|
||
#skipReporting() { | ||
return this.#reportedChildren > 0 && (!this.error || this.error.failureType === kSubtestsFailed); | ||
return ( | ||
( | ||
this.#reportedChildren > 0 && | ||
(!this.error || | ||
this.error.failureType === kSubtestsFailed || | ||
this.error.failureType === kTestBailedOut | ||
) | ||
)); | ||
} | ||
#checkNestedComment(comment) { | ||
const firstSpaceIndex = StringPrototypeIndexOf(comment, ' '); | ||
|
@@ -216,6 +232,12 @@ class FileTest extends Test { | |
if (item.data.details?.error) { | ||
item.data.details.error = deserializeError(item.data.details.error); | ||
} | ||
if (item.type === 'test:bail') { | ||
this.root.harness.abortTestTree(); | ||
this.root.bailed = true; | ||
this.bailed = true; | ||
return; | ||
} | ||
if (item.type === 'test:pass' || item.type === 'test:fail') { | ||
item.data.testNumber = isTopLevel ? (this.root.harness.counters.topLevel + 1) : item.data.testNumber; | ||
countCompletedTest({ | ||
|
@@ -478,6 +500,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)); | ||
} | ||
|
@@ -564,6 +588,7 @@ function run(options = kEmptyObject) { | |
execArgv = [], | ||
argv = [], | ||
cwd = process.cwd(), | ||
bail = false, | ||
} = options; | ||
|
||
if (files != null) { | ||
|
@@ -663,6 +688,15 @@ function run(options = kEmptyObject) { | |
|
||
validateStringArray(argv, 'options.argv'); | ||
validateStringArray(execArgv, 'options.execArgv'); | ||
validateBoolean(bail, 'options.bail'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should probably follow the same pattern as |
||
// TODO(pmarchini): watch mode with bail needs to be implemented | ||
if (bail && watch) { | ||
throw new ERR_INVALID_ARG_VALUE( | ||
'options.bail', | ||
watch, | ||
'bail is not supported while watch mode is enabled', | ||
); | ||
} | ||
|
||
const rootTestOptions = { __proto__: null, concurrency, timeout, signal }; | ||
const globalOptions = { | ||
|
@@ -678,6 +712,7 @@ function run(options = kEmptyObject) { | |
branchCoverage: branchCoverage, | ||
functionCoverage: functionCoverage, | ||
cwd, | ||
bail, | ||
}; | ||
const root = createTestTree(rootTestOptions, globalOptions); | ||
let testFiles = files ?? createTestFileList(globPatterns, cwd); | ||
|
@@ -705,6 +740,7 @@ function run(options = kEmptyObject) { | |
isolation, | ||
argv, | ||
execArgv, | ||
bail, | ||
}; | ||
|
||
if (isolation === 'process') { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of needing to check this for every failure, even if bail out mode is not enabled. Couldn't we break out in the
test:bail
event?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but in that case, we would lose the "report' section.
Specifically, how would you do that? Would you finalise the reporter, or propagate the abort up to the test runner root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmarchini we need to also account for the fact these changes impact custom reporters