Skip to content

bun test: keep the result of a callback that throws after its timeout - #47867

Open
robobun wants to merge 3 commits into
mainfrom
robobun/2703fc0a/late-throw-keeps-verdict
Open

bun test: keep the result of a callback that throws after its timeout#47867
robobun wants to merge 3 commits into
mainfrom
robobun/2703fc0a/late-throw-keeps-verdict

Conversation

@robobun

@robobun robobun commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Problem

  • A test or hook that throws after its timeout is reported as timed out. This happens when it ends without a yield to the event loop. test.failing fails, and JUnit loses the thrown error (report, ruling).
  • The cause: step_sequence_one (src/runtime/test_runner/Execution.rs) calls evaluate_timeout after every callback that ended inside run_test_callback.

Fix

  • run_test_callback returns how the callback ended: Running, Returned or Failed. The runner checks the deadline only for Returned.
  • Two cases keep the timeout. Bun killed a process for it (Execution.killed_on_timeout_at). Or a hook fails after a test.failing body failed as expected.
  • Verified: test/js/bun/test/test-test.test.ts, vitest-flavor.test.ts (13 rows equal to Vitest 5.0.5), node-test.test.ts. Also 17 other runner suites.
  • Self-reviewed: 19 concerns raised, 18 addressed. Not done: the hook bug in Downsides.

Background

  • An entry is one test or hook callback with a deadline. The timer of a file cannot run while a callback blocks.
  • At a deadline bun kills the processes that the test spawned.
  • Considered a guard on sequence.result or maybe_skip. Both also drop the timeout of a late return after a stray rejection.

Downsides

  • A hook error cannot fail a test.failing whose body threw (also on main). So a late throw of the body, then a hook error, now passes.
  • A callback that waits in Bun.build() past its timeout, then throws, loses timed out if bun killed nothing.
  • BunTest grows from 440 to 456 bytes per file. Release .text does not change.
Notes

Rows that change, main (3bf1a06) to this PR. Each callback passes its timeout inside one call and does not yield first.

Callback main this PR Other runners
test throws error, timed out error Vitest 5.0.5, Jest 30.5.2, Node 26.3.0: error
async test rejects after await 0, or returns a rejected promise error, timed out error Vitest, Jest: error
returns an object whose then getter throws error, timed out error Jest: error
done(error) after the timeout, also from a microtask or an async callback error, timed out ... before its done callback was called error Jest: error (callback without async)
throws after done() error, timed out error Jest: error
test.failing throws fail pass Jest: pass
test.failing with retry throws fail at attempt 2 pass
test.todo throws, --todo fail todo
hook throws (beforeAll, beforeEach, afterEach, afterAll, onTestFinished) error, hook timed out error Vitest, Jest: error
skip() of the test context (vitest) fail skip Vitest: skip
afterEach throws after skip() (vitest) fail skip Vitest: skip
aroundEach throws in set-up or teardown error and an AroundHook...Error timeout error Vitest: error
aroundEach set-up returns without runTest() "not called" error and a set-up timeout "not called" error Vitest: the same error only
test.extend fixture returns without use() error, hook timed out error Vitest: its error only
signal.aborted after a throw (vitest) true false Vitest: false
node:test body throws error, timed out error Node: error
node:test expectFailure body returns error, timed out error Node: the same error
waits in Bun.build() for an async plugin past its timeout, then throws, and bun killed nothing error, timed out error
test.failing or test.todo body throws, then a hook throws in time or an aroundEach teardown times out fail, timed out pass or todo, the hook error is printed Jest: fail (test.failing with afterEach)

JUnit for the first row: <failure type="TimeoutError" message="test timed out" /> on main, <failure type="Error" message="thrown by the test"> with this PR.

What follows from a changed result also changes: retry and repeats runs, --bail, onTestFailed, and task.result.state. Each of these rows prints what the same callback prints when it fails in time.

Rows that do not change.

Callback main and this PR Other runners
test returns, resolves, or calls done() timed out Vitest: timed out. Jest, Node: pass
returns after expect.soft() failed, after a stray rejection, or after a matcher that it did not await failed error, timed out Vitest: the same for expect.soft()
afterEach returns late after a test that threw in time error, hook timed out Vitest: the same
done(error) in time, then a late return, also test.failing error, timed out Jest: error, and test.failing passes
a hook calls done(error) error, hook timed out
afterEach or onTestFinished throws after the body of a test.failing or test.todo failed as expected in time fail, hook timed out Jest: fail
a bun:test hook throws after a node:test body called t.skip() fail, hook timed out
throws after Bun.spawnSync outlived the timeout, also test.failing and test.concurrent timed out
Bun.spawnSync itself throws after bun killed its child timed out
throws after bun killed a process that it spawned, while it waited in Bun.build() timed out
node:test body returns timed out Node: pass
node:test expectFailure body throws fail, timed out Node: pass
node:test t.skip() fail, timed out Node: skip

The end kind is that of the function the runner calls. For an around set-up, a test.extend fixture and a node:test test, that function is a wrapper of bun. When the wrapper throws for a user function that returned (no runTest(), no use(), expectFailure that passed, a plan mismatch, a t.after() that throws), the end is Failed. When the wrapper accepts a throw and calls done() (expectFailure), the end is Returned.

done(error). It ends the callback as Failed only when the call comes after the deadline of the test (DoneCallback.failed_late). A test that calls done(error) in time and returns late is a late return, so the runner reports the timeout, as on main. The first push kept only the error there. Review pointed out that this is a late return, and the second commit restores the timeout.

The mark for a killed process. killed_on_timeout_at is a time. The runner compares it with the deadline of the entry, so a kill for an earlier entry does not count. Two places write it, because two places act: kill_dangling_processes_on_timeout (only in a group with one sequence, when it killed a process) and the wait loop of Bun.spawnSync (it ends its own wait, also in a concurrent group). A mark each time the timer ran was tried first. With it one test.failing passed in 11 of 40 runs, by its position in the file.

The clause for hooks. handle_uncaught_exception writes the failure of a hook only over Pending. After the body of a test.failing stored Pass, a test.todo body stored Todo, or t.skip() of node:test stored Skip, the error of a hook is printed and the test does not fail. On main the timeout was the only thing that failed such a test when the hook was late. result_survives_hook_error keeps that. Delete it when the hook arm is fixed. skip() of a test context hides later errors on purpose, so that case follows the thrown error, as Vitest does.

Left alone on purpose.

  • The hook arm of handle_uncaught_exception. Its fix also changes the row where body and hook both throw in time (pass to fail, Jest fails it).
  • node:test: a late t.skip() or t.todo(), and an expectFailure body that throws late, stay timed out. The shim ends those tests with done().
  • done(error) in a hook is not attributed to the hook on main. It stays "Unhandled error between tests" plus the hook timeout. A before() or after() of node:test on a suite or a file is such a hook (bun test: fail the test or hook whose done() received an error #39112).
  • skip() that the test catches, then a late return: timed out, as on main. Vitest 5.0.5 reports skipped.
  • An around hook phase that returns late keeps its AroundHook...Error. Vitest 5.0.5 passes it.
  • Bun kills only what it tracks, and only when the timer of the file runs for the deadline of the callback on the stack. That timer only moves to an earlier deadline, and bun does not set it again under a running callback (bun test: on a timeout, kill only the processes the timed-out test or hook callback spawned #45081 works on it). So after an earlier test with an earlier deadline, a callback that waits in Bun.build() and then throws keeps its error and its child. The same holds for a Bun.$ child, a process in a concurrent group, and a Bun.spawnSync whose own timeout option ended the wait first.
  • After an await in a concurrent group, Bun.spawnSync watches the latest deadline of the group (get_active_timeout). Bun then kills nothing at the deadline of this test, so a throw after the wait keeps its error. Without the await, bun kills the child at the deadline of the test and the timeout stands.
  • kill_dangling_processes_on_timeout treats an entry without a timeout as past its deadline (fix(test): preserve unlimited test children after stale deadlines #44613 fixes that line). The mark that this PR writes there has no effect for such an entry, because evaluate_timeout ignores it.
  • The completion path of a callback that yielded (Execution::step) has no deadline check on main. That is the second part of bun test: setDefaultTimeout() in a preload applies only to the first file, and a sync block after a timer escapes the timeout #45192, and bun test: time out a test or hook whose callback completes after its deadline #39440 adds the check. A returned thenable that is not a promise ends on that path.

Related open PRs. This diff conflicts in source with two open PRs that merge onto main. For each, the PR that lands second has to keep both changes.

A local merge of this PR with each open PR that shares a file with it found these two. The heads: main 675ee5b, this PR d927955, #45792 027d3b8, #47731 da299de.

#47730, #47760 and #47800 edit the call in run_test_callback whose result this PR classifies. They merge with this diff without a conflict. #39112 edits lines near this diff and does not merge onto main now. #47760 calls then() of a returned thenable before the microtask drain, so a thenable that settles at once then reaches this check. This PR has rows for thenables that reject or throw. #38153 (a test.todo body that times out) and #41865 (test.failing and unhandled errors) change how those modes store a result.

Measurements (linux-x64, main 3bf1a06 against this change on the same base).

  • Struct bytes (gdb, debug): Execution 72 to 88, and BunTest, which contains it, 440 to 456. DoneCallback 56 to 56, the new Cell<bool> is in former padding. ExecutionSequence 208 and ExecutionEntry 144 do not change. CallbackEnd is 1 byte and replaces a 40-byte Option<RefDataValue> return.
  • Clock reads on the main thread (LD_PRELOAD clock_gettime counter, debug builds): one more read for each done(error) that fails the running test. The other counts are equal on main and this PR. 250 and 1250 tests that pass: 1080 and 5084 (4.004 per test). 250 and 1250 tests that throw in time: 1081 and 5087 (4.006 per test). 10 and 20 tests whose Bun.spawnSync child outlives a 100 ms timeout: 183 and 292 (10.9 per test).
  • Instructions (llvm-objdump, release, step_sequence): 28 to 31 from the return of run_test_callback to the call of advance_sequence, for a callback that returned in time. Function totals: run_test_callback 568 to 560, step_sequence 1885 to 1908.
  • Release .text (llvm-size -A): 110,642,729 to 110,642,729 bytes. Seven symbols change size, net +95 bytes: Bun__Bindgen__BunTest__bunTestDoneCallback 1058 to 1164, BunTest::run 12402 to 12310, execution::step_sequence 8413 to 8499, TestCommand::run 15692 to 15642, Execution::kill_dangling_processes_on_timeout 409 to 441, spawn_maybe_sync 24184 to 24200, BunTest::run_test_callback 2426 to 2423.
  • Lines with timed out per fixture, main to this PR: late throws 21 to 0, late returns 16 to 16, killed process 6 to 6, vitest rows 13 to 3, node:test 5 to 3. Result lines that change: the test.failing, test.todo and skip() rows of the first table. No other result line changes.

Tests.

  • test-test.test.ts, three fixtures. "keeps its own result when it threw" (27 tests) fails on main. "is timed out when it returned, or when its error could not fail the test" (16 tests) and "is timed out when bun killed a process for its timeout before it threw" (6 tests) pass on main and with this PR. They fail with a guard on sequence.result or maybe_skip, without the clause for hooks, without the deadline test for done(error), or without either write of killed_on_timeout_at.
  • vitest-flavor.test.ts: 13 rows with import from "vitest". The fixture gives the same results on Vitest 5.0.5.
  • node-test.test.ts: five rows for node:test. The last one fails without the Skip arm of the clause for hooks.
  • All three files fail with the sources of main (28 pass 1 fail, 191 pass 1 fail, 45 pass 1 fail on the debug build) and pass with this change.
  • A search for unlisted differences ran about 1,500 more fixtures on release builds of main and of this change. It found the t.skip() row, which the clause for hooks now covers, and the around-hook form of the hook bug in Downsides.

Suites run on the debug build: test/js/bun/test/{test-test,vitest-flavor,vitest-apis,expect-deferred,bun_test,done-async,concurrent,concurrent_immediate,test-retry-repeats-basic,test-on-test-finished,failure-skip,jest-hooks,describe,test-failing,test-error-code-done-callback}.test.ts, test/js/node/test_runner/node-test.test.ts, test/cli/test/{bun-test,test-timeout-behavior}.test.ts, test/js/bun/spawn/spawnSync.test.ts, test/regression/issue/23865.test.ts.

History. The check after a synchronous completion came with #22928 in 1.2.23: b537fee added it, de90183 removed it, c4d565b restored it. 1.2.22 reports the thrown error. A callback that returns late is timed out on purpose, and stays so.

no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/test/test-test.test.ts

@robobun
bun test: keep the result of a callback that throws after its timeout
186c1a3 
A test or hook callback that blocked past its timeout and then threw was
reported as timed out when it ended without a yield to the event loop.
`test.failing` failed, and JUnit lost the thrown error.

`BunTest::run_test_callback` now returns how the callback ended:
`Running`, `Returned` or `Failed`. `Failed` is a throw, a rejection, or
a test that failed through `done(error)`. `step_sequence_one` checks the
deadline only after a callback that returned.

A callback that failed still gets the deadline check in two cases:
- bun killed a process, or ended a `Bun.spawnSync` wait, because the
  timeout passed, at or after the deadline of the callback.
  `Execution.killed_on_timeout_at` records that where
  `kill_dangling_processes_on_timeout` and `Bun.spawnSync` act.
- it is a hook whose error cannot fail the test: the body of a
  `test.failing` or `test.todo` already failed as expected, or a
  `node:test` body called `t.skip()`. The timeout is then the only
  failure.
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The test runner now tracks whether callbacks are running, returned, or failed. It uses callback outcomes and timeout-related process-kill timing when evaluating deadlines. Regression tests cover late callbacks and hooks in Bun, Vitest-flavor, and node:test.

Changes

Test timeout handling

Layer / File(s) Summary
Represent callback completion outcomes
src/runtime/test_runner/bun_test.rs, src/runtime/test_runner/DoneCallback.rs, src/runtime/test_runner/Collection.rs		 run_test_callback now returns CallbackEnd values for running, returned, and failed callbacks. The runner records late done(error) failures and queues completed collection callbacks based on their outcome.
Evaluate deadlines after callback and process events
src/runtime/test_runner/Execution.rs, src/runtime/api/bun/js_bun_spawn_bindings.rs		 Execution records timeout-related process-kill time and uses it with callback outcomes and hook-error results when checking deadlines. ExecutionEntry::is_past_deadline checks non-epoch deadlines.
Test late callbacks across runner modes
test/js/bun/test/test-test.test.ts, test/js/bun/test/vitest-flavor.test.ts, test/js/node/test_runner/fixtures/30-late-throw-without-yield.js, test/js/node/test_runner/node-test.test.ts		 Regression tests cover callbacks and hooks that exceed their deadlines, including timeout behavior during process kills and Bun.spawnSync(). They also check reported results, output, and exit codes.

Possibly related issues

  • oven-sh/bun#45192 — The change checks callbacks that return after their deadline, addressing the issue’s synchronous-blocking-after-await behavior. It does not change setDefaultTimeout behavior.

Suggested reviewers: jarred-sumner, alii, dylan-conway

Merge Risk: Minimal · up to d9279

No actionable merge-blocking issue is established for the callback timeout changes. Merge after normal checks pass.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: preserving the result of callbacks that throw after their timeout.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description follows the template's two required sections, "What does this PR do?" and "How did you verify your code works?". The Problem, Fix, and Background sections state the cause and the chang…
Full details: Description check

Explanation

The description follows the template's two required sections, "What does this PR do?" and "How did you verify your code works?". The Problem, Fix, and Background sections state the cause and the change. The Downsides section lists the limits of the change. The Notes section gives a detailed table of changed and unchanged results, measurements, tests run, and related open PRs. The verification section names the three suites that fail on main and pass with the change, plus 17 other runner suites. It also states that the platform-specific test proof was deferred to CI. The description is thorough and mostly complete. Its header uses the Problem, Fix, Background, and Downsides headings, not the template's exact headings, and it is very long.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between e7e0064 and d927955.

Files selected for processing: 1
  • src/runtime/test_runner/Execution.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

@robobun

robobun commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced with this fixture, on a build of main (3bf1a06) and on this branch:

import { test } from "bun:test";

test("throws after its timeout", () => {
  Bun.sleepSync(200);
  throw new Error("own error");
}, 100);

test.failing("test.failing throws after its timeout", () => {
  Bun.sleepSync(200);
  throw new Error("the expected failure");
}, 100);
  • main: the first test prints error: own error and ^ this test timed out after 100ms.. The second test prints (fail) with the same timeout line.
  • This branch: the first test prints only error: own error. The second test prints (pass).

CI, build 137324 on d927955 (finished):

  • The three test files of this PR pass on every lane: Linux x64 and aarch64 (glibc, musl, ASAN), Windows x64 and aarch64, macOS x64 and aarch64.
  • The red jobs are test/aot/* and test/bundler/compile-aot-size.test.ts. This diff does not touch them. They fail on main's build of the base commit too (build 137162). One of them, test/aot/rerun-each-runs-a-test-file-again-over-what-it-imports.test.ts, is not in that list: I ran its failing project on a build of main's sources at the base commit, and plain bun test and bun test --aot print the same two different outputs there.

PR: #47867

@github-actions github-actions Bot added the claude label Nov 3, 2026
@robobun

robobun commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author
Updated 11:10 AM PT - Nov 3rd, 2026

❌ @robobun, your commit d927955 has 40 failures in Build #137324 (All Failures):

🧪   To try this PR locally:

bunx bun-pr 47867

That installs a local version of the PR into your bun-47867 executable, so you can run:

bun-47867 --bun

claude[bot]
claude Bot reviewed Nov 3, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/runtime/api/bun/js_bun_spawn_bindings.rs Resolved
Comment thread src/runtime/test_runner/Execution.rs Resolved
Comment thread src/runtime/test_runner/bun_test.rs Resolved
Comment thread src/runtime/test_runner/Execution.rs Resolved
@robobun
bun test: time out a test that calls done(error) in time and returns …
e7e0064 
…late

`done(error)` ends a callback as `Failed` only when the call comes
after the deadline of the test. A test that passes an error to `done`
in time and then returns after its timeout is a late return: the
runner reports the timeout, as it does on main.

`DoneCallback.failed_late` replaces `failed_test`.
`ExecutionEntry::is_past_deadline` holds the deadline comparison that
`evaluate_timeout` had inline.
github-actions[bot]
github-actions Bot reviewed Nov 3, 2026
View reviewed changes
Comment thread src/runtime/test_runner/Execution.rs Resolved
github-actions[bot]
github-actions Bot reviewed Nov 3, 2026
View reviewed changes
Comment thread src/runtime/test_runner/Execution.rs Resolved
github-actions[bot]
github-actions Bot reviewed Nov 3, 2026
View reviewed changes
Comment thread src/runtime/test_runner/Execution.rs Resolved
claude[bot]
claude Bot reviewed Nov 3, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code review found no issues

No high-confidence issues detected in this change.

This was referenced Nov 5, 2026
Open
Open
Open
Open
Draft
Open
Open
Open

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments
@claude claude[bot] claude[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Jarred-Sumner Jarred-Sumner

Labels

claude

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@robobun @Jarred-Sumner