node:test: cancel subtests still pending when their parent finishes - #39286
node:test: cancel subtests still pending when their parent finishes#39286robobun wants to merge 5 commits into
Conversation
A test used to drain its whole subtest chain after its body settled, so a t.test() whose await was forgotten was always waited for and passed. Node only waits until the first time every subtest created so far has finished (subtestsPromise); a subtest started after that and still unfinished once the test's own hooks have run fails with cancelledByParent and fails the parent (postRun). Track each node's unfinished subtests, wait for the first settle only, and cancel what is left in the postRun position: a subtest that has not reached its turn on the chain reports the cancellation instead of running, a running body has its stop promise rejected so nobody waits for it, and an inline suite cancels its own pending children. Because the chain starts a body a tick later than Node's synchronous start, stragglers get until the next macrotask before being cancelled, so a forgotten await on a subtest that needs no timer or I/O still completes, as it does in Node. Subtest failures are now rolled up after the after hooks, where Node does it, and a run() parent counts cancelledByParent results under `cancelled`.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 0 remain after this review. WalkthroughChangesThe Node test runner now tracks cancellation separately from failure, shares timeout budgets across test execution and hooks, settles unfinished subtests, propagates suite results, and reports consistent run events. New fixtures and tests cover cancellation ordering, hanging hooks, timeout bounds, and event summaries. Node test lifecycle
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
|
Status: reproduced and fixed, waiting on CI.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Cancellation cannot interrupt a before hook or describe() callback that never settles, so a subtest queued behind one kept the drain waiting until bun:test's watchdog instead of failing with the test's own timeout, which is what bounded the drain when it still ran inside the body race. Race the drain against a stop controller armed with the same timeout. Also trims the comments added by the previous commits.
…ted arm run() counted a timed out test as failed because the timeout error carried no failure type; node reports it as testTimeoutFailure and counts it as cancelled. The hook timeout is left untagged on purpose: node reports a hook timeout as a hook failure, which is a plain failure. Also covers a subtest stuck in its own after hook in the timeout fixture, and trims the remaining multi-line comments.
…e timeout Arming the drain with the full timeout again let a test whose chain is stuck spend up to twice its timeout, which past 2500ms loses to bun:test's own watchdog and reports the wrong error. The body phase's stop controller now reports how much of the budget is left (hooks in between stay exempt, as before) and the drain is armed with that, so the whole thing still fits in one timeout like it did when the drain ran inside the body race.
| if (node.cancelled) failure ??= cancelledByParentFailure(); | ||
|
|
||
| const bodyFailure = failure; | ||
| failure = applyExpectFailure(node, failure); | ||
| const acceptedXfail = bodyFailure !== undefined && failure === undefined; |
There was a problem hiding this comment.
🟡 An in-flight subtest cancelled by its parent has the cancelledByParent error fed through applyExpectFailure here, so a subtest with expectFailure (explicit or inherited from the parent, line 1604) reports test:pass and does not increment parent.failedSubtests — Node reports it as cancelled, and so does the queued path via finishCancelled() at line 753, which skips the xfail check. Repro: test('p', async t => { await t.test('a', () => {}); t.test('slow', { expectFailure: true }, () => new Promise(() => {})); }) — slow is accepted as an xfail and p passes when it should fail with 1 subtest failed. Guarding the xfail check on !node.cancelled (or excluding failure?.failureType === 'cancelledByParent') makes the two cancellation paths agree.
Extended reasoning...
What the bug is
This PR introduces two paths by which a parent cancels a pending subtest: a subtest still queued on subtestChain when its turn comes goes through finishCancelled() (line 753), and a subtest whose body is already running has its stop promise rejected with cancelledByParentFailure() (line 2474 in cancelUnfinishedSubtests). The first path sets passed = false directly and never consults expectFailure. The second path lands in executeTestNode at line 2381 (if (node.cancelled) failure ??= cancelledByParentFailure()), and the very next line feeds that failure into applyExpectFailure(node, failure).
applyExpectFailure with no validator (parseExpectFailure(true) returns {label: undefined, match: undefined}) accepts any non-undefined failure and returns undefined — so acceptedXfail = true, node.passed = true, the child emits test:pass via reportNodeToRunParent, and executeTestNode returns undefined. Back in scheduleSubtest, failure === undefined means parent.failedSubtests is not incremented and the parent may pass. The same applies to a subtest cancelled during its beforeEach hooks: line 2332's !node.cancelled gate skips the body block, line 2381 sets failure, and line 2384 accepts it.
Why existing code doesn't prevent it
The comment at line 2380 ("an earlier failure wins") and the ??= are about not overwriting a body error with the cancellation — they don't address the interaction with expectFailure. Nothing between line 2381 and line 2384 checks node.cancelled or the failure's failureType. applyExpectFailure itself has no cancellation carve-out. And because expectFailure is inherited from the parent (line 1604: parseExpectFailure(options.expectFailure) || parent?.expectFailure), a user doesn't even have to put the option on the stale subtest to hit this — an xfail parent that forgets an await propagates it.
Step-by-step proof
test('p', async t => {
await t.test('a', () => {});
t.test('slow', { expectFailure: true }, () => new Promise(() => {}));
});acompletes and resolvesp.subtestsSettled(first settle).slowis chained behindaand starts on the next microtask; its body awaits a never-settling promise.p's body returns;await p.subtestsSettled?.promiseis already resolved, so the body race completes withfailure === undefined.p's afterEach/after hooks run, thensettleSubtests(p):unfinishedSubtests = {slow}, onesetImmediategrace, thencancelUnfinishedSubtests(p)setsslow.cancelled = trueand callsslow.stop.reject(cancelledByParentFailure()).- In
slow'sexecuteTestNode: the body race rejects,failure = cancelledByParent. Line 2381 is a no-op (failurealready set). Line 2384:applyExpectFailure(slow, failure)—slow.expectFailure = {label: undefined, match: undefined}, so it returnsundefined.acceptedXfail = true,slow.passed = true,slow.error = cancelledByParent. reportNodeToRunParent(slow, ...)emitstest:passwithexpectFailure: true.executeTestNodereturnsundefined.scheduleSubtestseesfailure === undefinedand does not touchp.failedSubtests.- Back in
p'ssettleSubtests, the drain completes.p.failedSubtests === 0, soppasses.
Contrast with the queued path: insert a third subtest t.test('queued', { expectFailure: true }, () => {}) after slow. It's still on subtestChain when cancelled, so its link runs finishCancelled(queued): passed = false, test:fail with failureType: 'cancelledByParent', and p.failedSubtests++. Same option, opposite verdict, decided only by whether the body had started.
Node v26.3.0 reports slow as test:fail / cancelledByParent and fails p with 1 subtest failed. Node's #cancel() calls this.fail() (which sets endTime) before run()'s catch block observes the abort, so the subsequent this.pass() from the xfail branch is a no-op — Node never lets expectFailure swallow a cancellation.
Impact
A false pass in a Node-compat test runner: an { expectFailure: true } subtest whose await was forgotten (the exact bug this PR exists to catch) reports as an accepted xfail, and its parent may pass. Under run(), it lands in counts.passed instead of counts.cancelled. The trigger is narrow — expectFailure + not-first-batch + forgotten await — so this is a nit rather than blocking, but per REVIEW.md "Fix the whole class in the same PR" the two cancellation paths introduced here should agree.
Fix
Skip the xfail check when the node was cancelled, matching finishCancelled:
const bodyFailure = failure;
if (!node.cancelled) failure = applyExpectFailure(node, failure);
const acceptedXfail = bodyFailure !== undefined && failure === undefined;(or equivalently exclude failureType === 'cancelledByParent' inside applyExpectFailure). None of the fixtures cover expectFailure × cancellation, so a case in fixture 30 would lock this in.
There was a problem hiding this comment.
The inconsistency between the two paths is real, but node resolves it the other way around. #cancel() goes through fail(), and fail() is where expectFailure is applied, so in node a cancelled expectFailure subtest is an accepted expected failure on both paths. Checked on v26.3.0 with TAP output:
test("p", async t => {
await t.test("first", () => {});
t.test("running xfail", { expectFailure: true }, () => new Promise(() => {}));
t.test("queued xfail", { expectFailure: true }, () => {});
});
ok 1 - first
ok 2 - running xfail # EXPECTED FAILURE
ok 3 - queued xfail # EXPECTED FAILURE
ok 1 - p
The same holds when the option is inherited from an expectFailure parent. So the in-flight path here (cancellation fed through applyExpectFailure()) already matches node; it is the queued path, finishCancelled(), that does not, since it reports the cancellation as a failure regardless. I will make finishCancelled() apply expectFailure as well and add this case to fixture 30 (it passes under node as shown above, and it fails on the current revision because the queued one is counted against the parent).
2 participants
Problem
awaitwas forgotten is always waited for bybun test, so this file reports2 pass, 0 failand exits 0;node --test(v26.3.0) reportsslow subtestas'test did not finish before its parent and was cancelled'(failureType: 'cancelledByParent'), fails the parent with1 subtest failed, and exits 1:executeTestNode()insrc/js/node/test.tsrandrainSubtestChain(node)after the body, i.e. it waited for every subtest ever scheduled. Node'sTest#runwaits onsubtestsPromise, which is created with the first subtest and resolved the first time no subtest is left unfinished, and is never re-armed;postRun()then cancels whatever is still unfinished. So the earlier awaitedt.test()is what arms the check: a sync parent, or an async parent whose only unawaited subtest is the first one, is waited for in Node too (verified, and covered by the PASS cases in the fixture).Fix
TestNodetracksunfinishedSubtestsandsubtestsSettled(Node'sunfinishedSubtests/subtestsPromise). Every inline subtest (test, inline suite, and skip directive, which Node also counts toward the first settle) is scheduled through onechainSubtest()that maintains them. After the body (and after aplan({ wait })wait) the test awaitssubtestsSettledinstead of draining the chain.settleSubtests()is the subtest half of Node'spostRun(), and runs where Node runs it, after the test's own afterEach/after hooks: whatever is still unfinished is cancelled and the chain is drained only so the cancellations get recorded.cancelUnfinishedSubtests()marks the subtree; a subtest that has not reached its turn on the chain reports the cancellation instead of running (finishCancelled(), no hooks or body), a running body has its stop promise rejected so the parent stops waiting for it (Node aborts the subtest's signal, which rejects itsstopPromise), and a cancelled inline suite skips its hooks, drains its already cancelled children, and reportscancelledByParentitself, as in Node. The stop controller therefore exists for every run, not only when a timeout is set.describe()callback that never settles cannot be cancelled, and neither can a subtest that is only waiting on its own after hooks (its result is already in and is kept). The drain is therefore raced against a second stop controller armed with whatever the body phase left of the test'stimeout(the hooks in between stay exempt), which is the bound the drain had before when it ran inside the body race: such a test still fails with its owntest timed out after Nms, within one timeout, instead of waiting for bun:test's watchdog (fixtures/31-timeout-bounds-hanging-hook.js). That timeout error now carriesfailureType: 'testTimeoutFailure'like Node's; the hook timeout does not, because Node reports a hook timeout as a hook failure.t.test(), so a forgotten await on a subtest that finishes on microtasks alone (a sync body) is already complete when Node'spostRun()runs; bun's chain starts it a tick later.settleSubtests()yields onesetImmediatebefore cancelling when something is still pending, so those complete here too, while anything waiting on a timer or I/O is cancelled like in Node. Known remaining difference, on the lenient side only: a straggler that needs exactly onesetImmediate(or a handful of microtasks more than Node's own continuation) completes here and is cancelled by Node; nothing Node passes is cancelled here.t.passed === truewhen only a subtest failed (previously false), and anexpectFailureparent is no longer satisfied by a failing subtest alone.run(): a child'scancelledByParentortestTimeoutFailureresult is counted undercounts.cancelled(Node's runner keys this off the failure type as well,kCanceledTests;testAbortedis left out becauset.signalnever aborts in this shim), and the per-file summary treats cancellations as failures. Suites report through the samereportNodeToRunParent()as tests, so a cancelled suite carries the cancellation error.test/js/node/test_runner/fixtures/30-cancelled-subtests.js(4 cancellation cases that must fail, 8 waiting cases that must pass) via two new tests intest/js/node/test_runner/node-test.test.ts:bun testcounts/markers, and therun()event stream and counts.node --testv26.3.0 fails and passes exactly the same tests of the fixture and reports the same eight cancellations in the same order. Two more tests runfixtures/31-timeout-bounds-hanging-hook.jsunderbun test(every test ends with its own timeout error) and underrun()(the timeouts land incounts.cancelled). Without the fix the cancellation cases of fixture 30 hang until the 5s bun:test timeout and its two tests fail; with itnode-test.test.tspasses 49/49, and the portedtest/js/node/test/parallel/test-runner-*files are unchanged (24 pass;test-runner-mock-timers-scheduler.jsfails its 100ms wall-clock bound under the debug build with or without this change).t.test()is called after the parent already finished; this PR is about subtests that exist but are unfinished when the parent finishes. The two touch different paths ofaddTest().postRun()sweep for the case where the parent is stopped by its timeout orsignal, plus the actual aborting oft.signal, and also changescreateStopController()and adds fixtures numbered 30 and 31, so the two conflict textually and one of them has to be rebased onto the other. The sweep here already covers the timeout trigger (a parent whose body race times out cancels its pending subtests the same way; fixture 31 exercises it), so if this lands first, node:test: abort t.signal and enforce the test-level signal option #39287 reduces to owning theAbortControlleronTestNode, aborting it fromcancelUnfinishedSubtests()and at the end ofexecuteTestNode(), and adding thesignaloption to the stop controller. If node:test: abort t.signal and enforce the test-level signal option #39287 lands first, the first-settle tracking, the grace hop, the bounded drain and therun()counting from here get rebased onto itscancel()/cancelSubtests()instead; either order works, and I will do the rebase of whichever one is second.Background
test()calls register bun:test tests; at.test()inside a running test is an inline subtest, run by the shim itself. Inline subtests of one parent are serialized throughsubtestChain, a promise chain each subtest appends a link to;drainSubtestChain()waits until that chain goes idle. A parent's result underbun testis its own body outcome plusN subtests failedwhen any subtest failed.executeTestNode()runs one test: inherited beforeEach hooks, the body raced against a stop controller (one promise that rejects on the test's timeout, now also when the parent cancels the test), the plan check, the verdict, afterEach/after hooks, and now the postRun step. Node'spostRun()is the step after a test's hooks where it cancels unfinished subtests, counts failed ones, and reports.run()is node:test's programmatic runner: it spawnsbun testper file withNODE_TEST_CONTEXTset, the child prints one JSON event per finished test (reportNodeToRunParent()), and the parent rebuilds Node's event stream and counters from them (republishChildEvent()).Node v26.3.0 probes behind the design
The debug build with this change gives the same verdicts for all of the above except the two "cancelled" microtask/setImmediate lines, which pass (the lenient difference described under Fix).