From 03bd8c48f67c202c30138cb5607d9cb955110811 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 05:01:29 +0000 Subject: [PATCH 1/7] bun test: ignore completions from an earlier attempt of a retried test A test callback's completion (promise settling, done() call) is matched back to the running entry by group, sequence, entry pointer and the sequence's remaining repeat count. A retry resets the sequence without changing any of those, so when a timed-out attempt's promise settled or its done() fired while the retry was running, the runner took it as the retry's completion: the retry was reported as passed while its body was still running, and a late rejection was charged to the retry. Give ExecutionSequence a generation counter that every reset (retry or repeat) bumps, stamp it into EntryData in place of the repeat count, and compare it in get_current_and_valid_execution_sequence, so a completion from any earlier attempt is discarded the same way a completion from an earlier repeat already was. --- src/runtime/test_runner/Execution.rs | 18 ++- src/runtime/test_runner/bun_test.rs | 9 +- .../bun/test/test-retry-repeats-basic.test.ts | 144 +++++++++++++++++- 3 files changed, 159 insertions(+), 12 deletions(-) diff --git a/src/runtime/test_runner/Execution.rs b/src/runtime/test_runner/Execution.rs index a864c8f8b17b..e02906bcbb49 100644 --- a/src/runtime/test_runner/Execution.rs +++ b/src/runtime/test_runner/Execution.rs @@ -93,7 +93,7 @@ pub struct Execution { /// around `run_test_callback` so code re-entered from a test body (e.g. /// spawnSync's wait loop) can read the calling entry's own deadline. pub(crate) on_stack_entry: core::cell::Cell>>, - /// The (group_index, sequence_index, entry, repeat) for `on_stack_entry`, + /// The (sequence_index, entry, generation) for `on_stack_entry`, /// set/restored alongside it. `get_current_state_data()` can't name a /// sequence inside a concurrent group; this can, for code re-entered from /// the microtask drain inside `run_test_callback` (node:test's runtime @@ -159,6 +159,13 @@ pub struct ExecutionSequence { pub(crate) test_entry: Option>, pub(crate) remaining_repeat_count: u32, pub(crate) remaining_retry_count: u32, + /// Bumped by every [`Execution::reset_sequence`] (retry or repeat). Each + /// callback's [`EntryData`] records the generation it started under, so a + /// completion arriving from an earlier attempt of this sequence (a timed-out + /// attempt's promise settling or `done()` firing while the retry runs) is + /// rejected by [`Execution::get_current_and_valid_execution_sequence`] + /// instead of finishing the attempt that is currently running. + pub(crate) generation: u32, pub(crate) result: Result, pub(crate) executing: bool, pub(crate) started_at: Timespec, @@ -183,6 +190,7 @@ impl ExecutionSequence { remaining_repeat_count: repeat_count, remaining_retry_count: retry_count, // defaults: + generation: 0, result: Result::Pending, executing: false, started_at: Timespec::EPOCH, @@ -475,9 +483,9 @@ impl Execution { return None; } let sequence = &mut self.sequences[seq_abs]; - if i64::from(sequence.remaining_repeat_count) != entry_data.remaining_repeat_count { + if sequence.generation != entry_data.generation { group_log::log(format_args!( - "runOneCompleted: the data is for a previous repeat count (outdated)", + "runOneCompleted: the data is for a previous retry/repeat of the sequence (outdated)", )); return None; } @@ -740,12 +748,14 @@ impl Execution { } // Preserve retry/repeat counts across reset + let generation = sequence.generation; *sequence = ExecutionSequence::init( sequence.first_entry, sequence.test_entry, sequence.remaining_retry_count, sequence.remaining_repeat_count, ); + sequence.generation = generation.wrapping_add(1); // Snapshot counters are keyed by full test name and incremented on every // toMatchSnapshot() call. Without this reset, retries / repeats would @@ -1004,7 +1014,7 @@ fn step_sequence_one( let entry_data = EntryData { sequence_index, entry: next_item_ptr.as_ptr() as *const (), - remaining_repeat_count: sequence.remaining_repeat_count as i64, + generation: sequence.generation, }; let callback_data = RefDataValue::Execution { group_index: this.group_index, diff --git a/src/runtime/test_runner/bun_test.rs b/src/runtime/test_runner/bun_test.rs index 8f2ee1f522c2..924e2ced261e 100644 --- a/src/runtime/test_runner/bun_test.rs +++ b/src/runtime/test_runner/bun_test.rs @@ -721,7 +721,7 @@ impl BunTest { entry_data: Some(EntryData { sequence_index: active_sequence_index, entry: active_entry.as_ptr().cast::<()>(), - remaining_repeat_count: sequence.remaining_repeat_count as i64, + generation: sequence.generation, }), } } @@ -1426,7 +1426,8 @@ bun_jsc::jsc_host_abi! { pub struct EntryData { pub(crate) sequence_index: usize, pub(crate) entry: *const (), - pub(crate) remaining_repeat_count: i64, + /// `ExecutionSequence::generation` at the time the entry was started. + pub(crate) generation: u32, } // Clone: bitwise OK — `active_scope` is a non-owning borrow of a @@ -1493,8 +1494,8 @@ impl fmt::Display for RefDataValue { if let Some(ed) = entry_data { write!( f, - "execution: group_index={},sequence_index={},entry_index={:x},remaining_repeat_count={}", - group_index, ed.sequence_index, ed.entry as usize, ed.remaining_repeat_count + "execution: group_index={},sequence_index={},entry_index={:x},generation={}", + group_index, ed.sequence_index, ed.entry as usize, ed.generation ) } else { write!(f, "execution: group_index={}", group_index) diff --git a/test/js/bun/test/test-retry-repeats-basic.test.ts b/test/js/bun/test/test-retry-repeats-basic.test.ts index d3edd46561a8..4bbde85ddafc 100644 --- a/test/js/bun/test/test-retry-repeats-basic.test.ts +++ b/test/js/bun/test/test-retry-repeats-basic.test.ts @@ -1,8 +1,8 @@ -// Runs the retry/repeats hook-ordering checks in a subprocess so the -// intentional intermediate retry failures don't leak into this run's -// reporter output (JUnit, GitHub Actions annotations). +// Every check here runs `bun test` in a subprocess so the intentional +// intermediate retry failures don't leak into this run's reporter output +// (JUnit, GitHub Actions annotations). import { expect, test } from "bun:test"; -import { bunEnv, bunExe } from "harness"; +import { bunEnv, bunExe, tempDir } from "harness"; import { join } from "node:path"; test("retry and repeats hook ordering", async () => { @@ -20,3 +20,139 @@ test("retry and repeats hook ordering", async () => { expect(stderr).toContain("(attempt 3)"); expect(exitCode).toBe(0); }); + +async function runRetryFixture(name: string, source: string) { + using dir = tempDir(name, { "retry.test.ts": source }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", "retry.test.ts"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; +} + +// In each fixture below, attempt 1 times out while waiting on something that +// attempt 2 completes as soon as it starts. Attempt 1's late completion must +// not count as the completion of attempt 2. + +test.concurrent("a late resolve from a timed-out attempt does not complete the retry", async () => { + const { stdout, stderr, exitCode } = await runRetryFixture( + "retry-stale-resolve", + ` + import { test, expect } from "bun:test"; + const first = Promise.withResolvers(); + let attempt = 0; + test("retry", async () => { + attempt++; + if (attempt === 1) { + await first.promise; + return; + } + first.resolve(); + await Bun.sleep(1); + console.log("attempt 2 body finished"); + expect(attempt).toBe(1); + }, { retry: 1, timeout: 500 }); + `, + ); + + expect(stdout).toContain("attempt 2 body finished"); + expect(stderr).toContain("(fail) retry (attempt 2)"); + expect(stderr).toContain("Expected: 1"); + expect(stderr).toContain("Received: 2"); + expect(stderr).toContain("0 pass"); + expect(stderr).toContain("1 fail"); + expect(exitCode).toBe(1); +}); + +test.concurrent("a late resolve from a timed-out attempt keeps the retry's own timeout armed", async () => { + const { stderr, exitCode } = await runRetryFixture( + "retry-stale-resolve-timeout", + ` + import { test } from "bun:test"; + const first = Promise.withResolvers(); + let attempt = 0; + test("retry", async () => { + attempt++; + if (attempt === 1) { + await first.promise; + return; + } + first.resolve(); + await new Promise(() => {}); + }, { retry: 1, timeout: 100 }); + `, + ); + + expect(stderr).toContain("(fail) retry (attempt 2)"); + expect(stderr).toContain("this test timed out after 100ms"); + expect(stderr).toContain("0 pass"); + expect(stderr).toContain("1 fail"); + expect(exitCode).toBe(1); +}); + +test.concurrent("a late done() from a timed-out attempt does not complete the retry", async () => { + const { stdout, stderr, exitCode } = await runRetryFixture( + "retry-stale-done", + ` + import { test } from "bun:test"; + let firstDone: (err?: unknown) => void; + let attempt = 0; + test("retry", done => { + attempt++; + if (attempt === 1) { + firstDone = done; + return; + } + firstDone(); + setTimeout(() => { + console.log("attempt 2 body finished"); + done(new Error("attempt 2 failed on its own")); + }, 1); + }, { retry: 1, timeout: 500 }); + `, + ); + + expect(stdout).toContain("attempt 2 body finished"); + expect(stderr).toContain("error: attempt 2 failed on its own"); + expect(stderr).toContain("(fail) retry (attempt 2)"); + expect(stderr).toContain("0 pass"); + expect(stderr).toContain("1 fail"); + expect(exitCode).toBe(1); +}); + +test.concurrent("a late rejection from a timed-out attempt is not attributed to the retry", async () => { + const { stdout, stderr, exitCode } = await runRetryFixture( + "retry-stale-reject", + ` + import { test } from "bun:test"; + const first = Promise.withResolvers(); + let attempt = 0; + test("retry", async () => { + attempt++; + if (attempt === 1) { + await first.promise; + return; + } + first.reject(new Error("late rejection from attempt 1")); + await Bun.sleep(1); + console.log("attempt 2 body finished"); + }, { retry: 2, timeout: 500 }); + `, + ); + + // Same as a timed-out test's promise rejecting while the next test runs: the + // error is reported between tests, and the running attempt is left alone. + expect(stdout).toContain("attempt 2 body finished"); + expect(stderr).toContain("Unhandled error between tests"); + expect(stderr).toContain("error: late rejection from attempt 1"); + expect(stderr).toContain("(pass) retry (attempt 2)"); + expect(stderr).not.toContain("(attempt 3)"); + expect(stderr).toContain("1 pass"); + expect(stderr).toContain("0 fail"); + expect(stderr).toContain("1 error"); + expect(exitCode).toBe(1); +}); From bf1edbe08bf6c2896237f34c48d0dca23f709c82 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 05:51:06 +0000 Subject: [PATCH 2/7] bun test: report done(error) against the entry that owns the done callback done(error) went through the VM's generic uncaught-exception path, which attributes the error to whatever the runner is executing at that moment. The DoneCallback already holds the RefData naming the entry and attempt it was handed to, so a done(error) arriving after that attempt timed out was charged to the retry (or to the next test), and a done(error) from a concurrent test was reported as an unhandled error while the test passed. Route it through BunTest::on_uncaught_exception with the stored RefDataValue, as the promise rejection path already does. The generation check then discards a previous attempt's error the same way it discards its completion; a done(error) called synchronously inside the callback (no ref attached yet) keeps using the generic path. The sequence now advances on the next tick for done(error) as it already did for done(), which drops the previous test's done() frame from the next test's error stacks in the existing snapshot. --- src/runtime/test_runner/bun_test.rs | 50 ++++++++----- .../test-error-code-done-callback.test.ts | 71 ++++++++++++++++++- .../bun/test/test-retry-repeats-basic.test.ts | 39 +++++++++- 3 files changed, 136 insertions(+), 24 deletions(-) diff --git a/src/runtime/test_runner/bun_test.rs b/src/runtime/test_runner/bun_test.rs index 924e2ced261e..d83573a4a2c3 100644 --- a/src/runtime/test_runner/bun_test.rs +++ b/src/runtime/test_runner/bun_test.rs @@ -819,31 +819,45 @@ impl BunTest { let [value] = callframe.arguments_as_array::<1>(); let was_error = !value.is_empty_or_undefined_or_null(); + // A second done() is a no-op, as in Bun 1.2.20. + // In Jest it is "Expected done to be called once, but it was called multiple times." + // Vitest does not support done callbacks. // SAFETY: `this` is the live `*mut DoneCallback` returned by `from_js`; // single-threaded JS VM, GC keeps the wrapper alive for the call frame. - if unsafe { (*this).called } { - // in Bun 1.2.20, this is a no-op - // in Jest, this is "Expected done to be called once, but it was called multiple times." - // Vitest does not support done callbacks - } else { - // error is only reported for the first done() call - if was_error { - let _ = global_this.bun_vm().as_mut().uncaught_exception(global_this, value, false); - } - } - // SAFETY: see above — `this` is a live `*mut DoneCallback`. - let ref_in = unsafe { + let (first_call, ref_in) = unsafe { + let first_call = !(*this).called; (*this).called = true; - (*this).r#ref.take() + (first_call, (*this).r#ref.take()) }; - let Some(ref_in) = ref_in else { - return Ok(JSValue::UNDEFINED); - }; - // `this.ref` was already taken above. // RefPtr currently has NO Drop impl, so decrement the // intrusive count explicitly at scope exit. Without this the // paired promise then/catch path never sees has_one_ref()==true and the RefData leaks. - let ref_in = scopeguard::guard(ref_in, |r: RefDataPtr| r.deref()); + let ref_in = ref_in.map(|r| scopeguard::guard(r, |r: RefDataPtr| r.deref())); + + // error is only reported for the first done() call + if first_call && was_error { + // Report against the entry/attempt the callback was handed to (as `bun_test_then_or_catch` + // does for a rejection), so a done(error) that arrives after that entry timed out is an + // unhandled error rather than a failure of whatever runs now. The ref is only attached + // once the callback returns; a done(error) made while it is still on the stack has no + // ref, and the generic path reports against the running entry. + let owner = ref_in + .as_ref() + .and_then(|r| Some((r.buntest_weak.upgrade()?, &r.phase))); + match owner { + Some((strong, phase)) => { + // SAFETY: `&mut` derived via `UnsafeCell`; the borrow ends with this call. + strong.get().on_uncaught_exception(global_this, Some(value), false, phase); + } + None => { + let _ = global_this.bun_vm().as_mut().uncaught_exception(global_this, value, false); + } + } + } + + let Some(ref_in) = ref_in else { + return Ok(JSValue::UNDEFINED); + }; // dupe the ref and enqueue a task to call the done callback. // this makes it so if you do something else after calling done(), the next test doesn't start running until the next tick. diff --git a/test/js/bun/test/test-error-code-done-callback.test.ts b/test/js/bun/test/test-error-code-done-callback.test.ts index 610c4d85e7ec..c4c045cabe4d 100644 --- a/test/js/bun/test/test-error-code-done-callback.test.ts +++ b/test/js/bun/test/test-error-code-done-callback.test.ts @@ -1,5 +1,5 @@ import { expect, test } from "bun:test"; -import { bunEnv, bunExe } from "harness"; +import { bunEnv, bunExe, tempDir } from "harness"; import path from "path"; test("verify we print error messages passed to done callbacks", () => { @@ -80,7 +80,6 @@ test("verify we print error messages passed to done callbacks", () => { ^ error: you should see this(async) at (/test-error-done-callback-fixture.ts:42:14) - at (/test-error-done-callback-fixture.ts:37:3) (fail) error done callback (async) 43 | }); 44 | }); @@ -111,7 +110,6 @@ test("verify we print error messages passed to done callbacks", () => { ^ error: you should see this(async, nextTick) at (/test-error-done-callback-fixture.ts:60:14) - at (/test-error-done-callback-fixture.ts:54:5) (fail) error done callback (async, nextTick) 62 | }); 63 | @@ -140,3 +138,70 @@ test("verify we print error messages passed to done callbacks", () => { " `); }); + +async function runDoneFixture(name: string, source: string) { + using dir = tempDir(name, { "done.test.ts": source }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", "done.test.ts"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; +} + +test.concurrent("done(error) from a test that already timed out is not charged to the test running now", async () => { + const { stdout, stderr, exitCode } = await runDoneFixture( + "done-error-after-timeout", + ` + import { test } from "bun:test"; + let firstDone: (err?: unknown) => void; + test("first", done => { + firstDone = done; + }, { timeout: 100 }); + test("second", done => { + firstDone(new Error("late error from first")); + setTimeout(() => { + console.log("second body finished"); + done(); + }, 1); + }, { timeout: 500 }); + `, + ); + + // Same report as a timed-out test's promise rejecting during the next test. + expect(stdout).toContain("second body finished"); + expect(stderr).toContain("(fail) first"); + expect(stderr).toContain("Unhandled error between tests"); + expect(stderr).toContain("error: late error from first"); + expect(stderr).toContain("(pass) second"); + expect(stderr).toContain("1 pass"); + expect(stderr).toContain("1 fail"); + expect(stderr).toContain("1 error"); + expect(exitCode).toBe(1); +}); + +test.concurrent("done(error) called later from a concurrent test fails that test", async () => { + const { stderr, exitCode } = await runDoneFixture( + "done-error-concurrent", + ` + import { test } from "bun:test"; + test.concurrent("fails", done => { + setTimeout(() => done(new Error("reported through done")), 1); + }); + test.concurrent("passes", done => { + setTimeout(() => done(), 1); + }); + `, + ); + + expect(stderr).toContain("error: reported through done"); + expect(stderr).toContain("(fail) fails"); + expect(stderr).toContain("(pass) passes"); + expect(stderr).not.toContain("Unhandled error between tests"); + expect(stderr).toContain("1 pass"); + expect(stderr).toContain("1 fail"); + expect(exitCode).toBe(1); +}); diff --git a/test/js/bun/test/test-retry-repeats-basic.test.ts b/test/js/bun/test/test-retry-repeats-basic.test.ts index 4bbde85ddafc..06dfc894c665 100644 --- a/test/js/bun/test/test-retry-repeats-basic.test.ts +++ b/test/js/bun/test/test-retry-repeats-basic.test.ts @@ -36,7 +36,9 @@ async function runRetryFixture(name: string, source: string) { // In each fixture below, attempt 1 times out while waiting on something that // attempt 2 completes as soon as it starts. Attempt 1's late completion must -// not count as the completion of attempt 2. +// not count as the completion of attempt 2, and a late error from attempt 1 is +// reported the way a timed-out test's late error is reported while the next +// test runs: as an unhandled error between tests, leaving attempt 2 alone. test.concurrent("a late resolve from a timed-out attempt does not complete the retry", async () => { const { stdout, stderr, exitCode } = await runRetryFixture( @@ -124,6 +126,39 @@ test.concurrent("a late done() from a timed-out attempt does not complete the re expect(exitCode).toBe(1); }); +test.concurrent("a late done(error) from a timed-out attempt is not attributed to the retry", async () => { + const { stdout, stderr, exitCode } = await runRetryFixture( + "retry-stale-done-error", + ` + import { test } from "bun:test"; + let firstDone: (err?: unknown) => void; + let attempt = 0; + test("retry", done => { + attempt++; + if (attempt === 1) { + firstDone = done; + return; + } + firstDone(new Error("late error from attempt 1")); + setTimeout(() => { + console.log("attempt 2 body finished"); + done(); + }, 1); + }, { retry: 2, timeout: 500 }); + `, + ); + + expect(stdout).toContain("attempt 2 body finished"); + expect(stderr).toContain("Unhandled error between tests"); + expect(stderr).toContain("error: late error from attempt 1"); + expect(stderr).toContain("(pass) retry (attempt 2)"); + expect(stderr).not.toContain("(attempt 3)"); + expect(stderr).toContain("1 pass"); + expect(stderr).toContain("0 fail"); + expect(stderr).toContain("1 error"); + expect(exitCode).toBe(1); +}); + test.concurrent("a late rejection from a timed-out attempt is not attributed to the retry", async () => { const { stdout, stderr, exitCode } = await runRetryFixture( "retry-stale-reject", @@ -144,8 +179,6 @@ test.concurrent("a late rejection from a timed-out attempt is not attributed to `, ); - // Same as a timed-out test's promise rejecting while the next test runs: the - // error is reported between tests, and the running attempt is left alone. expect(stdout).toContain("attempt 2 body finished"); expect(stderr).toContain("Unhandled error between tests"); expect(stderr).toContain("error: late rejection from attempt 1"); From 7dcd6574cc2dbb23d08cdfbeedbd3e91c30a4dd6 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 05:53:59 +0000 Subject: [PATCH 3/7] test runner: shorten the generation and done(error) comments --- src/runtime/test_runner/Execution.rs | 8 ++------ src/runtime/test_runner/bun_test.rs | 12 ++++-------- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/src/runtime/test_runner/Execution.rs b/src/runtime/test_runner/Execution.rs index e02906bcbb49..447ada3fd0db 100644 --- a/src/runtime/test_runner/Execution.rs +++ b/src/runtime/test_runner/Execution.rs @@ -159,12 +159,8 @@ pub struct ExecutionSequence { pub(crate) test_entry: Option>, pub(crate) remaining_repeat_count: u32, pub(crate) remaining_retry_count: u32, - /// Bumped by every [`Execution::reset_sequence`] (retry or repeat). Each - /// callback's [`EntryData`] records the generation it started under, so a - /// completion arriving from an earlier attempt of this sequence (a timed-out - /// attempt's promise settling or `done()` firing while the retry runs) is - /// rejected by [`Execution::get_current_and_valid_execution_sequence`] - /// instead of finishing the attempt that is currently running. + /// Bumped by every [`Execution::reset_sequence`] (retry or repeat). [`EntryData`] carries the + /// generation a callback started under, so a completion from an earlier attempt is stale. pub(crate) generation: u32, pub(crate) result: Result, pub(crate) executing: bool, diff --git a/src/runtime/test_runner/bun_test.rs b/src/runtime/test_runner/bun_test.rs index d83573a4a2c3..d1fd8b108e6d 100644 --- a/src/runtime/test_runner/bun_test.rs +++ b/src/runtime/test_runner/bun_test.rs @@ -819,9 +819,7 @@ impl BunTest { let [value] = callframe.arguments_as_array::<1>(); let was_error = !value.is_empty_or_undefined_or_null(); - // A second done() is a no-op, as in Bun 1.2.20. - // In Jest it is "Expected done to be called once, but it was called multiple times." - // Vitest does not support done callbacks. + // A second done() is a no-op (as in Bun 1.2.20; Jest reports it as an error). // SAFETY: `this` is the live `*mut DoneCallback` returned by `from_js`; // single-threaded JS VM, GC keeps the wrapper alive for the call frame. let (first_call, ref_in) = unsafe { @@ -836,11 +834,9 @@ impl BunTest { // error is only reported for the first done() call if first_call && was_error { - // Report against the entry/attempt the callback was handed to (as `bun_test_then_or_catch` - // does for a rejection), so a done(error) that arrives after that entry timed out is an - // unhandled error rather than a failure of whatever runs now. The ref is only attached - // once the callback returns; a done(error) made while it is still on the stack has no - // ref, and the generic path reports against the running entry. + // Report against the entry the ref names, like `bun_test_then_or_catch`, so a late + // done(error) from a timed-out entry is not charged to whatever runs now. No ref means + // done() is still inside its own callback (the ref is attached after it returns). let owner = ref_in .as_ref() .and_then(|r| Some((r.buntest_weak.upgrade()?, &r.phase))); From d228023f9defcb641a0a15fa1f86e934a6bf7717 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 05:57:35 +0000 Subject: [PATCH 4/7] test runner: one-line comments for generation and done(error) attribution --- src/runtime/test_runner/Execution.rs | 3 +-- src/runtime/test_runner/bun_test.rs | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/runtime/test_runner/Execution.rs b/src/runtime/test_runner/Execution.rs index 447ada3fd0db..454b9be3fb61 100644 --- a/src/runtime/test_runner/Execution.rs +++ b/src/runtime/test_runner/Execution.rs @@ -159,8 +159,7 @@ pub struct ExecutionSequence { pub(crate) test_entry: Option>, pub(crate) remaining_repeat_count: u32, pub(crate) remaining_retry_count: u32, - /// Bumped by every [`Execution::reset_sequence`] (retry or repeat). [`EntryData`] carries the - /// generation a callback started under, so a completion from an earlier attempt is stale. + /// Bumped by every [`Execution::reset_sequence`]; an [`EntryData`] with an older one is from an earlier retry/repeat. pub(crate) generation: u32, pub(crate) result: Result, pub(crate) executing: bool, diff --git a/src/runtime/test_runner/bun_test.rs b/src/runtime/test_runner/bun_test.rs index d1fd8b108e6d..e3ed7e458103 100644 --- a/src/runtime/test_runner/bun_test.rs +++ b/src/runtime/test_runner/bun_test.rs @@ -834,17 +834,16 @@ impl BunTest { // error is only reported for the first done() call if first_call && was_error { - // Report against the entry the ref names, like `bun_test_then_or_catch`, so a late - // done(error) from a timed-out entry is not charged to whatever runs now. No ref means - // done() is still inside its own callback (the ref is attached after it returns). let owner = ref_in .as_ref() .and_then(|r| Some((r.buntest_weak.upgrade()?, &r.phase))); match owner { + // Same attribution as `bun_test_then_or_catch`: a stale ref is reported as unhandled, not charged to the running entry. Some((strong, phase)) => { // SAFETY: `&mut` derived via `UnsafeCell`; the borrow ends with this call. strong.get().on_uncaught_exception(global_this, Some(value), false, phase); } + // No ref yet: done() is still inside its own callback (`run_test_callback` attaches the ref after it returns). None => { let _ = global_this.bun_vm().as_mut().uncaught_exception(global_this, value, false); } From af5f7f5b7183ae99b7621799c9385b9c1d5fb839 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:28:09 +0000 Subject: [PATCH 5/7] bun test: leave done(error) attribution to #33089 Drops the bun_test_done_callback change and its tests. It duplicated a subset of #33089 (and collides with #34041, which edits the same block), and on its own it made a hook's done(error) fail the hook only when the callback fired from a macrotask. This PR is the generation check alone; #33089's rerouting of done(error) through the stored ref picks up the retry case once it is rebased on it. --- src/runtime/test_runner/bun_test.rs | 45 +++++------- .../test-error-code-done-callback.test.ts | 71 +------------------ .../bun/test/test-retry-repeats-basic.test.ts | 39 +--------- 3 files changed, 24 insertions(+), 131 deletions(-) diff --git a/src/runtime/test_runner/bun_test.rs b/src/runtime/test_runner/bun_test.rs index e3ed7e458103..924e2ced261e 100644 --- a/src/runtime/test_runner/bun_test.rs +++ b/src/runtime/test_runner/bun_test.rs @@ -819,40 +819,31 @@ impl BunTest { let [value] = callframe.arguments_as_array::<1>(); let was_error = !value.is_empty_or_undefined_or_null(); - // A second done() is a no-op (as in Bun 1.2.20; Jest reports it as an error). // SAFETY: `this` is the live `*mut DoneCallback` returned by `from_js`; // single-threaded JS VM, GC keeps the wrapper alive for the call frame. - let (first_call, ref_in) = unsafe { - let first_call = !(*this).called; - (*this).called = true; - (first_call, (*this).r#ref.take()) - }; - // RefPtr currently has NO Drop impl, so decrement the - // intrusive count explicitly at scope exit. Without this the - // paired promise then/catch path never sees has_one_ref()==true and the RefData leaks. - let ref_in = ref_in.map(|r| scopeguard::guard(r, |r: RefDataPtr| r.deref())); - - // error is only reported for the first done() call - if first_call && was_error { - let owner = ref_in - .as_ref() - .and_then(|r| Some((r.buntest_weak.upgrade()?, &r.phase))); - match owner { - // Same attribution as `bun_test_then_or_catch`: a stale ref is reported as unhandled, not charged to the running entry. - Some((strong, phase)) => { - // SAFETY: `&mut` derived via `UnsafeCell`; the borrow ends with this call. - strong.get().on_uncaught_exception(global_this, Some(value), false, phase); - } - // No ref yet: done() is still inside its own callback (`run_test_callback` attaches the ref after it returns). - None => { - let _ = global_this.bun_vm().as_mut().uncaught_exception(global_this, value, false); - } + if unsafe { (*this).called } { + // in Bun 1.2.20, this is a no-op + // in Jest, this is "Expected done to be called once, but it was called multiple times." + // Vitest does not support done callbacks + } else { + // error is only reported for the first done() call + if was_error { + let _ = global_this.bun_vm().as_mut().uncaught_exception(global_this, value, false); } } - + // SAFETY: see above — `this` is a live `*mut DoneCallback`. + let ref_in = unsafe { + (*this).called = true; + (*this).r#ref.take() + }; let Some(ref_in) = ref_in else { return Ok(JSValue::UNDEFINED); }; + // `this.ref` was already taken above. + // RefPtr currently has NO Drop impl, so decrement the + // intrusive count explicitly at scope exit. Without this the + // paired promise then/catch path never sees has_one_ref()==true and the RefData leaks. + let ref_in = scopeguard::guard(ref_in, |r: RefDataPtr| r.deref()); // dupe the ref and enqueue a task to call the done callback. // this makes it so if you do something else after calling done(), the next test doesn't start running until the next tick. diff --git a/test/js/bun/test/test-error-code-done-callback.test.ts b/test/js/bun/test/test-error-code-done-callback.test.ts index c4c045cabe4d..610c4d85e7ec 100644 --- a/test/js/bun/test/test-error-code-done-callback.test.ts +++ b/test/js/bun/test/test-error-code-done-callback.test.ts @@ -1,5 +1,5 @@ import { expect, test } from "bun:test"; -import { bunEnv, bunExe, tempDir } from "harness"; +import { bunEnv, bunExe } from "harness"; import path from "path"; test("verify we print error messages passed to done callbacks", () => { @@ -80,6 +80,7 @@ test("verify we print error messages passed to done callbacks", () => { ^ error: you should see this(async) at (/test-error-done-callback-fixture.ts:42:14) + at (/test-error-done-callback-fixture.ts:37:3) (fail) error done callback (async) 43 | }); 44 | }); @@ -110,6 +111,7 @@ test("verify we print error messages passed to done callbacks", () => { ^ error: you should see this(async, nextTick) at (/test-error-done-callback-fixture.ts:60:14) + at (/test-error-done-callback-fixture.ts:54:5) (fail) error done callback (async, nextTick) 62 | }); 63 | @@ -138,70 +140,3 @@ test("verify we print error messages passed to done callbacks", () => { " `); }); - -async function runDoneFixture(name: string, source: string) { - using dir = tempDir(name, { "done.test.ts": source }); - await using proc = Bun.spawn({ - cmd: [bunExe(), "test", "done.test.ts"], - env: bunEnv, - cwd: String(dir), - stdout: "pipe", - stderr: "pipe", - }); - const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - return { stdout, stderr, exitCode }; -} - -test.concurrent("done(error) from a test that already timed out is not charged to the test running now", async () => { - const { stdout, stderr, exitCode } = await runDoneFixture( - "done-error-after-timeout", - ` - import { test } from "bun:test"; - let firstDone: (err?: unknown) => void; - test("first", done => { - firstDone = done; - }, { timeout: 100 }); - test("second", done => { - firstDone(new Error("late error from first")); - setTimeout(() => { - console.log("second body finished"); - done(); - }, 1); - }, { timeout: 500 }); - `, - ); - - // Same report as a timed-out test's promise rejecting during the next test. - expect(stdout).toContain("second body finished"); - expect(stderr).toContain("(fail) first"); - expect(stderr).toContain("Unhandled error between tests"); - expect(stderr).toContain("error: late error from first"); - expect(stderr).toContain("(pass) second"); - expect(stderr).toContain("1 pass"); - expect(stderr).toContain("1 fail"); - expect(stderr).toContain("1 error"); - expect(exitCode).toBe(1); -}); - -test.concurrent("done(error) called later from a concurrent test fails that test", async () => { - const { stderr, exitCode } = await runDoneFixture( - "done-error-concurrent", - ` - import { test } from "bun:test"; - test.concurrent("fails", done => { - setTimeout(() => done(new Error("reported through done")), 1); - }); - test.concurrent("passes", done => { - setTimeout(() => done(), 1); - }); - `, - ); - - expect(stderr).toContain("error: reported through done"); - expect(stderr).toContain("(fail) fails"); - expect(stderr).toContain("(pass) passes"); - expect(stderr).not.toContain("Unhandled error between tests"); - expect(stderr).toContain("1 pass"); - expect(stderr).toContain("1 fail"); - expect(exitCode).toBe(1); -}); diff --git a/test/js/bun/test/test-retry-repeats-basic.test.ts b/test/js/bun/test/test-retry-repeats-basic.test.ts index 06dfc894c665..4bbde85ddafc 100644 --- a/test/js/bun/test/test-retry-repeats-basic.test.ts +++ b/test/js/bun/test/test-retry-repeats-basic.test.ts @@ -36,9 +36,7 @@ async function runRetryFixture(name: string, source: string) { // In each fixture below, attempt 1 times out while waiting on something that // attempt 2 completes as soon as it starts. Attempt 1's late completion must -// not count as the completion of attempt 2, and a late error from attempt 1 is -// reported the way a timed-out test's late error is reported while the next -// test runs: as an unhandled error between tests, leaving attempt 2 alone. +// not count as the completion of attempt 2. test.concurrent("a late resolve from a timed-out attempt does not complete the retry", async () => { const { stdout, stderr, exitCode } = await runRetryFixture( @@ -126,39 +124,6 @@ test.concurrent("a late done() from a timed-out attempt does not complete the re expect(exitCode).toBe(1); }); -test.concurrent("a late done(error) from a timed-out attempt is not attributed to the retry", async () => { - const { stdout, stderr, exitCode } = await runRetryFixture( - "retry-stale-done-error", - ` - import { test } from "bun:test"; - let firstDone: (err?: unknown) => void; - let attempt = 0; - test("retry", done => { - attempt++; - if (attempt === 1) { - firstDone = done; - return; - } - firstDone(new Error("late error from attempt 1")); - setTimeout(() => { - console.log("attempt 2 body finished"); - done(); - }, 1); - }, { retry: 2, timeout: 500 }); - `, - ); - - expect(stdout).toContain("attempt 2 body finished"); - expect(stderr).toContain("Unhandled error between tests"); - expect(stderr).toContain("error: late error from attempt 1"); - expect(stderr).toContain("(pass) retry (attempt 2)"); - expect(stderr).not.toContain("(attempt 3)"); - expect(stderr).toContain("1 pass"); - expect(stderr).toContain("0 fail"); - expect(stderr).toContain("1 error"); - expect(exitCode).toBe(1); -}); - test.concurrent("a late rejection from a timed-out attempt is not attributed to the retry", async () => { const { stdout, stderr, exitCode } = await runRetryFixture( "retry-stale-reject", @@ -179,6 +144,8 @@ test.concurrent("a late rejection from a timed-out attempt is not attributed to `, ); + // Same as a timed-out test's promise rejecting while the next test runs: the + // error is reported between tests, and the running attempt is left alone. expect(stdout).toContain("attempt 2 body finished"); expect(stderr).toContain("Unhandled error between tests"); expect(stderr).toContain("error: late rejection from attempt 1"); From 688fa461de45dad43f6431b21ea7df2b9839aba4 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:36:57 +0000 Subject: [PATCH 6/7] test(retry): document why the stale-completion fixtures do not depend on timer ordering --- test/js/bun/test/test-retry-repeats-basic.test.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/js/bun/test/test-retry-repeats-basic.test.ts b/test/js/bun/test/test-retry-repeats-basic.test.ts index 4bbde85ddafc..e1de57380ff0 100644 --- a/test/js/bun/test/test-retry-repeats-basic.test.ts +++ b/test/js/bun/test/test-retry-repeats-basic.test.ts @@ -37,6 +37,13 @@ async function runRetryFixture(name: string, source: string) { // In each fixture below, attempt 1 times out while waiting on something that // attempt 2 completes as soon as it starts. Attempt 1's late completion must // not count as the completion of attempt 2. +// +// The ordering does not depend on timers: attempt 1's completion is queued while +// attempt 2's body is still on the stack (synchronously by done(), or in the +// microtask drain that follows the body), and the runner consumes it in the same +// step, before anything attempt 2 scheduled can run. On an unfixed runner attempt +// 2 is therefore reported as passed before its own timer fires; the timers only +// keep attempt 2 alive past that point. test.concurrent("a late resolve from a timed-out attempt does not complete the retry", async () => { const { stdout, stderr, exitCode } = await runRetryFixture( From 1bec8cdaa75294237741fa45898a88b8ab8a45c1 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 15 Aug 2026 11:47:12 +0000 Subject: [PATCH 7/7] ci: retrigger