bun test: nest results under describe scopes, with attributed failures and console output - #35794
Open
jakeboone02 wants to merge 7 commits into
Open
bun test: nest results under describe scopes, with attributed failures and console output#35794jakeboone02 wants to merge 7 commits into
jakeboone02 wants to merge 7 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe test reporter now buffers results per file, renders nested ChangesNested test reporter output
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/cli/test/parallel/Coordinator.rs (1)
394-411: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNon-dots parallel runs now emit two headers for any file that writes to stdout/stderr.
flush_captured(w)→ensure_header(idx)prints a bare\n<path>:\nabove the captured console output, andflush_block(idx)later prints its own\n<glyph> <path>:\nabove the buffered rows for the same file. The serial path avoids this becauseCurrentFile::setsetshas_printed_filename = truesoprint_if_neededstays silent while the block is pending. Consider suppressingensure_header(or the block header) when a buffered block already exists for that index.🔧 Sketch
fn ensure_header(&mut self, file_idx: u32) { if self.dots { return; } + // The buffered block will print its own glyph-carrying header. + if self.blocks.iter().any(|(idx, _)| *idx == file_idx) { + return; + } if self.last_header_idx == Some(file_idx) {Note this only helps once a row has been buffered; captured output arriving before the first
TestDonestill needs a header, so you may instead want to suppress the duplicate inflush_blockwhenlast_header_idx == Some(file_idx).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/cli/test/parallel/Coordinator.rs` around lines 394 - 411, Prevent duplicate file headers in non-dots parallel runs by coordinating flush_captured and flush_block for the same test index. Update the relevant header-tracking logic around ensure_header, flush_captured, and flush_block so a buffered block reuses the header already emitted for captured output, while captured output before any buffered row still receives its required header.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/cli/test_command.rs`:
- Around line 422-440: Move high-water tracking from clear() into push(),
updating peak_bytes and peak_rows after each insertion based on the current
buffered rows. Keep clear() focused on removing buffered data, and preserve
log_peak() so both flush_file_block and Coordinator::flush_block report the
block currently being flushed, including moved-out coordinator blocks.
- Around line 278-290: Update the worst method’s todo branch to follow the
documented precedence by removing the self.skip exclusion, so any scope with
todo tests and no failures or passes returns R::Todo even when skipped tests are
also present.
In `@test/cli/test/bun-test.test.ts`:
- Around line 1677-1703: Add mixed-status describe groups to the test case
around the existing aggregate-status assertions, including pass+todo and
todo+skip combinations, and assert their expected aggregate statuses. Preserve
the existing fail, all-skipped, all-todo, and all-passing coverage while
ensuring the assertions validate the complete status-precedence matrix.
- Around line 1742-1753: Update the group-header lookup in the test around
groups and resultLines so it compares against trimmed output lines before
calling indexOf. Preserve the existing assertions that each group header is
followed by tests “one” and “two”.
In `@test/cli/test/parallel.test.ts`:
- Around line 240-251: Replace fixed sleeps with a shared observable barrier in
test/cli/test/parallel.test.ts lines 240-251, ensuring both fixture files have
begun before releasing fast and slow completions. In
test/cli/test/bun-test.test.ts lines 1727-1753, coordinate both fixture files
through an awaited observable condition before asserting describe-group
contiguity; do not rely on assumed worker overlap.
---
Outside diff comments:
In `@src/runtime/cli/test/parallel/Coordinator.rs`:
- Around line 394-411: Prevent duplicate file headers in non-dots parallel runs
by coordinating flush_captured and flush_block for the same test index. Update
the relevant header-tracking logic around ensure_header, flush_captured, and
flush_block so a buffered block reuses the header already emitted for captured
output, while captured output before any buffered row still receives its
required header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 017b592f-b14e-4f04-a4e2-d652d81209e6
⛔ Files ignored due to path filters (2)
test/cli/test/__snapshots__/claudecode-flag.test.ts.snapis excluded by!**/*.snaptest/js/bun/test/__snapshots__/test-test.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (28)
docs/guides/test/coverage-threshold.mdxdocs/guides/test/coverage.mdxdocs/test/reporters.mdxsrc/runtime/cli/test/parallel/Coordinator.rssrc/runtime/cli/test/parallel/Frame.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rssrc/runtime/test_runner/bun_test.rssrc/runtime/test_runner/jest.rstest/cli/test/bun-test.test.tstest/cli/test/coverage.test.tstest/cli/test/parallel.test.tstest/cli/test/test-filter-lifecycle-snapshot.test.tstest/js/bun/test/bun_test.test.tstest/js/bun/test/concurrent.test.tstest/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/describe.test.tstest/js/bun/test/only-failures.test.tstest/js/bun/test/printing/diffexample.test.tstest/js/bun/test/stack.test.tstest/js/bun/test/test-error-code-done-callback.test.tstest/js/bun/test/test-failing.test.tstest/js/bun/test/test-test.test.tstest/regression/issue/11793.test.tstest/regression/issue/19875.test.tstest/regression/issue/20092.test.tstest/regression/issue/20980.test.tstest/regression/issue/23865.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/test/parallel.test.ts`:
- Around line 4-21: Deduplicate the barrier fixture helper by keeping
test/cli/test/parallel.test.ts lines 4-21 as the canonical barrier
implementation, or moving it to test/harness.ts and importing it there; remove
the duplicate barrier() definition at test/cli/test/bun-test.test.ts lines
1654-1671 and import the shared helper instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 359da9e1-7441-4085-b1b0-9e068f576278
📒 Files selected for processing (3)
src/runtime/cli/test_command.rstest/cli/test/bun-test.test.tstest/cli/test/parallel.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/harness.ts`:
- Around line 451-469: Update parallelBarrierFixture so that after its wait
loop, it re-evaluates which entries in peers still lack barrier files; if the
deadline has been reached with missing peers, throw an error identifying me, the
missing peers, and the deadline, while preserving normal continuation once all
peers are present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 785c7520-232d-4063-917f-a447589713bd
📒 Files selected for processing (3)
test/cli/test/bun-test.test.tstest/cli/test/parallel.test.tstest/harness.ts
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jakeboone02
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/cli/test/parallel/Coordinator.rs (1)
153-176: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSIGINT/SIGTERM abort path drops buffered per-file output.
abort_all()kills every worker and callsGlobal::exit(130)without ever callingself.flush_all_blocks().bail_out()(line 275) explicitly flushes all buffered blocks before printing its own error, precisely to avoid losing already-computed results now that output is buffered per file instead of streamed immediately.abort_all()doesn't get that same treatment, so a user hitting Ctrl-C mid-run will see less output than they would have pre-buffering (in-flight files' already-completed sub-test results are silently discarded instead of shown).🐛 Proposed fix
fn abort_all(&mut self) -> ! { abort_handler::uninstall(); + self.flush_all_blocks(); for w in self.workers[..self.spawned_count as usize].iter_mut() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/cli/test/parallel/Coordinator.rs` around lines 153 - 176, Update Coordinator::abort_all to call self.flush_all_blocks() before terminating with Global::exit(130), after stopping the workers so already-buffered per-file results are emitted on SIGINT/SIGTERM. Preserve the existing worker termination and cleanup flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bun_core/util.rs`:
- Around line 1755-1802: Add a concise safety note to the VecWriter::interface
documentation stating that the VecWriter must not be moved while the returned
Writer reference or pointer remains outstanding, since callers may retain its
address. Keep the existing field-order and vtable-view documentation unchanged.
In `@src/runtime/test_runner/jest.rs`:
- Around line 146-151: Remove the contradictory first line from the doc comment
above TestRunner’s write_path method. Document that write_path emits only the
file’s title/display path without the prefix, while preserving the existing
implementation and explanation that the GitHub Actions group marker is handled
separately.
In `@test/cli/test/bun-test.test.ts`:
- Around line 2023-2039: Replace the fixed timer-based delay in the “unhandled
errors between tests name their file” test with an observable synchronization
signal: have the timer callback schedule the stray throw separately, then
resolve a promise signaling that the callback ran, and await that signal before
asserting stderr. Preserve the separate ordering needed for the reporter to
attribute the error between tests, without relying on wall-clock sleep.
- Around line 2012-2018: Update the Bun.spawn options in the process.exit()
subprocess test to set stdout to "ignore" or explicitly drain proc.stdout
alongside stderr and proc.exited, ensuring the child cannot block on an unread
stdout pipe.
In `@test/regression/issue/20980.test.ts`:
- Line 21: Update render_failure_report in src/runtime/cli/test_command.rs to
pluralize the failure-section header conditionally, using singular “test
failed:” when bail equals 1 and plural “tests failed:” otherwise, consistent
with the neighbouring bail message; then re-record the affected snapshot.
---
Outside diff comments:
In `@src/runtime/cli/test/parallel/Coordinator.rs`:
- Around line 153-176: Update Coordinator::abort_all to call
self.flush_all_blocks() before terminating with Global::exit(130), after
stopping the workers so already-buffered per-file results are emitted on
SIGINT/SIGTERM. Preserve the existing worker termination and cleanup flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4befcdc7-cc82-4090-976b-e5d8a17649eb
⛔ Files ignored due to path filters (1)
test/cli/test/__snapshots__/claudecode-flag.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (33)
src/bun_core/util.rssrc/jsc/ConsoleObject.rssrc/jsc/VirtualMachine.rssrc/runtime/cli/test/ParallelRunner.rssrc/runtime/cli/test/parallel/Coordinator.rssrc/runtime/cli/test/parallel/Frame.rssrc/runtime/cli/test/parallel/runner.rssrc/runtime/cli/test_command.rssrc/runtime/jsc_hooks.rssrc/runtime/test_runner/bun_test.rssrc/runtime/test_runner/jest.rstest/cli/test/bun-test.test.tstest/cli/test/test-filter-lifecycle-snapshot.test.tstest/js/bun/test/bun_test.test.tstest/js/bun/test/concurrent.test.tstest/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/dots.test.tstest/js/bun/test/only-failures.test.tstest/js/bun/test/only-inside-only.test.tstest/js/bun/test/printing/diffexample.test.tstest/js/bun/test/test-error-code-done-callback.test.tstest/js/bun/test/test-test.test.tstest/regression/issue/08964/08964.test.tstest/regression/issue/11793.test.tstest/regression/issue/14135.test.tstest/regression/issue/19758.test.tstest/regression/issue/19850/19850.test.tstest/regression/issue/20100.test.tstest/regression/issue/20980.test.tstest/regression/issue/21177.test.tstest/regression/issue/21830.test.tstest/regression/issue/5738.test.tstest/regression/issue/5961.test.ts
# Conflicts: # src/runtime/cli/test/parallel/Coordinator.rs # src/runtime/cli/test/parallel/runner.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3342, closes #12378, closes #2109
Note
This is a proof-of-concept. I don't expect it to be merged as-is — it changes the shape of
bun test's output, touches the VM's error-printing path, and trades a constant memory floor for readability. It's here to make the aesthetic argument concrete and to give something specific to react to. Happy to cut scope, split it, or close it.What changed
1. Results nest under their
describescopes, buffered per file.Each file's rows are collected and rendered as one contiguous tree when the file finishes. A parent shows its aggregate status, so a failure is visible at every level without scanning.
2. Failure diagnostics are batched and attributed.
Diffs and stack traces no longer print where the failure happened — they're collected and rendered at the end of the run under a header naming exactly which file and test they belong to:
3.
console.*output gets a Vitest-style attribution header.Emitted only when the producing test changes, on the same stream as the output it labels (
stdoutforlog/info/debug,stderrforwarn/error).Why 2 and 3 are part of this PR
Buffering the tree (change 1 alone) breaks attribution. Anything written while a file runs — a diff, a
console.log— flushes before the block that would identify it. In a multi-file run you get two identical-looking error blocks and no way to tell them apart. Changes 2 and 3 are the cost of change 1, not extras.Prior art
This follows Vitest's two-phase model — inline per-file results, diagnostics batched into an end-of-run section,
stdout | file > describe > testfor console output — while keeping Bun's plainer visual vocabulary. No⎯⎯⎯dividers, no[i/n]rules, noFAILbadges; the existing(pass)/(fail)/(skip)/(todo)glyphs and theN tests failed:heading are unchanged.Tradeoffs
Memory
This is the main cost, and it is a real one.
So peak RSS grows by roughly (largest file's tree) + (total failure diagnostics, capped at 32 MB). For a healthy suite that's kilobytes. For a suite where thousands of tests fail with large object diffs, it's tens of megabytes held until the run ends — where before it was streamed and freed. Both buffers log their high-water mark under
BUN_DEBUG_testreporter=1.The 32 MB cap is currently a hard constant. Making it a
BUN_CONFIG_*env var is an open question.Latency and durability
process.exit(),--bail, and worker-crash teardown are all covered by an explicit flush; SIGKILL is not.Deviations and known gaps
--reporter=junit(it costs a call pertest()), so using it here would be a perf regression for every run. The throw site is free and arguably more useful.console.*is labeled. Rawprocess.stdout.write,Bun.write(Bun.stdout), and subprocess output stay unlabeled.stdout | fileheader rather than guessing.--parallel: the coordinator's glyph-less file header on the captured-output path is gone, since workers now emit their own labels. Unlabeled raw writes lose that header.Implementation notes
VirtualMachine::error_writer_overrideredirectsprint_exceptioninto aVec<u8>-backedio::Writerfor the duration of onerun_error_handler, following the existingon_print_error_zig_exception{,_ctx}set/call/clear pattern on the same lines.on_print_error_zig_exceptionslot exists; the reporter now owns it and forwards to JUnit.JunitReporter::record_failure_cbis removed.--parallelgains one frame kind (FailureDiagnostic); the coordinator owns the run-level report. Oversized payloads are truncated by the existingFrame::strguard..todotests that throw still report inline — their error is expected output, not a failure — but now carry a(todo) file:line > nameheader.N tests failed:name list is replaced by the detailed section, and thepass > 20gate no longer suppresses failures (it still applies to skip/todo lists).Testing
test/cli/test/bun-test.test.tscovering batching, multi-file attribution, console headers on both streams, header de-duplication,--dots,--only-failures,--parallel,--bail,process.exit()mid-run, the unhandled-error banner, and.todo. All 12 fail underUSE_SYSTEM_BUN=1and pass underbun bd test.test/cli/test/,test/js/bun/test/, andtest/regression/issue/.test/cli/test/+test/js/bun/test/and the touched regression files are at or below their pre-change failure baseline (this also fixes Possible Memory Safety Issue in bun test / symbol resolution #19850's test, which the buffering in change 1 had broken).