node:test: report t.test() after parent finished as a parentAlreadyFinished failure - #34583
node:test: report t.test() after parent finished as a parentAlreadyFinished failure#34583robobun wants to merge 10 commits into
Conversation
…nished failure When t.test() is called after its parent test already finished (the typical forgot-to-await shape), Node runs the body, resolves the returned promise with undefined, and records the late subtest as a failed test with failureType parentAlreadyFinished so the run exits 1. Bun previously resolved the promise but dropped the late subtest entirely, so a suite that Node fails red would pass green. addTest()/addSuite() now route the late subtest through runLateSubtest(), which runs the body, discards its outcome, and calls a new native hook (js_node_test_report_late_failure) to print a '(fail) <name>' line with the error and bump the reporter's fail counter so the run exits 1. Late skip/todo subtests are left alone (Node does not count those as failures either).
WalkthroughChangesLate Late subtest failure handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
|
Updated 7:42 AM PT - Jul 18th, 2026
❌ @robobun, your commit b723491 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34583That installs a local version of the PR into your bun-34583 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… cover late describe
Route the late-failure status line through worker_emit_test_done when
running as a --parallel worker, matching handle_test_completed's sibling
path instead of writing to stderr unconditionally.
Pass a no-op done callback when a late subtest body declares two
parameters so (t, done) => { done(); ... } does not throw on undefined().
Extend fixture 25 to cover saved.describe('late-suite', ...) (the
isSuite=true branch of runLateSubtest) and the (t, done) body shape.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/node/test_runner/node-test.test.ts`:
- Around line 258-265: Update the late-subtest assertions in the node test
runner test to require the `late-done` reporting variant, while explicitly
asserting that `late-skip` and `late-todo` are absent from stderr. Keep the
existing checks for `late`, `late-suite`, the parent-finished message, and the
expected failure count.
🪄 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: fe1b138e-938c-491a-8c48-bcbfdb575199
📒 Files selected for processing (5)
src/js/node/test.tssrc/runtime/test_runner/jest.rstest/js/node/test_runner/fixtures/16-plan-and-late-subtest.jstest/js/node/test_runner/fixtures/25-late-subtest-failure.jstest/js/node/test_runner/node-test.test.ts
The two existing serial summary().fail increments use an == bail check, so an increment that skips the check lets fail step past bail without it ever firing. Mirror the check here so a late subtest that is the Nth failure under --bail=N terminates the run like any other failure. Also note the intentional omission of a JUnit entry: maybe_print_junit_line needs a bun:test sequence/entry that a late subtest does not have (same as unhandled_errors_between_tests).
…ock tracker
Node's Test constructor replaces a {skip:true} body with a noop, so a
late {skip:true} subtest's body never runs; pre-PR Bun also never ran it.
Gate the body invocation on !child.skipped ({todo:true} bodies still run,
matching Node).
Reset child.mockTracker after the body settles, mirroring executeTestNode,
so a late body's t.mock.method() does not leak into later tests.
…t() runs as a subtest With child.finished set before the body ran, a t.test() inside the late body saw its own parent as finished and recursively fired a second spurious parentAlreadyFinished failure. Node runs the nested call as an ordinary subtest of the late test. child.finished is now set in settle(), and drainSubtestChain(child) runs first so nested subtests the body scheduled are awaited before the outer promise resolves. isExecutionPhase=true already makes isRunning() true for the duration, so the nested call falls through to scheduleSubtest.
There was a problem hiding this comment.
No new issues found, but I'd like a human to look at this one — it adds a second body-running path in src/js/node/test.ts that intentionally diverges from executeTestNode (timeout race and after-hooks deferred to a follow-up per the thread), plus a new native hook in jest.rs that directly mutates CommandLineReporter state under unsafe and mirrors the --bail exit path.
What was reviewed:
runLateSubtestlifecycle vs.executeTestNode: nested-subtest routing (child.finisheddeferred tosettle),{skip:true}body gate,mockTracker.reset()— all now match Node'spostRun.js_node_test_report_late_failure:--bailmirror, worker-IPC branch,failures_to_repeat_bufgating on!dots && !only_failuresmatcheshandle_test_completed.- Fixture 16's
{skip:true}addition keeps its exit-0 contract; fixture 25 pins body-ran/skip/todo variants and the3 failcount.
Extended reasoning...
Overview
Fixes a Node-compat gap where t.test() called after its parent finished was silently dropped (Bun exited 0; Node exits 1 with parentAlreadyFinished). Touches src/js/node/test.ts (new runLateSubtest path wired into addTest/addSuite, ~50 lines), src/runtime/test_runner/jest.rs (new js_node_test_report_late_failure native hook, ~60 lines with unsafe reporter mutation, worker-IPC branch, and a mirrored --bail exit), one existing fixture tweak, one new fixture, and new assertions in node-test.test.ts.
Security risks
None identified. No untrusted-input parsing; the only new surface is a $newRustFunction binding reachable from the built-in node:test module. The unsafe blocks follow the same buntest_as_mut / reporter-pointer provenance pattern as the adjacent js_node_test_mark_result.
Level of scrutiny
Medium-high. This is Node-compat runtime code (not sandbox), and the Rust side adds a third site that increments summary().fail and can Global::exit(1). The JS side adds a second body-running path that partially replicates executeTestNode's lifecycle — the kind of parallel-implementation shape REVIEW.md flags ("parameterize the existing path rather than cloning a parallel branch"). The author acknowledged this and deferred routing through executeTestNode to a follow-up; a maintainer should confirm that tradeoff is acceptable.
Other factors
The PR went through seven rounds of review-driven fixes (done-callback, worker-IPC, --bail mirror, {skip:true} body gate, mockTracker.reset(), nested-subtest recursion, plus test-assertion tightening), all resolved. One raised item — honoring {timeout: N} on a late body — was intentionally left unaddressed with a stated rationale (triple edge case, no false-green, bounded by bun:test's watchdog). All 41 tests in node-test.test.ts pass on both debug/ASAN and release per the evidence block. Given the accumulated surface and the deferred-refactor decision, this is past the "simple and self-contained" bar for auto-approval.
There was a problem hiding this comment.
No further findings after the earlier rounds — all six prior threads are addressed (or explicitly deferred). Leaving the final call to a human given the new unsafe reporter-write hook in jest.rs and runLateSubtest being a second body-execution path parallel to executeTestNode.
Checked: summary().fail sibling side-effects (worker-IPC, --bail mirrored; JUnit omission commented with the unhandled_errors_between_tests precedent); runLateSubtest vs executeTestNode parity — skip-body gating, mockTracker.reset(), nested t.test() via deferred child.finished, done-callback shape; fail glyph now via write_test_status_line. The {timeout: N} race on a never-settling late body was intentionally left for a follow-up (no false-green).
Extended reasoning...
Overview
Node-compat fix for node:test: when t.test() is called after its parent finished, book a parentAlreadyFinished failure so the run exits 1 (previously silently exited 0). Touches src/js/node/test.ts (new runLateSubtest path wired into addTest/addSuite), src/runtime/test_runner/jest.rs (new js_node_test_report_late_failure native hook that writes a fail line, bumps summary().fail, and mirrors the --bail exit), fixture 16 (adjusted to keep exit 0), a new fixture 25, and one new case in node-test.test.ts.
Security risks
None identified. The new native hook is only reachable via $newRustFunction from the bundled node:test builtin, takes a name string and an error value, and writes to the test reporter. No untrusted-input parsing, no filesystem/network surface.
Level of scrutiny
Medium-high. This is not a mechanical change: it adds a new unsafe { &mut *rep.as_ptr() } write path into CommandLineReporter from a JS-reachable hook, and runLateSubtest is a second body-running path alongside executeTestNode — the six earlier review rounds each surfaced a sibling invariant it was missing (worker-IPC, --bail, {skip:true} body, mockTracker.reset(), nested-subtest child.finished ordering, glyph duplication). All were addressed, and one (per-late-subtest {timeout: N} race) was reasonably deferred with a stated follow-up plan to route through executeTestNode. That architectural question — parallel path now vs. reuse executeTestNode — is worth a human's eye.
Other factors
CI evidence in the PR body shows the new test failing on main (both ASAN debug and release-canary) and passing with the fix. Fixture 25 pins body-runs, promise-resolves-undefined, done-callback, late-suite, late-skip body not run, late-todo body run, 3 fail / 2 pass, and exit 1; fixture 16 was updated so its own exit code stays 0. All prior inline threads (mine and coderabbit's) are marked resolved.
|
Diff is green:
Neither touches |
1 participant
When
t.test()is called after its parent test already finished (the typical forgot-to-await shape), Node runs the body, resolves the returned promise withundefined, and records the late subtest as a failed test withfailureType: 'parentAlreadyFinished'so the run exits 1. Bun resolved the promise but dropped the late subtest entirely, so a suite that Node fails red would pass green.Repro
Node v26.3.0:
LOG:late-body-ran,✖ late subtestwithtest could not be started because its parent finished, pass 2 / fail 1, exit 1.Bun (before):
LOG:await=resolved, body never runs,2 pass / 0 fail, exit 0.Cause
addTest()insrc/js/node/test.tsreturnedPromise.resolve(undefined)whenrunningNode.finishedwithout booking the failure into the run results. The adjacent comment noted Node fails the late subtest but nothing recorded it.Fix
addTest()/addSuite()now route the late subtest throughrunLateSubtest(), which runs the body (as Node does), discards its outcome, and calls a new native hookjs_node_test_report_late_failureinjest.rsto print a(fail) <name>line with the error and bump the reporter's fail counter so the run exits 1. Lateskip/todosubtests are left alone (Node exits 0 for those too).Verification
Bun (after):
New
test/js/node/test_runner/fixtures/25-late-subtest-failure.jscovers body-runs + promise-resolves + fail-count + exit-1. All 41 cases innode-test.test.tspass.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 1
evidence per changed file