-
Notifications
You must be signed in to change notification settings - Fork 4.3k
test: allow onTestFinished() and expect.assertions() in concurrent tests #29238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
dd5566a
676ecb3
cb7cab7
ba2055d
91e2c5b
776ec85
e3dc2d6
e15c068
5042682
64eb666
2d732d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -219,6 +219,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, | ||
|
|
@@ -253,6 +262,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 { | ||
|
|
@@ -268,6 +278,7 @@ pub const BunTest = struct { | |
| entry.destroy(this.gpa); | ||
| } | ||
| this.extra_execution_entries.deinit(); | ||
| this.current_callback_stack.deinit(); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| this.execution.deinit(); | ||
| this.collection.deinit(); | ||
|
|
@@ -350,6 +361,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 = .{} }; | ||
|
claude[bot] marked this conversation as resolved.
|
||
|
|
@@ -384,6 +405,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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The 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 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:
|
||
| 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(); | ||
| } | ||
|
claude[bot] marked this conversation as resolved.
|
||
| pub fn ref(this_strong: BunTestPtr, phase: RefDataValue) *RefData { | ||
| group.begin(@src()); | ||
| defer group.end(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| // https://github.com/oven-sh/bun/issues/29236 | ||
| // | ||
| // onTestFinished() must be usable from inside a concurrent test, just like | ||
| // a plain try/finally cleanup block is. Before the fix, any onTestFinished() | ||
| // call from a concurrent test threw: | ||
| // | ||
| // Cannot call onTestFinished() here. It cannot be called inside a | ||
| // concurrent test. Use test.serial or remove test.concurrent. | ||
| // | ||
| // because the hook lookup couldn't resolve which sequence owned the call | ||
| // when more than one concurrent sequence was active. | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| 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"; | ||
|
claude[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| const runs: string[] = []; | ||
|
|
||
| test.concurrent("a", async () => { | ||
| onTestFinished(() => { runs.push("a-finished"); }); | ||
| await new Promise<void>(r => setTimeout(r, 10)); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| expect(1).toBe(1); | ||
| }); | ||
|
|
||
| test.concurrent("b", async () => { | ||
| onTestFinished(() => { runs.push("b-finished"); }); | ||
| await new Promise<void>(r => setTimeout(r, 10)); | ||
| expect(1).toBe(1); | ||
| }); | ||
|
|
||
| test.concurrent("c", async () => { | ||
| onTestFinished(() => { runs.push("c-finished"); }); | ||
| await new Promise<void>(r => setTimeout(r, 10)); | ||
| 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).toContain("4 pass"); | ||
| expect(output).toContain("0 fail"); | ||
| expect(exitCode).toBe(0); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| }); | ||
|
|
||
| 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"; | ||
|
Check warning on line 65 in test/regression/issue/29236.test.ts
|
||
|
claude[bot] marked this conversation as resolved.
|
||
|
|
||
| test("one", async () => { | ||
| onTestFinished(() => {}); | ||
| await new Promise<void>(r => setTimeout(r, 5)); | ||
| expect(1).toBe(1); | ||
| }); | ||
|
|
||
| test("two", async () => { | ||
| onTestFinished(() => {}); | ||
| await new Promise<void>(r => setTimeout(r, 5)); | ||
| 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).toContain("2 pass"); | ||
| expect(output).toContain("0 fail"); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.