-
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 all 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 |
|---|---|---|
|
|
@@ -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)}); | ||
| }; | ||
|
|
||
claude[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
@@ -219,6 +227,17 @@ 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 `onTestFinished()` resolves | ||
| /// to the correct test. `expect.assertions()` / `expect.hasAssertions()` | ||
| /// / snapshot matchers intentionally do not use this stack — see | ||
| /// `expect.zig` for why they reject concurrent-test calls outright. | ||
| current_callback_stack: std.array_list.Managed(RefDataValue), | ||
|
|
||
| phase: enum { | ||
| collection, | ||
|
|
@@ -253,6 +272,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 +288,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(); | ||
|
|
||
| this.execution.deinit(); | ||
| this.collection.deinit(); | ||
|
|
@@ -350,6 +374,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 = .{} }; | ||
|
|
@@ -384,6 +418,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.
Show resolved
Hide resolved
|
||
| pub fn ref(this_strong: BunTestPtr, phase: RefDataValue) *RefData { | ||
| group.begin(@src()); | ||
| defer group.end(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.