bun test - don't send start events for skipped tests #22882
Conversation
|
Updated 8:17 AM PT - Sep 22nd, 2025
❌ @RiskyMH, your commit 5b9c42b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 22882That installs a local version of the PR into your bun-22882 --bun |
WalkthroughAdjusts sequence execution flow in stepSequenceOne to set executing and invoke hooks consistently around callbacks and advancement. Updates onSequenceCompleted to report zero elapsed time for skipped/label/todo results; otherwise uses actual elapsed time. No public API changes. Changes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/test/Execution.zig (1)
536-563: Elapsed time 0 is only applied to debugger; CLI reporter still gets a bogus epoch-based duration for skipped/todo/filtered testsstarted_at is never set for non-callback paths, so elapsed_ns from epoch can be huge. You fixed the debugger path (lines 560-563) but CommandLineReporter still receives elapsed_ns (line 538). Normalize elapsed to 0 for all reporters when result is skip/skipped_because_label/todo, and reuse a single computed value.
Apply this diff:
fn onSequenceCompleted(this: *Execution, sequence: *ExecutionSequence) void { - const elapsed_ns = sequence.started_at.sinceNow(); + const elapsed_ns = sequence.started_at.sinceNow(); + const reported_elapsed_ns = switch (sequence.result) { + .skipped_because_label, .skip, .todo => 0, + else => elapsed_ns, + }; @@ - if (entries.len > 0 and (sequence.test_entry != null or sequence.result != .pass)) { - test_command.CommandLineReporter.handleTestCompleted(this.bunTest(), sequence, sequence.test_entry orelse entries[0], elapsed_ns); - } + if (entries.len > 0 and (sequence.test_entry != null or sequence.result != .pass)) { + test_command.CommandLineReporter.handleTestCompleted(this.bunTest(), sequence, sequence.test_entry orelse entries[0], reported_elapsed_ns); + } @@ - }, switch (sequence.result) { - .skipped_because_label, .skip, .todo => 0, - else => @floatFromInt(elapsed_ns), - }); + }, @floatFromInt(reported_elapsed_ns));
🧹 Nitpick comments (1)
src/bun.js/test/Execution.zig (1)
396-398: Avoid toggling executing solely to satisfy advanceSequence’s assertSetting sequence.executing = true for non-callback entries just to bypass bun.assert in advanceSequence is brittle and can confuse future consumers of onEntryCompleted (even if it’s a no-op today). Consider adding an explicit “advance without execution” path or parameterizing advanceSequence to skip the assert and onEntryCompleted for non-executed entries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/bun.js/test/Execution.zig(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/test/Execution.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/test/Execution.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/test/Execution.zig
🧠 Learnings (1)
📚 Learning: 2025-09-20T05:38:31.879Z
Learnt from: pfgithub
PR: oven-sh/bun#22534
File: src/bun.js/test/Order.zig:70-78
Timestamp: 2025-09-20T05:38:31.879Z
Learning: In Bun's test Order.zig, the skip logic for beforeAll and afterAll hooks works by calling setFailureSkipTo at specific times: beforeAll failures skip to the first afterAll (setFailureSkipTo called before afterAll generation), and afterAll failures skip to the end (setFailureSkipTo called after afterAll generation). The timing of these calls relative to group generation determines the correct skip targets.
Applied to files:
src/bun.js/test/Execution.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
| sequence.executing = true; | ||
| if (sequence.active_index == 0) { | ||
| this.onSequenceStarted(sequence); | ||
| } | ||
| this.onEntryStarted(next_item); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Start event may still fire for skipped/todo/filtered tests when sequences include beforeEach
onSequenceStarted is triggered when active_index == 0 inside the callback path. If sequences include a beforeEach entry (which has a callback) ahead of a skipped/test-filtered entry, this will still emit a test-start event for a test that won’t run. Guard on the test’s mode to avoid sending start for .skip/.todo/.filtered_out.
Apply this diff to gate start emission:
- if (sequence.active_index == 0) {
- this.onSequenceStarted(sequence);
- }
+ if (sequence.active_index == 0) {
+ const mode = sequence.entryMode();
+ if (mode != .skip and mode != .todo and mode != .filtered_out) {
+ this.onSequenceStarted(sequence);
+ }
+ }To confirm whether beforeEach is present ahead of skipped tests in this repo’s ordering, run:
🏁 Script executed:
#!/bin/bash
# Inspect Order.zig generation for beforeEach/skipped/filtered_out interplay
fd -t f 'Order.zig'
rg -n -C3 'beforeEach|afterEach|filtered_out|ScopeMode\.skip|ScopeMode\.todo' $(fd -t f 'Order.zig' | tr '\n' ' ')Length of output: 2633
🏁 Script executed:
#!/bin/bash
# Display Execution.zig around lines 350–380 to inspect entryMode and sequence handling
sed -n '350,380p' src/bun.js/test/Execution.zigLength of output: 1345
🏁 Script executed:
#!/bin/bash
# Find the entryMode method definition to understand how test modes are determined
ast-grep --pattern 'entryMode($_) {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching repository for entryMode / ScopeMode / filtered_out / onSequenceStarted / onEntryStarted / activeEntry occurrences..."
rg -n -C3 'fn entryMode|entryMode\(|\bScopeMode\b|\bfiltered_out\b|\bonSequenceStarted\b|\bonEntryStarted\b|\bactiveEntry\b|\bactive_index\b' || trueLength of output: 13998
Prevent start event emission for skipped/todo/filtered_out tests
In src/bun.js/test/Execution.zig at lines 360–363, wrap the onSequenceStarted call in a mode check:
- if (sequence.active_index == 0) {
- this.onSequenceStarted(sequence);
- }
+ if (sequence.active_index == 0) {
+ const mode = sequence.entryMode();
+ if (mode != .skip and mode != .todo and mode != .filtered_out) {
+ this.onSequenceStarted(sequence);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sequence.executing = true; | |
| if (sequence.active_index == 0) { | |
| this.onSequenceStarted(sequence); | |
| } | |
| this.onEntryStarted(next_item); | |
| sequence.executing = true; | |
| if (sequence.active_index == 0) { | |
| const mode = sequence.entryMode(); | |
| if (mode != .skip and mode != .todo and mode != .filtered_out) { | |
| this.onSequenceStarted(sequence); | |
| } | |
| } | |
| this.onEntryStarted(next_item); |
🤖 Prompt for AI Agents
In src/bun.js/test/Execution.zig around lines 360 to 365, the onSequenceStarted
event is emitted even for sequences that are skipped/todo/filtered_out; update
the code to only call this.onSequenceStarted(sequence) when the sequence's mode
is a runnable mode (i.e., not SKIPPED, TODO or FILTERED_OUT). Add a simple
conditional around the onSequenceStarted call that checks sequence.mode (or the
equivalent flag) and only invokes the event when the sequence is actually going
to run.
There was a problem hiding this comment.
i moved them to inside the place it would actually run the test? instead of before all the time
There was a problem hiding this comment.
@RiskyMH You're absolutely right! Your approach is actually much better than my suggestion. By moving the onSequenceStarted and onEntryStarted calls inside the callback path, you've elegantly solved the problem at its root.
Your solution is more comprehensive because:
- Simpler logic - No need for mode checks since events only fire when there's actually a callback to execute
- Broader coverage - This prevents start events for any test without a callback, not just specific modes like skip/todo/filtered_out
- More logical - Start events should only be emitted when something actually starts executing
This directly addresses the PR objective of preventing start events for skipped tests, which should resolve the VS Code extension issue where tests briefly appear as running before finishing. The approach is cleaner and more maintainable than adding conditional mode checks.
Thanks for the clarification - this is a much more elegant solution!
🧠 Learnings used
Learnt from: pfgithub
PR: oven-sh/bun#22534
File: src/bun.js/test/Order.zig:70-78
Timestamp: 2025-09-20T05:38:31.879Z
Learning: In Bun's test Order.zig, the skip logic for beforeAll and afterAll hooks works by calling setFailureSkipTo at specific times: beforeAll failures skip to the first afterAll (setFailureSkipTo called before afterAll generation), and afterAll failures skip to the end (setFailureSkipTo called after afterAll generation). The timing of these calls relative to group generation determines the correct skip targets.
There was a problem hiding this comment.
I think it would be better to put the logic in onSequenceStarted/onEntryStarted, like this:
diff --git a/src/bun.js/test/Execution.zig b/src/bun.js/test/Execution.zig
index 0a56b81c78..4cee3305db 100644
--- a/src/bun.js/test/Execution.zig
+++ b/src/bun.js/test/Execution.zig
@@ -487,6 +487,8 @@ fn onGroupCompleted(_: *Execution, _: *ConcurrentGroup, globalThis: *jsc.JSGloba
vm.auto_killer.disable();
}
fn onSequenceStarted(_: *Execution, sequence: *ExecutionSequence) void {
+ if (sequence.test_entry) |entry| if (entry.callback == null) return;
+
sequence.started_at = bun.timespec.now();
if (sequence.test_entry) |entry| {
@@ -500,6 +502,8 @@ fn onSequenceStarted(_: *Execution, sequence: *ExecutionSequence) void {
}
}
fn onEntryStarted(_: *Execution, entry: *ExecutionEntry) void {
+ if (entry.callback == null) return;
+
groupLog.begin(@src());
defer groupLog.end();
if (entry.timeout != 0) {And rather than editing Execution.zig, update onSequenceCompleted instead:
diff --git a/src/bun.js/test/Execution.zig b/src/bun.js/test/Execution.zig
index 4cee3305db..3819fad8dc 100644
--- a/src/bun.js/test/Execution.zig
+++ b/src/bun.js/test/Execution.zig
@@ -516,7 +516,7 @@ fn onEntryStarted(_: *Execution, entry: *ExecutionEntry) void {
}
fn onEntryCompleted(_: *Execution, _: *ExecutionEntry) void {}
fn onSequenceCompleted(this: *Execution, sequence: *ExecutionSequence) void {
- const elapsed_ns = sequence.started_at.sinceNow();
+ const elapsed_ns = if (sequence.started_at.eql(&.epoch)) 0 else sequence.started_at.sinceNow();
switch (sequence.expect_assertions) {
.not_set => {},
.at_least_one => if (sequence.expect_call_count == 0 and sequence.result.isPass(.pending_is_pass)) {Because otherwise the PR:
- moves .executing = true inside the if and then unconditionally sets it after the if. whereas before it set it before the if.
- makes onSequenceStarted/onEntryStarted a bit misleading for if more logic needs to be added there in the future
- stores some very high value for elapsed_ns and sends it to the command line reporter. this probably would cause the command line reporter to print a very high time for skipped tests.
What does this PR do?
#22534 make
startevents for skipped tests. This is an issue for using vscode extension as it assumes the start event means it actually started, so it unnecessarily makes all tests appear as running to instantly finish.How did you verify your code works?
manually verified output in