Conversation
… --bail Snapshots are buffered in memory and only written back when the next file opens its own .snap or when the run ends. Both --bail exits skipped that write, so the .snap of the file being run (or of the previously run file) was left as the empty file created when it was opened; with --update-snapshots that empty file replaced the committed snapshots. Pending inline snapshots were dropped the same way.
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: fix pushed (head a09511d), waiting on CI. Reproduced on bun 1.4.0 and on main. A file whose first snapshot assertion runs under After self-review the PR was reshaped from a bail-only flush into: open the Tests: All review threads so far are addressed (984da5a, f0778dd, a09511d). PR: #38974 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix that flushes buffered snapshots on both --bail exit paths, mirroring the end-of-run write. One inline nit on the test helper's undrained stdout pipe; not blocking.
Extended reasoning...
Overview
This PR fixes bun test --bail to write pending .snap files and inline snapshots before exiting. Without it, the .snap file (which is opened with O_CREAT — and O_TRUNC under -u — on the first snapshot assertion) is left as a 0-byte file on disk when bail fires, and pending toMatchInlineSnapshot() writes are dropped. The fix adds a write_snapshots_before_bail() helper on CommandLineReporter that calls snapshots.write_inline_snapshots() and snapshots.write_snapshot_file() (reporting write errors via Output::err, exit code already 1), and invokes it at both bail exit sites in test_command.rs — right alongside the existing write_junit_report_if_needed() / write_timings_if_needed() calls that exist there for the identical reason. Five new test.concurrent cases in new-snapshot.test.ts cover the bailing file, --update-snapshots over an existing .snap, an earlier file's .snap when a later file fails or fails to load, and pending inline snapshots.
Security risks
None. This touches only the test runner's snapshot-flush timing on an already-failing exit path; no untrusted input parsing, auth, or network surface is involved.
Level of scrutiny
Low-to-moderate. The Rust change is ~17 lines and purely additive: it calls the same two functions, in the same order, that the normal end-of-run path already calls at test_command.rs:2740/2744. Both are no-ops when there is nothing pending (write_snapshot_file guards on _current_file.take(); write_inline_snapshots iterates an empty map), so there is no risk of a spurious write. The placement follows the established pattern at both sites exactly (same neighbours, same ordering relative to JUnit/timings). --parallel is unaffected as the description notes — workers don't receive --bail and flush in worker_flush_aggregates.
Other factors
- Tests cover the full variant matrix the PR description enumerates, use
tempDir+bunEnv+bunExe(), assert the exact.snapbytes (header + entry) rather than just existence, and assert the bail message and exit code. They aretest.concurrentand each spawns one short-lived subprocess. - The one finding is a nit:
runUntilBailsetsstdout: "pipe"but never reads it.bun testwrites its reporter output to stderr and the fixtures print nothing to stdout, so there's no deadlock risk here — it's a harness-convention deviation, not a functional bug. - I confirmed there are exactly two "Bailed out after" exit sites in
test_command.rsand both are covered. - No prior reviewer comments to address; no CODEOWNERS on these paths.
|
Updated 11:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit a09511d has some failures in 🧪 To try this PR locally: bunx bun-pr 38974That installs a local version of the PR into your bun-38974 --bun |
Bailing on a failed test exits without tearing the VM down, so under the ASAN lane's detect_leaks=1 the child aborts with 134 instead of exiting 1 regardless of what this change does; the tests check what reaches disk before that exit.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — both prior nits (stdout drain, doc-comment length) are addressed in the current head.
What was reviewed:
- Confirmed only two serial-runner bail exits exist (
handle_test_completedand the rejected-entry-point branch); both now flush snapshots alongside the existing JUnit/timings writes. write_snapshots_before_bailcalls the same two functions in the same order as the run-end path atTestCommand::exec;write_snapshot_file()uses.take()so it is a no-op with no open file.- Tests cover both bail paths (test failure and load failure),
-uover an existing.snap, and inline snapshots; the LSAN opt-out is scoped todetect_leaks=0with a stated reason.
Extended reasoning...
Overview
Adds CommandLineReporter::write_snapshots_before_bail() (8 lines) and calls it at the two serial-runner --bail exit sites in src/runtime/cli/test_command.rs, immediately before the existing write_junit_report_if_needed() / write_timings_if_needed() calls that already sit there for the same reason. Adds a 5-test --bail describe block to test/js/bun/test/snapshot-tests/new-snapshot.test.ts. ~11 production lines, ~80 test lines.
Security risks
None. This is test-runner artifact flushing on an exit path that already writes JUnit reports and timings; no untrusted input parsing, no auth/crypto/permissions.
Level of scrutiny
Low-to-medium. The production change is mechanical: it mirrors the run-end path (write_inline_snapshots() then write_snapshot_file(), verified at lines ~2735–2739) and slots into an established sibling pattern at both bail sites. I grepped for jest.bail / .bail == and confirmed there are exactly two bail-exit sites in this file; both are patched. write_snapshot_file() takes _current_file.take(), so calling it with no open file is a safe no-op. Write errors are surfaced via Output::err and the process still exits 1.
Other factors
- All prior review feedback is addressed: 984da5a drains stdout in
runUntilBail(verified in diff), and f0778dd removed the doc comment that both comment-cop and my earlier review flagged. - Tests use
tempDir+using,test.concurrent, drain all three pipes, assert exact.snapbytes withtoBe, and assertexitCode === 1after content checks. The five cases hit both bail exit paths (failing test vs. throwing at load), the destructive-ucase, the earlier-file case, and inline snapshots. - The child-process
detect_leaks=0is narrowly scoped (ASAN itself stays on) and commented with the observed pre-existing failure mode, satisfying REVIEW.md's "every skip needs a comment with the observed failure" rule; the PR description notes it is filed separately. --parallelis unaffected per the PR description (workers finish and flush inworker_flush_aggregates); the change is confined to the serial reporter.
…and write it per file --update-snapshots truncated the .snap as soon as it was opened and only wrote it back at the next file's open or at the end of the run, so any exit in between (a bail, process.exit() in a test, a crash, a later file failing to load) left the committed snapshots as an empty file. Open the file without O_TRUNC and have write_snapshot_file() overwrite and ftruncate instead, and write each file's .snap when the file finishes so finished files no longer depend on how a later file ends. The bail exits keep writing the running file's snapshots and the pending inline snapshots.
robobun
changed the title
| // Per file, so this file's snapshots are on disk however a later file ends. | ||
| if let Err(err) = reporter.jest.snapshots.write_snapshot_file() { | ||
| Output::err(err.name(), "Failed to write snapshot file", ()); | ||
| return Err(err); | ||
| } |
There was a problem hiding this comment.
🟡 The per-file write_snapshot_file() at line 3421 both prints via Output::err and return Err(err); every caller of TestCommand::run routes that Err to handle_top_level_test_error_before_javascript_start (-> !, Global::exit(1)), so a pwrite/ftruncate failure after a file's tests have run hard-exits without JUnit/timings/summary or remaining files, and a --parallel worker exits before sending FileDone (which the coordinator reads as a crash — see the comment in build_worker_argv). This is inconsistent with write_snapshots_before_bail in the same PR, which prints the identical error and continues; drop the return Err(err) to match.
Extended reasoning...
What the bug is
The new per-file snapshot flush at test_command.rs:3421-3424 handles a write_snapshot_file() failure by printing it (Output::err(err.name(), "Failed to write snapshot file", ())) and propagating it (return Err(err)). But TestCommand::run's callers do not handle a returned Err gracefully:
run_all_testsattest_command.rs:3125andtest_command.rs:3154WorkerLoop::beginatparallel/runner.rs:571
All three route it to handle_top_level_test_error_before_javascript_start (test_command.rs:3429-3436), which is -> ! and calls Global::exit(1) unconditionally. So an I/O failure writing the .snap — after the file's tests have already run to completion — aborts the whole process.
The code path
- A test file finishes;
TestCommand::runreaches line 3421 and callsreporter.jest.snapshots.write_snapshot_file(). pwrite_allorftruncatefails (e.g.ENOSPC,EIO, filesystem gone read-only mid-run) →Err(FailedToWriteSnapshotFile).- Line 3422 prints "Failed to write snapshot file"; line 3423 returns
Err. - The caller passes it to
handle_top_level_test_error_before_javascript_start, whichGlobal::exit(1)— beforewrite_junit_report_if_needed/write_timings_if_needed/ the summary, and before any remaining test files run. In a--parallelworker the process exits before theFileDoneframe is sent, whichbuild_worker_argv's own comment says the coordinator "would misread as a crash". - In debug builds the error is also double-reported: once by
Output::errat 3422, again bydebug_warn!at 3432.
Why existing code doesn't prevent it
The handler is named ..._before_javascript_start and was designed for module-resolution-style failures that happen before a file's JS runs. This new Err is returned after the file's tests have all executed, so it lands in a handler semantically not meant for it. There is no other exit-time flush for JUnit/timings on this path — contrast the two bail exits at lines 1527-1529 and 3313-3315, which explicitly call write_junit_report_if_needed / write_timings_if_needed before Global::exit.
Why it's inconsistent within this PR
write_snapshots_before_bail (test_command.rs:1556-1563), added in the same PR, handles the identical error from the identical function with print-and-continue:
if let Err(err) = self.jest.snapshots.write_snapshot_file() {
Output::err(err, "Failed to write snapshot file", ());
}— no propagation, so JUnit/timings still get written on the very next lines. The per-file site should do the same.
It is also a behavior change vs. main: before this PR the equivalent write happened lazily inside get_snapshot_file() on the next file's first toMatchSnapshot(), where an error surfaced through get_or_put as a per-test JS exception (one assertion fails, the run continues). This PR upgrades the same underlying I/O failure to a whole-process abort.
Impact
Rare in practice — pwrite_all/ftruncate failing on an fd that was successfully opened O_RDWR moments earlier requires disk-full/quota/EIO/EROFS mid-run. Nothing breaks in normal operation, hence nit. But when it does trigger: the JUnit report and --timings file are lost (both of which the bail paths in this PR go out of their way to preserve), remaining test files are silently skipped, and under --parallel the coordinator reports the file as a worker crash rather than a snapshot-write failure.
Fix
Drop the return Err(err); at line 3423 to match write_snapshots_before_bail:
// Per file, so this file's snapshots are on disk however a later file ends.
if let Err(err) = reporter.jest.snapshots.write_snapshot_file() {
Output::err(err.name(), "Failed to write snapshot file", ());
}
Ok(())If a nonzero exit is desired for this case, bump a failure counter on the reporter/summary instead so the run finishes and JUnit/timings are still written.
…he full report (#41423) ### Problem - `test/regression/issue/26851.test.ts` takes up to 12s on the debian 13 x64-asan lane. It is in the slowest 5 percent of test files. Both tests spawn a `bun test --bail` child, and they ran one after the other. - On that lane each child exits 134, not 1. `bun test --bail` exits with a bare `exit(1)`, so the `BUN_DESTRUCT_VM_ON_EXIT` teardown is skipped and LeakSanitizer aborts the child after several seconds of report symbolization (#32183, fix open in #39010). Locally each child takes 0.3s. Under the lane environment it takes 3.8s. The old `expect(exitCode).not.toBe(0)` hid the abort. - The old assertions only checked that the JUnit file exists and contains four substrings. They did not check the counters, the failure element, the bail message, or the exit code. ### Fix - Add the file to `test/no-validate-leaksan.txt`, next to `test/regression/issue/12250.test.ts`, which is on the list for the same bail exit. The entry is to be removed when #39010 lands. Without LeakSanitizer the children exit 1 in well under a second. - Run both tests with `test.concurrent`. Each test owns its tempDir and its outfile, so the two children overlap. A shared `runBailWithJUnit` helper does the spawn and reads the report. - Assert the full JUnit document with `toMatchInlineSnapshot`: the `testsuites` root with `tests`, `assertions`, `failures` and `skipped`, one `testsuite` per file that ran, the `testcase` names and lines, and the `failure` element with its type and message. Only the `time` and `hostname` attributes are normalized. Assert the child output before the exit code: stdout is the version banner, stderr has the `(pass)`/`(fail)` lines, the `Ran N tests across M files.` summary and `Bailed out after 1 failure`. The exit code is exactly 1. - Add a second test after the failing one, and a third test file `c_never.test.ts`. Neither appears in stderr or in the report. This proves that `--bail` stopped the run and that the report holds only what ran. Discovery order of the root directory is sorted by name, so `a_pass` runs before `b_fail`. - Verified: `bun bd test test/regression/issue/26851.test.ts`. Plain, 3 runs: before 2.66s to 3.14s wall, after 2.27s to 2.44s wall. With the ASAN lane environment (`BUN_DESTRUCT_VM_ON_EXIT=1`, `detect_leaks=1`): before 9.53s with both children exiting 134, after the list entry that environment is not applied. Also passes with `USE_SYSTEM_BUN=1`. ### Background - `--bail` makes `bun test` stop after the first failing test. #26851 was about the JUnit outfile not being written in that case. - `test/no-validate-leaksan.txt` lists test files for which `scripts/runner.node.mjs` does not set `BUN_DESTRUCT_VM_ON_EXIT` and `detect_leaks=1` on the ASAN lane. - #33704 also adds `test.concurrent` to this file as part of a broad speed pass. This PR is scoped to the one file and also strengthens the assertions. <details><summary>Notes</summary> The first CI run (build 110365) failed on the x64-asan lane with exit code 134 on both tests and 8.7s per test. That is the LeakSanitizer abort described above. The fix for the bail exit itself is in #39010 and out of scope here. The stderr checks use `toContain` rather than a snapshot of the whole stream. Open PRs #39010, #39276, #38974 and #38265 change parts of the bail and reporter output, and a whole-stream snapshot would conflict with them for no gain. The XML snapshot is the thing under test. The failure body in the XML contains `at fail.test.ts:2:40`. The fixture sources are built from fixed strings, not indented template literals, so the column is stable. `test/js/junit-reporter/junit.test.js` snapshots the same shape on every platform. </details> <!-- robobun:evidence:begin --> --- **[auto-merge]** gate passed · iteration 1 · 2 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/regression/issue/26851.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "test/regression/issue/26851.test.ts" bun test v1.4.3 (e0a2b82) test/regression/issue/26851.test.ts: (pass) --bail writes JUnit reporter outfile [356.62ms] (pass) --bail writes JUnit reporter outfile with multiple files [349.11ms] 2 pass 0 fail 2 snapshots, 15 expect() calls Ran 2 tests across 1 file. [2.42s] Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/no-validate-leaksan.txt | 3 + test/regression/issue/26851.test.ts | 140 +++++++++++++++++++++--------------- 2 files changed, 87 insertions(+), 56 deletions(-) ``` </details> **gate history** · 3 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/no-validate-leaksan.txt 1 1 25 test/regression/issue/26851.test.ts 3 5 19 ``` </details> **root cause** · written by the author bot The regression test for issue #26851 was slow because each of its five tests spawned a separate `bun test` child one after another, and under ASAN every child paid several seconds of startup, while the assertions only checked that the JUnit outfile existed. The fix runs the independent cases concurrently through a shared runner that captures stdout, stderr, the exit code and the report once per fixture set, then asserts the full normalized JUnit document shape, the bail message, the summary line and the exact exit code. A follow-up strips the CI environment variables that make the reporter … <!-- robobun:evidence:end -->
This branch has not been deployed
1 participant
Problem
bun test -utruncates a test file's committed__snapshots__/<file>.snapto 0 bytes the moment the file's first snapshot assertion runs, and only writes the new contents back much later. Any exit in between destroys the committed snapshots:bun test -u --bailwith a failing test,process.exit()inside a test, a crash, or a later file that throws while loading all leave the file at 0 bytes (all reproduced on 1.4.0).-u, snapshots recorded during the run are lost the same way:.snapfiles are written back only when the next file opens its own.snapor when the whole run ends, so--bail(or a later file callingprocess.exit()) loses the snapshots of the file that was running and of every fully passing file before it whose.snaphad not been written yet; pendingtoMatchInlineSnapshot()writes are dropped on--bailtoo.Snapshots::get_snapshot_file()(src/runtime/test_runner/snapshot.rs) opens the.snapwithO_CREAT | O_RDWR, plusO_TRUNCunder-u, and buffers the contents in memory;write_snapshot_file()/write_inline_snapshots()run only on the next file's open and at the end ofTestCommand::exec. Both--bailexits insrc/runtime/cli/test_command.rs(handle_test_completedwhen the failure count reaches the bail count, and the rejected entry point branch ofTestCommand::run) exit before either runs.Fix
snapshot.rs: the.snapis no longer truncated when it is opened.write_snapshot_file()writes the buffer at offset 0 andftruncates to its length, so the file is only ever replaced by complete contents and whatever exit happens before that leaves the committed file intact. The Windows-onlyseek_to(0)after the initial read goes away because the write is positional now. (A.snapthat did not exist yet can still be left as an empty file by an abnormal exit; an empty file is treated as absent on the next run, so nothing is lost.)TestCommand::run: each file's.snapis written when the file finishes (serial runner and--parallelworkers both go throughrun), so however a later file ends, the finished files' snapshots are on disk. The writes at the next file's open and at the end of the run remain as no-ops. Inline snapshots stay end-of-run:write_inline_snapshots()is written to run once per process.--bail:CommandLineReporter::write_snapshots_before_bail()writes the pending inline snapshots and the.snapof the file that was running, called on both bail exits next to the JUnit and timings writes that already exist there. This persists what the run had recorded, the same as a run without--bailpersists the snapshots of a test that fails later;--baildecides when the process stops, not what is kept. Under-uthat is the snapshots of the tests that ran, which is what-ualready writes for any partial run (for example-u -t name).TestCommand::runthe call happens beforeJest::RUNNERis cleared, whichwrite_inline_snapshots()reads; the new inline test on that path covers it.test/js/bun/test/snapshot-tests/new-snapshot.test.ts. "writing the .snap file":-uwithprocess.exit()before the write keeps the committed file;-ushrinks a longer existing file; a file's.snapis written before a later fileprocess.exit()s. "--bail": the bailing file's.snap,-uover a longer existing.snap, the bailing file's inline snapshots, an earlier file's.snapwhen a later file fails or fails to load, and an earlier file's inline snapshots when a later file fails to load. 8 of the 9 fail on 1.4.0 (0 byte.snap/ unmodified source); the shrink test passes before and after and pins theftruncate. All pass with the debug build, also under the ASAN lane's LSAN environment.detect_leaks=0(ASAN stays on): bailing on a failed test exits without tearing the VM down, so LSAN reports the runner's JSC-owned objects and the child exits 134 instead of 1 regardless of this change. Reported separately; same precedent as other tests whose child exits without teardown.test/js/bun/test/snapshot-tests/(all files),test/cli/test/bun-test.test.ts -t bail,test/cli/test/parallel.test.ts -t bail,test/regression/issue/26851.test.ts,test/regression/issue/12250.test.ts.afterAll) also dropsO_TRUNC; whichever lands second drops that part. bun test: write coverage reports when --bail stops the run #38265 (coverage), bun test: run afterAll hooks when --bail stops a run #35504 (afterAll) and test: keep --watch alive when --bail stops a run #35094 (--watch) rework the two bail exits; the onewrite_snapshots_before_bail()call per exit moves into whatever replaces them.Background
.snaplifecycle: on a test file's first snapshot assertion,get_snapshot_file()opens__snapshots__/<file>.snap, loads the existing entries intofile_buf(under-uit starts from the header instead) and appends new entries tofile_bufas tests run;write_snapshot_file()writesfile_bufback and closes the file. Until it runs, the file on disk is the previous contents (new behaviour) rather than empty.toMatchInlineSnapshot()) are queued ininline_snapshots_to_writeand spliced into the test sources bywrite_inline_snapshots()at the end of the run.--bail[=N]stops the run after N failures. In the serial runner this is a direct process exit from the reporter callback or from the file loader, so artifacts that are normally written at the end of the run (JUnit, timings, now snapshots) are written on those paths explicitly.--parallelworkers do not get--bail; the coordinator stops dispatching files and waits for them.Earlier version of this PR (bail-only flush)
The first version only added the flush on the two bail exits. Self-review pointed out that the destructive part (
-utruncating on open) has the same effect on every other abnormal exit and that earlier files' snapshots depended on the lazy write, so the PR was reshaped to fix those insnapshot.rs/ per file, keeping the bail flush for the file that was running.