Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/test/lifecycle.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ test("cleanup after test", () => {
});
```

Not supported in concurrent tests; use `test.serial` instead.
Works inside concurrent tests when `onTestFinished()` is called synchronously from the test callback body (including microtasks drained before the first suspension point). Registrations made after yielding to a later event-loop turn — for example after `await`ing a timer — may not resolve which concurrent sequence they belong to.

## Global Setup and Teardown

Expand Down
6 changes: 6 additions & 0 deletions packages/bun-types/test.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,12 @@ declare module "bun:test" {
*
* Can only be called inside a test, not in describe blocks.
*
* Works inside concurrent tests when `onTestFinished()` is called
* synchronously from the test callback body (including microtasks drained
* before the first suspension point). Registrations made after yielding to
* a later event-loop turn — for example after `await`ing a timer — may
* not resolve which concurrent sequence they belong to.
*
* @example
* test("my test", () => {
* onTestFinished(() => {
Comment thread
claude[bot] marked this conversation as resolved.
Expand Down
10 changes: 10 additions & 0 deletions src/bun.js/test/Execution.zig
Original file line number Diff line number Diff line change
Expand Up @@ -394,12 +394,22 @@
.remaining_repeat_count = sequence.remaining_repeat_count,
},
},
};
groupLog.log("runSequence queued callback: {f}", .{callback_data});

// Mark this sequence/entry as the synchronously-executing callback
// so that hooks called from user code (onTestFinished,
// expect.assertions) can look up which concurrent sequence they
// belong to. Any awaited microtasks that are drained by
// `runCallbackWithResultAndForcefullyDrainMicrotasks` also see this
// context. The context is popped once the JS call returns, even if
// the returned promise is still pending.
Comment thread
robobun marked this conversation as resolved.
buntest.pushCurrentCallback(callback_data);
defer buntest.popCurrentCallback();

if (BunTest.runTestCallback(buntest_strong, globalThis, cb.get(), next_item.has_done_parameter, callback_data, &next_item.timespec) != null) {
now.* = bun.timespec.now(.force_real_time);
_ = next_item.evaluateTimeout(sequence, now);

Check warning on line 412 in src/bun.js/test/Execution.zig

View check run for this annotation

Claude / Claude Code Review

toMatchSnapshot() silently unblocked in concurrent tests; test.failing fixture marker now stale

The `pushCurrentCallback` mechanism inadvertently lifts the `SnapshotInConcurrentGroup` restriction for synchronous calls inside concurrent tests, making the `test.failing('snapshot in concurrent group')` fixture stale. The fixture now passes by coincidence (via `SnapshotCreationNotAllowedInCI` in CI) rather than by the original guard, meaning the behavioral change is silently masked. Running `bun_test.fixture.ts` directly in a non-CI environment (without `CI=1`) will cause `toMatchSnapshot()` t
Comment thread
robobun marked this conversation as resolved.

// the result is available immediately; advance the sequence and run again.
this.advanceSequence(sequence, group);
Expand Down
55 changes: 53 additions & 2 deletions src/bun.js/test/bun_test.zig
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,18 @@ pub const js_fns = struct {
.execution => {
const active = bunTest.getCurrentStateData();
const sequence, _ = bunTest.execution.getCurrentAndValidExecutionSequence(active) orelse {
// We only reach this branch when `current_callback_stack` is
// empty *and* the active concurrent group has more than one
// sequence in flight. The common way to hit this is calling
// the hook after the first `await` in a concurrent test —
// by then the synchronous push/pop has already been popped
// and we can no longer tell which sequence the caller
// belongs to. Tell the user to hoist the call above the
// first `await`, which is the only actionable fix here.
const message = if (tag == .onTestFinished)
"Cannot call {s}() here. It cannot be called inside a concurrent test. Use test.serial or remove test.concurrent."
"Cannot call {s}() here. In a concurrent test, call it synchronously before the first `await`."
else
"Cannot call {s}() here. It cannot be called inside a concurrent test. Call it inside describe() instead.";
"Cannot call {s}() here. In a concurrent test, call it synchronously before the first `await`, or move it into a describe() block.";
return globalThis.throw(message, .{@tagName(tag)});
};

Comment thread
claude[bot] marked this conversation as resolved.
Expand Down Expand Up @@ -219,6 +227,15 @@ pub const BunTest = struct {
first_last: BunTestRoot.FirstLast,
extra_execution_entries: std.array_list.Managed(*ExecutionEntry),
wants_wakeup: bool = false,
/// Stack of concurrent-sequence contexts whose JS callback is currently
/// being executed synchronously. Pushed by `Execution.stepSequenceOne`
/// before invoking `runTestCallback` and popped after it returns.
///
/// During synchronous JS execution (including drained microtasks), the
/// top of this stack tells `getCurrentStateData` which concurrent
/// sequence the calling code belongs to, so hooks like `onTestFinished`
/// and `expect.assertions` resolve to the correct test.
current_callback_stack: std.array_list.Managed(RefDataValue),

phase: enum {
collection,
Expand Down Expand Up @@ -253,6 +270,7 @@ pub const BunTest = struct {
.default_concurrent = default_concurrent,
.first_last = first_last,
.extra_execution_entries = .init(this.gpa),
.current_callback_stack = .init(this.gpa),
};
}
pub fn deinit(this: *BunTest) void {
Expand All @@ -268,6 +286,10 @@ pub const BunTest = struct {
entry.destroy(this.gpa);
}
this.extra_execution_entries.deinit();
// every pushCurrentCallback must be paired with popCurrentCallback —
// a non-empty stack at teardown means we leaked a frame.
bun.debugAssert(this.current_callback_stack.items.len == 0);
this.current_callback_stack.deinit();
Comment thread
coderabbitai[bot] marked this conversation as resolved.

this.execution.deinit();
this.collection.deinit();
Expand Down Expand Up @@ -350,6 +372,16 @@ pub const BunTest = struct {
return switch (this.phase) {
.collection => .{ .collection = .{ .active_scope = this.collection.active_scope } },
.execution => blk: {
// If a JS callback is currently executing synchronously (including
// during drained microtasks), the innermost push on
// `current_callback_stack` tells us exactly which sequence/entry
// we're inside. This disambiguates the concurrent case, where
// multiple sequences may be in-flight at the same time.
if (this.current_callback_stack.items.len > 0) {
const top = this.current_callback_stack.items[this.current_callback_stack.items.len - 1];
if (top == .execution) break :blk top;
}

const active_group = this.execution.activeGroup() orelse {
bun.debugAssert(false); // should have switched phase if we're calling getCurrentStateData, but it could happen with re-entry maybe
break :blk .{ .done = .{} };
Comment thread
claude[bot] marked this conversation as resolved.
Expand Down Expand Up @@ -384,6 +416,25 @@ pub const BunTest = struct {
.done => .{ .done = .{} },
};
}

/// Push an entry onto the callback-execution stack. Must be paired with
/// `popCurrentCallback`. Call this immediately before invoking user JS
/// from a concurrent-safe context so nested hooks (`onTestFinished`,
/// `expect.assertions`) can recover which sequence they belong to.
Comment on lines +422 to +425
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.

🟡 The pushCurrentCallback method docstring (lines 422-425 of bun_test.zig) still lists expect.assertions as a beneficiary of the callback stack, claiming it can "recover which sequence they belong to" — but the current_callback_stack field docstring in the same struct (lines 237-246) correctly says the opposite: these functions intentionally do not use this stack and throw unconditionally instead. Commit 64eb666 fixed the identical claim in Execution.zig but left the method docstring stale, creating an internal contradiction within the same struct.

Extended reasoning...

What the bug is:

The pushCurrentCallback method docstring in bun_test.zig lines 422-425 reads:

/// Push an entry onto the callback-execution stack. Must be paired with
/// popCurrentCallback. Call this immediately before invoking user JS
/// from a concurrent-safe context so nested hooks (onTestFinished,
/// expect.assertions) can recover which sequence they belong to.

This directly contradicts the current_callback_stack field docstring in the same struct (lines 237-246), which correctly states: 'expect.assertions() / expect.hasAssertions() / snapshot matchers intentionally do not use this stack — see expect.zig for why they reject concurrent-test calls outright.'

The specific code path that demonstrates the inaccuracy:

In expect.zig, both hasAssertions() and assertions() call isInMultiSequenceConcurrentGroup(buntest) before ever consulting getCurrentStateData() or the callback stack. The throw fires unconditionally before the stack is ever accessed. expect.assertions does not 'recover which sequence it belongs to' via the stack — it doesn't use the stack at all.

Why existing code does not prevent the mismatch:

Commit 64eb666 correctly updated the Execution.zig callsite comment to say these functions 'deliberately do NOT use this stack' and updated the current_callback_stack field docstring to match. However, the pushCurrentCallback method docstring was not updated, leaving an internal contradiction: the field doc and the caller's doc both say one thing, while the method doc says the opposite.

Impact:

This is documentation-only with no runtime impact. The only harm is that a future contributor reading the pushCurrentCallback method doc in isolation could conclude that expect.assertions stack-based recovery is already working, and attempt to build on a false premise when trying to relax the restriction.

How to fix:

Change the method docstring from listing 'onTestFinished, expect.assertions' to just 'onTestFinished', analogous to what 64eb666 already applied to Execution.zig and the field docstring.

Step-by-step proof:

  1. bun_test.zig lines 422-425: pushCurrentCallback docstring says expect.assertions can recover via the stack.
  2. bun_test.zig lines 241-246: current_callback_stack field docstring says expect.assertions intentionally does NOT use this stack.
  3. expect.zig: isInMultiSequenceConcurrentGroup throws before getCurrentStateData() is called — the stack is never consulted.
  4. Commit 64eb666 fixed the equivalent statement in Execution.zig but missed the method docstring in bun_test.zig.

pub fn pushCurrentCallback(this: *BunTest, data: RefDataValue) void {
bun.handleOom(this.current_callback_stack.append(data));
}

/// Pop the innermost callback-execution entry. Cheaply tolerates an
/// out-of-order pop during teardown — the only correct caller is the
/// defer that paired a preceding `pushCurrentCallback`.
pub fn popCurrentCallback(this: *BunTest) void {
if (this.current_callback_stack.items.len == 0) {
bun.debugAssert(false);
return;
}
_ = this.current_callback_stack.pop();
}
Comment thread
claude[bot] marked this conversation as resolved.
pub fn ref(this_strong: BunTestPtr, phase: RefDataValue) *RefData {
group.begin(@src());
defer group.end();
Expand Down
10 changes: 6 additions & 4 deletions test/js/bun/test/bun_test.fixture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,14 @@ test.failing("expect.assertions", () => {
expect.hasAssertions(); // make sure this doesn't overwrite the assertions count, matching existing behaviour
});

test.concurrent.failing("expect.assertions not yet supported in concurrent tests", () => {
expect.hasAssertions(); // this call will fail because expect.hasAssertions is not yet supported in concurrent tests
// https://github.com/oven-sh/bun/issues/29236 — expect.hasAssertions and
// expect.assertions resolve to the currently executing concurrent test.
test.concurrent("expect.hasAssertions works in concurrent tests", () => {
expect.hasAssertions();
expect(true).toBe(true);
});
test.concurrent.failing("expect.assertions not yet supported in concurrent tests", () => {
expect.assertions(1); // this call will fail because expect.assertions is not yet supported in concurrent tests
test.concurrent("expect.assertions works in concurrent tests", () => {
expect.assertions(1);
expect(true).toBe(true);
});

Expand Down
4 changes: 2 additions & 2 deletions test/js/bun/test/bun_test.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ test("describe/test", async () => {
(pass) addition 3 + 4 = 7
AssertionError: expected 1 assertion, but test ended with 0 assertions
(fail) expect.assertions
(pass) expect.assertions not yet supported in concurrent tests
(pass) expect.assertions not yet supported in concurrent tests
(pass) expect.hasAssertions works in concurrent tests
(pass) expect.assertions works in concurrent tests
(pass) expect.assertions works
(fail) expect.assertions combined with timeout
^ this test timed out after 1ms.
Expand Down
44 changes: 24 additions & 20 deletions test/js/bun/test/test-on-test-finished.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,26 +68,30 @@ describe("async onTestFinished", () => {
});
});

// Test that onTestFinished throws proper error in concurrent tests
describe("onTestFinished errors", () => {
test.concurrent("cannot be called in concurrent test 1", () => {
expect(() => {
onTestFinished(() => {
console.log("should not run");
});
}).toThrow(
"Cannot call onTestFinished() here. It cannot be called inside a concurrent test. Use test.serial or remove test.concurrent.",
);
});

test.concurrent("cannot be called in concurrent test 2", () => {
expect(() => {
onTestFinished(() => {
console.log("should not run");
});
}).toThrow(
"Cannot call onTestFinished() here. It cannot be called inside a concurrent test. Use test.serial or remove test.concurrent.",
);
// https://github.com/oven-sh/bun/issues/29236 — onTestFinished() is
// callable from inside a concurrent test. Each sequence accumulates its
// own hooks and runs them at the end of that sequence.
describe("onTestFinished in concurrent tests", () => {
const a_output: string[] = [];
const b_output: string[] = [];

test.concurrent("test a", () => {
onTestFinished(() => {
a_output.push("a-finished");
});
a_output.push("a-body");
});

test.concurrent("test b", () => {
onTestFinished(() => {
b_output.push("b-finished");
});
b_output.push("b-body");
});

test("verify each sequence ran its own hook", () => {
expect(a_output).toEqual(["a-body", "a-finished"]);
expect(b_output).toEqual(["b-body", "b-finished"]);
});
});

Expand Down
115 changes: 115 additions & 0 deletions test/regression/issue/29236.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
// https://github.com/oven-sh/bun/issues/29236
import { expect, test } from "bun:test";
import { bunEnv, bunExe, tempDir } from "harness";

test("onTestFinished works inside concurrent tests", async () => {
using dir = tempDir("issue-29236-on-test-finished-concurrent", {
"concurrent.test.ts": /* ts */ `
import { expect, onTestFinished, test } from "bun:test";
Comment thread
claude[bot] marked this conversation as resolved.
Outdated

const runs: string[] = [];

test.concurrent("a", () => {
onTestFinished(() => { runs.push("a-finished"); });
expect(1).toBe(1);
});

test.concurrent("b", () => {
onTestFinished(() => { runs.push("b-finished"); });
expect(1).toBe(1);
});

test.concurrent("c", () => {
onTestFinished(() => { runs.push("c-finished"); });
expect(1).toBe(1);
});

test("report", () => {
runs.sort();
expect(runs).toEqual(["a-finished", "b-finished", "c-finished"]);
});
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "test", "concurrent.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]);
const output = stdout + stderr;
expect(output).not.toContain("Cannot call onTestFinished");
expect(output).toMatch(/\b4 pass\b/);
expect(output).toMatch(/\b0 fail\b/);
expect(exitCode).toBe(0);
});

test("onTestFinished works inside concurrent tests via --concurrent flag", async () => {
using dir = tempDir("issue-29236-on-test-finished-cli", {
"plain.test.ts": /* ts */ `
import { expect, onTestFinished, test } from "bun:test";
Comment thread
claude[bot] marked this conversation as resolved.

test("one", () => {
onTestFinished(() => {});
expect(1).toBe(1);
});

test("two", () => {
onTestFinished(() => {});
expect(1).toBe(1);
});
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "test", "--concurrent", "plain.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]);
const output = stdout + stderr;
expect(output).not.toContain("Cannot call onTestFinished");
expect(output).toMatch(/\b2 pass\b/);
expect(output).toMatch(/\b0 fail\b/);
expect(exitCode).toBe(0);
});

// When onTestFinished() is called in a concurrent test *after* control has
// returned to the event loop (e.g. after a timer-based await), the
// synchronous push/pop stack has already been popped, so the hook cannot
// resolve which concurrent sequence it belongs to. The error message should
// tell the user to hoist the call before the first await — not to remove
// .concurrent or use describe(), which was the pre-fix wording.
test("error message after yielding await in concurrent test tells users to hoist", async () => {
using dir = tempDir("issue-29236-after-await-error", {
"after-await.test.ts": /* ts */ `
import { onTestFinished, test } from "bun:test";
test.concurrent("a", async () => {
await Bun.sleep(5);
onTestFinished(() => {});
});
test.concurrent("b", async () => {
await Bun.sleep(5);
onTestFinished(() => {});
});
`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "after-await.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]);
const output = stdout + stderr;
// The legacy wording told users to switch to test.serial — that's wrong
// after this fix; synchronous registration works fine.
expect(output).not.toContain("Use test.serial");
expect(output).toMatch(/before the first `?await`?/);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
expect(exitCode).not.toBe(0);
});
Loading