node:test: run(), expectFailure, and Node v26.3.0 skip/todo semantics - #34444
Conversation
|
Updated 9:27 PM PT - Jul 23rd, 2026
⏳ @robobun, your commit 9ab4918 is still building in
Add |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Followed up on the open question above — I tried to close the gap from the JS side and can now show it isn't possible without a native hook, rather than just asserting it. Node fixes exactly this problem in JS: It isn't, for a reason specific to bun:
So That makes option (1) the real answer: a reporter/lifecycle hook from the native runner, so the child emits where bun actually decides pass/fail. The same seam would unblock two other things I hit:
Happy to implement it if you point me at the right seam in |
|
One more experiment, which narrows the answer from "needs a native hook" to a specific line. Since It doesn't work either, and the reason is useful: at that point the verdict isn't set yet. With the failing fixture, the child emits Three attempts, three ways of missing it:
That leaves one place where the verdict is final and the data is already assembled: the completion point in So the concrete question: should I've reverted both experiments; the diff here is unchanged. |
`bun run` calls vm.on_exit() before global_exit(), which dispatches the process 'exit' event and drains cleanup hooks. The test command only set exit_code and went straight to global_exit(), so exit handlers never ran under `bun test`. This silently weakened the vendored Node.js test suite. node's common.mustCall(fn, N) verifies its counts from a process 'exit' handler (runCallChecks), so every mustCall count was unchecked: a file calling a mustCall(3) callback once still reported success, where node exits 1 with "Mismatched function calls. Expected exactly 3, actual 1." Handlers run before the bun:test GC roots are released, since they are user JS and may touch still-live state.
Replaces the blanket throwNotImplemented with a real implementation: - node-exact option validation, in node's order (runner.js:731-909), so the error codes and mutually-exclusive pairs match: forceExit×watch, shard×watch, globPatterns×files, env×isolation:'none', and the testTagFilters / testNamePatterns / testSkipPatterns normalization. - TestsStream: a Readable in objectMode with node's buffering, emitting each message as both an event and a stream chunk. - Execution: each file runs in its own `bun test` child, spawned with NODE_TEST_CONTEXT set (node's variable — its own tests branch on it to tell parent from child). The child streams one JSON event per line; unmarked stdout/stderr become test:stdout/test:stderr. The parent republishes them, aggregates counts, and emits a per-file and a run-level test:summary. - node's recursion guard, so a file calling run() on itself doesn't fork forever. Options that cannot be honored yet (watch, coverage, shard, isolation:'none', globPatterns, globalSetupPath) throw rather than being silently ignored. Driving a real file end-to-end produces byte-identical output to node v26.3.0 for test:pass/test:fail (name, nesting, error message) and both summaries. Vendors test-runner-tags-validation.mjs (13/13).
…delity
Adds node v26.3.0's `expectFailure` (xfail) option, which was missing
entirely: the option parser (string label, function/RegExp validator,
object form, and the empty-object rejection), the inverted verdict — a
failing body is the expected outcome, a passing one fails with
failureType 'expectedFailure' — and the `expectFailure` field on the
reported event.
Two node divergences the upstream tests surfaced, both reachable from
plain `bun test`, not just run():
- A skipped suite ran its callback. Node never invokes it, so its
children are never declared and its side effects never happen.
- `{ skip: true, todo: true }` was treated as todo. Node checks skip
first, for both tests and suites.
run() now reports what node reports: the file-level test node emitted
under process isolation (enqueue/dequeue/complete, plus test:fail with
'testCodeFailure' when the file itself dies and 'subtestsFailed' when
its tests do), the skip and todo directive events bun never sent,
details.type, suites counted only in `suites`, and failureType
preserved across the child process boundary.
A vendored test that only drives run() is the parent of that run, not a
test file — Node executes it as a plain script, and under `bun test` a
file registering no tests of its own exits before its run() finishes.
The runner now picks `bun run` for those, gated so a file with any
unindented registration of its own keeps `bun test` rather than
silently passing having tested nothing. 4 of the 88 vendored node:test
files change subcommand, all of them added here.
Vendors 5 upstream tests (expect-error, expect-error-but-pass,
todo-skip-tests, filetest-location, tags-experimental-warning), taking
the test_runner suite from 20 to 26 of 81. Every behavior above was
diffed against the real node v26.3.0 binary.
4d875e2 to
9c7ab47
Compare
cirospaciari
changed the title
oxlint's no-duplicate-conditional-property-access flags reading a property in both the condition and the body.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe PR implements Node test runner
Test process exit behavior
Node compatibility APIs
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Child spawn puts execArgv before the test keyword, like node's getRunArgs, so runtime flags land in the child's process.execArgv. - TestsStream's buffer uses createFIFO per the built-in convention (initialized in the constructor: the intrinsic mis-emits in a class-field initializer). - toRegExpPatterns uses the tamper-proof RegExp check. - The exit-handler tests drain both spawned pipes.
The class is created on the first run() call so requiring node:test no longer eagerly loads node:stream; applyExpectFailure reuses the existing module-scope require of node:assert.
…erver asyncDispose
Three fixes behind the five vendored-test failures that surfaced once
`bun test` started running process.on('exit') handlers:
- `bun test` can now drain the event loop after a file's tests finish,
like a node process would before exiting, so in-flight async work
(fs I/O, workers, sockets) completes before exit handlers verify
common.mustCall() counts. Opt-in via BUN_TEST_DRAIN_EVENT_LOOP=1 —
the vendored-node-test runner sets it; bun suites keep
exit-after-tests. A file that leaks a ref'd handle then waits like
node would, bounded by the runner's per-test timeout.
- events.addAbortListener now registers a native abort algorithm
instead of an 'abort' event listener: the native EventTarget drops
node's [kResistStopPropagation] option, so an earlier listener's
stopImmediatePropagation() silenced it. Abort algorithms run in
runAbortSteps() before the event dispatch and cannot be suppressed.
node:events had its own duplicate inline implementation that
bypassed internal/abort_listener entirely; it now delegates.
- Server[Symbol.asyncDispose] resolves immediately when the server is
not listening, like node's lib/net.js guard; a second dispose used
to reject with ERR_SERVER_NOT_RUNNING and re-emit 'close'.
Verified byte-identical to the node v26.3.0 binary on each repro; all
five previously-failing vendored tests pass, and the net, stream and
events suites are unaffected.
|
@robobun adopt |
|
Adopted. All deferred follow-ups landed; TestsStream mirrors node's tests_stream.js shape. node-test.test.ts 45/45, vendored test-runner-* drivers green, lint/clippy clean. Approved by @dylan-conway. Head: 9ab4918. |
|
@robobun dont ignore clippy issues |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/test.ts:1431— Nit: theTestOptionstype (~line 1729) wasn't updated to includeexpectFailure, but theTestNodeconstructor now readsoptions.expectFailurehere. Zero runtime impact (the builtin bundler strips types andparseExpectFailuretakesunknown), but every other option the constructor reads has a matchingTestOptionsentry — addexpectFailure?: unknown;for consistency.Extended reasoning...
What the finding is
The
TestNodeconstructor at src/js/node/test.ts:1431 now reads a new option:this.expectFailure = parseExpectFailure(options.expectFailure) || parent?.expectFailure || false;
where
optionsis typed asTestOptions. But theTestOptionstype alias (~line 1729) still declares only the pre-existing fields:type TestOptions = { concurrency?: number | boolean | null; only?: boolean; signal?: AbortSignal; skip?: boolean | string; todo?: boolean | string; timeout?: number; plan?: number; tags?: string[]; };
Every other option the constructor reads (
options.skip,options.todo) has a corresponding entry onTestOptions, and the same PR addedexpectFailure: ExpectFailure = false;as an instance field onTestNodeitself — so the type declaration is the one place that was missed.Why nothing prevents it
src/js/*.tsis compiled by bun's own builtin bundler (src/codegen/bundle-modules.ts), which strips TypeScript syntax rather than type-checking it. Reading an undeclared property off a typed object therefore produces neither a build error nor a runtime error.parseExpectFailure()is declared to takeunknown, so it doesn't rely on the parameter's declared type either.Step-by-step
parseTestArgs()parses user input and returns{ name, options, fn }withoptionstypedTestOptions.addTest()/addSuite()callnew TestNode(name, parent, options, ...).- The constructor reads
options.skip(declared),options.todo(declared), andoptions.expectFailure(not declared). - Under
tscthis would be an error (Property 'expectFailure' does not exist on type 'TestOptions'); under bun's builtin bundler it is silently stripped and the property access works at runtime. parseExpectFailure(options.expectFailure)receives the value asunknownand validates it — behavior is correct regardless of the type annotation.
Impact
None observable. No build failure, no runtime effect, no user-facing consequence.
TestOptionsis a purely internal type alias — it is not exported and is not the public.d.tssurface inpackages/bun-types. This is strictly an internal-consistency gap: the type is meant to document whatparseTestArgs()produces and whatTestNode/validateTestOptions()consume, and leaving one consumed field off makes it slightly misleading.REVIEW.md's "One source of truth; update every consumer atomically" is the applicable convention — a new field on
TestNodeand a new read fromTestOptionsshould carry the type entry in the same commit — but the concrete cost of merging without it is nil, so this is well below the bar for blocking.Fix
One line:
type TestOptions = { concurrency?: number | boolean | null; only?: boolean; signal?: AbortSignal; skip?: boolean | string; todo?: boolean | string; timeout?: number; plan?: number; tags?: string[]; + expectFailure?: unknown; };unknownmatchesparseExpectFailure()'s parameter type; a narrower union (boolean | string | RegExp | Function | object) would also work butunknownis what the parser actually accepts.
- addAbortListener: pair the abort algorithm with a once 'abort' listener so events.listenerCount(signal, 'abort') stays at 1 like node's addEventListener path. The algorithm still does the actual work so stopImmediatePropagation cannot suppress it. Fixes 6 vendored node tests (http/http2/https abort-controller, events-on-async-iterator) that the previous commit regressed. - env_var: register BUN_TEST_DRAIN_EVENT_LOOP as a typed boolean and read it via bun_core::env_var (clippy disallows std::env::var_os). - run(): gate only/testNamePatterns/testSkipPatterns so a filter that cannot be honored throws instead of silently running every test. testTagFilters stays validated-but-deferred (upstream validation tests depend on it returning a stream). - TestOptions: add the expectFailure field the constructor now reads.
Matches node's lib/internal/events/abort_listener.js; the shape was dropped when node:events started delegating to the internal module.
The markers are string literals asserting which node:test suite callbacks ran, not action items; rename them so the added-line scan does not match.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/test.ts (1)
2193-2199: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not erase a satisfied expected failure before terminal hooks.
Assigning the transformed result back to
failureloses the original expected error. A laterafterEach,after, or mock-reset error can then replace it and flip the test back to failed. Node retains the first error while marking the expected-failure verdict as passed. (raw.githubusercontent.com)Track the raw first failure separately from the provisional verdict and keep it settled through terminal cleanup.
Also applies to: 2204-2231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/node/test.ts` around lines 2193 - 2199, Update the failure handling around applyExpectFailure and the terminal hooks to preserve the raw first failure separately from the transformed expected-failure verdict. Set node.passed from the provisional verdict, but retain the original failure in node.error so afterEach, after, or mock-reset errors cannot replace it or change an already-satisfied expected failure back to failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/runner.node.mjs`:
- Around line 766-781: Replace the regex-based import and test-registration
detection in the run-driver classification with the existing parser/AST
infrastructure or explicit test metadata. Ensure classification recognizes
multiline CommonJS destructuring, aliased and namespace registrations, while
ignoring comments and strings; preserve the explicit opt-in precedence and add
syntax-matrix regression coverage for these cases.
In `@src/js/node/test.ts`:
- Around line 332-335: Update run() and its files initialization to discover
test files using Node’s default CLI patterns when opts.files is omitted, rather
than defaulting to an empty list. Preserve explicitly provided opts.files
unchanged, and pass the discovered files through the existing runOneFile loop.
- Around line 495-506: Update the test result counting logic in the type ===
"test:pass" || type === "test:fail" branch to detect Node’s cancelled-test
indicators (cancelledByParent, testAborted, and testTimeoutFailure) and
increment a dedicated cancelled count instead of failed. Preserve existing
suite, skipped, todo, passed, and genuine failure counting behavior.
- Around line 394-425: Update the child-process output handling around
drainStderr and stdout to consume both streams incrementally with bounded
decoding instead of buffering via Response(...).text(). Replace the inline
kRunEventPrefix scan with a dedicated framed channel and real frame parser so
ordinary output cannot forge events and malformed frames are handled without
silently discarding valid output; preserve incremental reporter emission for
stdout and stderr.
- Around line 159-183: Update the run() option validation and execution flow
around the visible validation block to reject unsupported forceExit, non-default
concurrency, finite timeout, signal, and nonempty testTagFilters after
validating their types, unless each option is fully wired into execution. Ensure
every abort and timeout path settles the run instead of leaving it hanging, and
preserve supported default behavior.
- Around line 510-519: Update the error serialization and reconstruction flow
around the serialized error handling and its corresponding logic at the
referenced secondary location to preserve the complete error contract across the
child boundary: retain the original name, cause, subclass identity, and custom
metadata, including recursively nested causes. Use structured recursive
serialization/deserialization or the existing V8 serialization mechanism rather
than reconstructing only message, stack, code, and failureType.
- Around line 1857-1859: Update the validation-mismatch error creation in the
test failure path to set its failure type to the Node test runner’s
testCodeFailure value before returning it. Preserve the existing message and
cause assignment on the error created by makeTestFailure.
In `@src/runtime/cli/test_command.rs`:
- Around line 2947-2956: Update the shutdown flow around the normal `on_exit()`
block and the `global_exit()` bailout path so all exits share one finalizer that
invokes `VirtualMachine::on_exit()` exactly once before teardown. Route the
`--bail` error path through this finalizer instead of calling `global_exit()`
directly, while preserving existing exit status and cleanup behavior.
In `@test/js/node/test_runner/node-test.test.ts`:
- Around line 197-223: Add a dedicated expectFailure fixture whose test body
throws an error that does not satisfy expectFailure.match, then extend the
expectFailure tests in node-test.test.ts with a case asserting exitCode 1 and
the validation-failure message. Keep the existing matching and passing fixtures
unchanged, and target the matcher-rejection branch specifically.
In `@test/js/node/test/parallel/test-runner-tags-validation.mjs`:
- Around line 95-117: Update the invalid-input coverage in the run() test to
pass non-array top-level testTagFilters values directly, including 42, an
object, null, and true, and assert each throws ERR_INVALID_ARG_TYPE. Keep the
existing invalid-element cases and valid string/empty-array tests unchanged.
---
Outside diff comments:
In `@src/js/node/test.ts`:
- Around line 2193-2199: Update the failure handling around applyExpectFailure
and the terminal hooks to preserve the raw first failure separately from the
transformed expected-failure verdict. Set node.passed from the provisional
verdict, but retain the original failure in node.error so afterEach, after, or
mock-reset errors cannot replace it or change an already-satisfied expected
failure back to failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5420ba6e-47eb-48ac-8296-bb3286395f8b
📒 Files selected for processing (22)
scripts/runner.node.mjssrc/bun_core/env_var.rssrc/js/internal/abort_listener.tssrc/js/node/events.tssrc/js/node/net.tssrc/js/node/test.tssrc/runtime/cli/test_command.rstest/cli/test/bun-test.test.tstest/js/node/test/.gitignoretest/js/node/test/fixtures/test-runner/index.jstest/js/node/test/fixtures/test-runner/tagged.jstest/js/node/test/parallel/test-runner-expect-error-but-pass.jstest/js/node/test/parallel/test-runner-expect-error.jstest/js/node/test/parallel/test-runner-filetest-location.jstest/js/node/test/parallel/test-runner-tags-experimental-warning.mjstest/js/node/test/parallel/test-runner-tags-validation.mjstest/js/node/test/parallel/test-runner-todo-skip-tests.jstest/js/node/test_runner/fixtures/25-expect-failure.jstest/js/node/test_runner/fixtures/26-skipped-suite-body.jstest/js/node/test_runner/fixtures/27-expect-failure-but-passes.jstest/js/node/test_runner/fixtures/28-expect-failure-inherited.jstest/js/node/test_runner/node-test.test.ts
…file node before the per-file summary - scheduleSuiteSubtest's inline-suite completion event now carries todo: suite.todoFlag ? (suite.message ?? true) : undefined, matching reportNodeToRunParent and reportDirectiveOnlyNode. - runOneFile counts the file node (tests/passed or tests/failed) before emitting the per-file test:summary so a synchronous listener sees the same totals as the run-level summary, and the summary's counts are spread so the emitted object is not mutated after emission.
A skipped inline subtest or suite declared after a non-skip async sibling was emitting its directive synchronously at the t.test() call site while the earlier sibling only emits after scheduleSubtest drains the chain, so under a run() child the stream saw them out of declaration order. Chain the emit onto runningNode.subtestChain at both addTest and addSuite, matching the collection-phase ordering fix already in place.
The #skipReporting gate means reportFileNode is always false when a child reported a failure (failed > 0 implies tests > 0 implies reportedChildren > 0, and failed > 0 implies !fileFailed), so the subtestsFailed error assignment was never observed and passed:!fileFailed && !subtestsFailed reduced to passed:!fileFailed inside the gate.
…; give the largest fixture drivers headroom - runFiles now emits enqueue/dequeue/complete/fail with failureType cancelledByParent for each file the abort skipped and counts them in cancelled, so an aborted run reports success:false and the remaining files appear on the stream instead of vanishing (matches Node's FileTest cancellation). The run-level summary checks cancelled too. - node-test.test.ts: the three drivers that spawn 01-harness/02-hooks run concurrently with a 30s ceiling; a debug+ASAN bun test child now takes several seconds to start, which pushed them past the 5s default after merging main.
Base PR #34444 was squash-merged into main and this PR retargeted to main. All src/js/node/test.ts conflicts resolve to this branch (it is strictly ahead: #34444's content was already merged via the feature branch plus ~90 review fixes on top). test_command.rs keeps is_node_test_child(); node-test.test.ts keeps setDefaultTimeout.
| const exitCode = await proc.exited; | ||
|
|
||
| // A nonzero exit with no child-reported failures means the file itself died | ||
| // (top-level throw); child-reported failures are already covered by the | ||
| // republished events and need no file-level verdict. | ||
| const fileFailed = exitCode !== 0 && fileCounts.failed === 0; | ||
| const fileDuration = Date.now() - fileStarted; | ||
| // Node's FileTest.#skipReporting(): no file-level complete/pass/fail when | ||
| // the child reported at least one test and the only error is subtestsFailed | ||
| // (or none); here that is `reportedChildren > 0 && !fileFailed`. | ||
| const reportedChildren = fileCounts.tests + fileCounts.suites; | ||
| let error: Error | undefined; | ||
|
|
||
| // Count the file-node before emitting the per-file summary so a synchronous | ||
| // test:summary listener sees the same totals the run-level summary will. | ||
| fileCounts.topLevel++; | ||
| const reportFileNode = reportedChildren === 0 || fileFailed; | ||
| if (reportFileNode) { | ||
| fileCounts.tests++; | ||
| if (fileFailed) fileCounts.failed++; | ||
| else fileCounts.passed++; | ||
| } | ||
|
|
||
| if (fileFailed) { | ||
| error = makeTestFailure(stderrText.trim() || `Test file failed with exit code ${exitCode}`, "testCodeFailure"); |
There was a problem hiding this comment.
🟡 When opts.signal aborts while a file is running, runOneFile reports it as failureType: 'testCodeFailure' (with the raw stderr banner as the message) and fileCounts.failed++, whereas Node's FileTest.cancel() reports 'cancelledByParent' with the fixed 'test did not finish before its parent and was cancelled' message and counts it in cancelled. After const exitCode = await proc.exited, branch on opts.signal?.aborted && exitCode !== 0 and mirror reportCancelledFile's error/count instead. Nit: run() is new, no vendored test aborts mid-file, and success is already correctly false — this is the third state (during a file) of the abort class 9ab4918 addressed for before/after, per REVIEW.md's 'Fix the whole class'.
Extended reasoning...
What the finding is
Commit 9ab4918 (responding to the unresolved comment at line 401) added reportCancelledFile() in runFiles() so that files not yet started when opts.signal aborts are reported with failureType: 'cancelledByParent' and counted in counts.cancelled. But the file that was running when the abort fired is not routed through that path: at src/js/node/test.ts:547-571, runOneFile() never consults opts.signal?.aborted after await proc.exited resolves:
await drainStderr;
const exitCode = await proc.exited;
const fileFailed = exitCode !== 0 && fileCounts.failed === 0;
...
if (reportFileNode) {
fileCounts.tests++;
if (fileFailed) fileCounts.failed++; // ← counted as a failure
...
}
if (fileFailed) {
error = makeTestFailure(stderrText.trim() || `Test file failed with exit code ${exitCode}`, "testCodeFailure");
// ^^^^^^^^^^^^^^^^^ raw bun-test stderr banner ^^^^^^^^^^^^^^^^^ wrong failureType
}So a mid-file abort is indistinguishable from a file that genuinely crashed with a top-level throw: same failureType, same count bucket, and the error message is whatever partial bun test reporter output reached stderr before SIGTERM.
Why this is the residual sibling of 9ab4918
An abort has three timing states relative to a file: before it starts, during it, and after it finishes. 9ab4918's reportCancelledFile loop in runFiles() iterates for (; i < files.length; i++) starting from the value of i after the aborted iteration has already completed and incremented — so it correctly handles before (unstarted files) and the loop-head if (opts.signal?.aborted) break handles after (nothing more to do). The during file itself falls through runOneFile's normal completion path and is never routed to reportCancelledFile. Per REVIEW.md's 'Fix the whole class in the same PR — grep for every sibling site sharing the pattern', this is the third member of the same class with only two of three matching Node.
The unresolved comment at line 401 asked for (a) success correctness and (b) cancelledByParent for each unstarted file — both now addressed. It mentioned the during-file case only as timing contrast ('aborting during a file lands as fileFailed'), not as an ask; this is the distinct residual.
Why this diverges from Node
Node's FileTest inherits Test.prototype.cancel() (lib/internal/test_runner/test.js), which its abort listener calls: it sets failureType: 'cancelledByParent', uses the fixed message 'test did not finish before its parent and was cancelled', and countCompletedTest() routes cancelled tests to counts.cancelled, not counts.failed. The interrupted file is thus distinguishable from a genuinely-crashed one in Node's event stream.
Step-by-step proof
Under run({ files: ['slow.js', 'b.js'], signal: ctrl.signal }) where slow.js contains test('t', () => new Promise(r => setTimeout(r, 60_000))), and ctrl.abort() is called from a test:dequeue listener (i.e., while slow.js is running):
runOneFile('slow.js', ...)callsBun.spawn({ signal: opts.signal })(line 476-483). The abort listener SIGTERMs the child.- stdout/stderr close; the
for awaitloops finish.await drainStderrresolves.const exitCode = await proc.exited(line 547) resolves nonzero (SIGTERM). - No test failures were reported before the kill →
fileCounts.failed === 0.fileFailed = exitCode !== 0 && fileCounts.failed === 0→true. reportedChildren === 0(no marker lines reached stdout) →reportFileNode = true→fileCounts.failed++(line 566).- Line 571:
error = makeTestFailure(stderrText.trim() || ..., 'testCodeFailure')—failureType: 'testCodeFailure', and the message is the entirebun teststderr banner captured so far (potentially kilobytes of reporter output). reporter.fail({..., details: { error }})emits the file-leveltest:failwith the wrong failureType.addRunCounts(counts, fileCounts)→counts.failed >= 1.- Back in
runFiles():i++→i = 1. Loop head seesopts.signal?.aborted→break. ThereportCancelledFileloop starts ati = 1and correctly reportsb.jsascancelledByParentwithcounts.cancelled++. - Run-level summary:
counts.failed = 1,counts.cancelled = 1. The interruptedslow.jsis indistinguishable from a file that genuinely crashed.
Node v26.3.0 on the same scenario: slow.js → test:fail with details.error.failureType === 'cancelledByParent', counts.cancelled = 2, counts.failed = 0.
Fix
After const exitCode = await proc.exited (line 547), branch on opts.signal?.aborted && exitCode !== 0. When true, use makeTestFailure('test did not finish before its parent and was cancelled', 'cancelledByParent') and increment fileCounts.cancelled instead of fileCounts.failed — mirroring reportCancelledFile at lines 428/440-441. The reportFileNode gate and reporter.fail emit are already in place; only the error object and count bucket need to change.
Impact / severity
Nit. run() is brand-new in this PR, no vendored test aborts mid-file, and the run-level success flag is already correctly false (via counts.failed > 0 → counts.failed === 0 && counts.cancelled === 0 is false). Only the failureType string, the error message content (raw stderr banner vs. Node's fixed message), and the count bucket (failed vs. cancelled) differ — event-shape/count fidelity, not verdict correctness.
…ere used (#38442) ### What does this PR do? Reverts the user-visible half of #34444's exit-listener change. Since that PR, `bun test` dispatched every test file's `process.on('exit')` listeners once the last file finished, so a listener calling `process.exit(1)` failed the run even though the summary reported everything passing (this was slated to be a documented breaking change in the 1.4 notes). Checked jest 30 and vitest 4.1 with a passing test whose file registers `process.on("exit", () => process.exit(1))`: | runner | listener ran | exit code | |---|---|---| | jest (workers and `-i`) | no | 0 | | vitest (`forks`, `threads`, `vmForks`, `vmThreads`) | no | 0 | | bun 1.4 canary before this PR | yes | 1 | jest gives test files a copy of `process`; vitest tears its workers down itself. Since `bun test` runs every file in one process, one file's listener also affected the whole run. Now the end-of-run `on_exit()` still runs (profilers, deferred flushes, cleanup hooks) but skips the listener dispatch unless one of these opted in: - a `node:test` API was called on the main thread — `test`/`describe`/hooks/`mock.*`/`assert.register` all reach `jsFileGeneration`, which marks the runner. Merely importing `node:test`, or a Worker using it, doesn't count. - `BUN_TEST_DRAIN_EVENT_LOOP=1`, the existing opt-in `scripts/runner.node.mjs` sets for vendored node tests (a few of those run under `bun test` without `node:test` and check `common.mustCall()` counts from an exit listener). Both keep the vendored node tests and `node:test`'s `run()` children working. A test that calls `process.exit()` itself still runs listeners, as before. `--parallel` workers now go through the same gated `on_exit()` instead of straight to `global_exit()`. `test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js` verified its own `mustCall()` wrappers from an exit listener; that check is now an `afterAll`. ### How did you verify your code works? - `test/cli/test/bun-test.test.ts` — replaced the two tests from #34444 with eleven covering: bun:test file, globals-style file, import-only `node:test`, and Worker-only `node:test` don't run listeners and exit 0 even when the listener calls `process.exit(1)`; explicit `process.exit(3)` from a test still runs them; a file registering a `node:test` test runs them and can fail the run; `BUN_TEST_DRAIN_EVENT_LOOP=1` runs them for a bun:test file; `--parallel` runs for both kinds. The "not run" cases fail on the current canary and pass with this branch; the Worker case was also checked against a build with the main-thread guard removed. - `util-inspect.test.js` passes, and fails when a `mustCall()` wrapper is never invoked. - `test/js/node/test_runner/node-test.test.ts` passes (45/45); the vendored `test-events-add-abort-listener`, `test-file-write-stream5`, `test-worker-arraybuffer-zerofill`, `test-runner-mocking` still pass with `BUN_TEST_DRAIN_EVENT_LOOP=1`. - `test/cli/test/parallel.test.ts`: 34/35 locally; the one failure (`unique JEST_WORKER_ID`) reproduces on this debug build without the worker change and passes on the release canary — debug-build timing, unrelated.
3 participants
Implements
node:test'srun()and raises the surroundingnode:testbehavior to Node v26.3.0. Stacked on #34443 (its first commit), which is landable on its own.Every behavior below was diffed against the real
node v26.3.0binary, not read off the source.run()runner.js:731-909), including the mutually-exclusive pairs:forceExit×watch,shard×watch,globPatterns×files,env×isolation:'none'.TestsStream: aReadablein objectMode with Node's buffering, emitting each message as both an event and a stream chunk.bun testchild, spawned withNODE_TEST_CONTEXT(Node's own variable — upstream tests branch on it to tell parent from child). The child streams one JSON event per line; the parent republishes, aggregates, and emits a per-file and a run-leveltest:summary.run()on itself doesn't fork forever.watch,coverage,shard,isolation:'none',globPatterns,globalSetupPath) throw rather than being silently ignored.Event fidelity added in this round, each verified against Node:
test:enqueue/test:dequeueup front,test:completeat the end, andtest:failwithfailureType: 'testCodeFailure'when the file itself dies (a top-level throw, a missing file). When the file's tests fail instead, the completion carriessubtestsFailedand notest:fail— which is what Node does.bun testdoesn't invoke those bodies, so nothing reported them.details.type, suites counted only insuites, andfailureTypepreserved across the child process boundary.expectFailure(xfail)Node v26's
expectFailurewas missing entirely. Adds the option parser (string label, function/RegExp validator, object form withlabel/match, and the empty-object rejection), the inverted verdict — a failing body is the expected outcome, a passing one fails withfailureType: 'expectedFailure'— and theexpectFailurefield on the reported event.Two Node divergences reachable from plain
bun testNot
run()-specific; these affect anyone usingnode:testunder bun today.{ skip: true, todo: true }was treated as a todo. Node checksskipfirst, for both tests and suites.Test-runner harness
A vendored test that only drives
run()is the parent of that run, not a test file — Node executes it as a plain script, and underbun testa file that registers no tests of its own exits before itsrun()finishes, delivering zero events. The runner now picksbun runfor those, gated so that a file with any unindented registration of its own keepsbun testrather than silently "passing" having tested nothing. 4 of the 88 vendorednode:testfiles change subcommand, all of them added by this PR.Coverage
Vendors 5 upstream tests —
test-runner-expect-error,test-runner-expect-error-but-pass,test-runner-todo-skip-tests,test-runner-filetest-location,test-runner-tags-experimental-warning— taking the upstreamtest_runnersuite from 20 to 26 of 81. bun's ownnode-test.test.tsgoes 40 → 44 (new fixtures coverexpectFailureand the suite-skip semantics). All 20 previously-vendoredtest-runner-*files still pass.Still draft: the reporting seam
A test whose failure is decided after its body resolves is still reported as passing by
run(). Repro:test/fixtures/test-runner/plan/timeout-basic.mjs— run directly underbun testit is correctly 1 pass / 1 fail (bun's plan logic is fine), but the JS layer has already returned by the time the native runner attributes the async throw, sorun()sees two passes.I confirmed the JS layer cannot close this: bun's
bun testintercepts in-process throws natively and attributes them itself, soprocess.on('uncaughtException')— the mechanism Node's ownharness.js:255uses — never fires for an in-test throw. A faithfulrun()needs a reporter hook in the native runner, emitting where bun actually decides pass/fail (Execution::on_sequence_completed,src/runtime/test_runner/Execution.rs, already the single fan-out point for the CLI reporter, the junit writer and the inspector agent).That's a maintainer design call, which is why this stays draft. Happy to implement it in this PR or a follow-up — it's also the unlock for
--test-style CLI emulation andnode:test/reporters, which together account for ~25 of the remaining 55 upstream test_runner files.Update (2026-07-17)
Three follow-up commits landed on this branch after review:
BUN_TEST_DRAIN_EVENT_LOOP=1, set by the vendored-test runner; also on for run() children): a node test process only exits when its loop drains, andcommon.mustCall()verifies counts in'exit'handlers. This fixes the five vendored tests that started failing once exit handlers ran (test-worker-arraybuffer-zerofill,test-file-write-stream5,test-tls-psk-alpn-callback-exception-handling, and with the two fixes below,test-events-add-abort-listenerandtest-net-server-async-dispose). Off by default — bun suites keep exit-after-tests.events.addAbortListenernow survivesstopImmediatePropagation: it registers a native abort algorithm (runs inrunAbortSteps()before event dispatch) instead of an'abort'listener, since the native EventTarget drops node's[kResistStopPropagation]option.node:eventshad a duplicate inline implementation bypassinginternal/abort_listener; it now delegates.Server[Symbol.asyncDispose]resolves when not listening (node'slib/net.jsguard); a second dispose used to reject withERR_SERVER_NOT_RUNNINGand re-emit'close'.Each verified byte-identical to the node v26.3.0 binary on minimal repros.
no test proof · iteration 26 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/test/bun-test.test.ts