bun test: fix the reason reported when a snapshot matcher's expect() has no running test

[robobun]

Problem

• toMatchSnapshot() / toThrowErrorMatchingSnapshot() on an expect() whose test is no longer running report the wrong reason. The snapshot is correctly refused in every case below; only the message is wrong:
  • expect() created in a test (or hook) that has since finished, for example a test that timed out while awaiting and then calls captured.toMatchSnapshot() during the next test: Snapshot matchers are not supported in concurrent tests. Nothing is concurrent.
  • expect(x).toMatchSnapshot() inside a describe callback, or on an expect() created in a preload: the same concurrent-tests message.
  • expect() created in a test file that has already finished (the existing cross-file fixture in bun_test.test.ts): Snapshot matchers cannot be used outside of a test, printed under an expect(received).not.toMatchSnapshot() signature although .not was never used.
• Cause: Expect::get_snapshot_name (src/runtime/test_runner/expect.rs) mapped every failure to resolve the captured entry to SnapshotInConcurrentGroup, although RefDataValue::entry() also returns None for an entry the runner has advanced past and for a capture made outside the execution phase. The guards at the top of toMatchSnapshot.rs / toThrowErrorMatchingSnapshot.rs handled the finished-file form separately with their own message and passed not = true to get_signature.
• Noticed in a yarn-lock-migration run where a timed-out test.each case kept running into the next case. That run's late call created its expect() after the timeout, so it was attributed to the test running at that moment; that attribution is not changed by this PR (see Background). The cases above, where the expect() already exists when its test ends, are what it fixes.

Fix

• SnapshotContextError (expect.rs) classifies from the RefDataValue the expect() captured: a captured entry that no longer resolves means its test finished (TestNotActive, "not supported after the test has finished executing", a message that already existed but was only reachable once the file was torn down); a capture with no entry means a concurrent group (SnapshotInConcurrentGroup, unchanged); a capture from the collection or done phase means no test ("cannot be used outside of a test"). It also owns the three message strings.
• get_snapshot_name uses that classification both when the file is gone and when entry() fails; Expect::snapshot renders any of the three through it (replacing two inlined arms); the two matchers call the new Expect::check_snapshot_context, which does the same classification for the finished-file form and prints the matcher's own signature instead of the .not one.
• Correct because the captured state is the only thing that differs between the cases: get_current_state_data (bun_test.rs) records an entry exactly when one sequence is running, records none in a concurrent group, and records the scope or Done outside execution; and entry() only fails for a recorded entry once the runner has advanced past it (Execution::get_current_and_valid_execution_sequence). Behaviour is otherwise unchanged: nothing is written in any of these cases before or after, inline snapshots still ignore the error (they are keyed by source position), and the strong BunTest reference the old guards held during the matcher was not load-bearing (exit_file only runs from the test command between files).
• The classification stays in expect.rs rather than in RefDataValue::entry() because the reasons and messages are specific to the file snapshot matchers (entry()'s other caller, jest.rs, only needs the Option), and this keeps the overlap with the open PRs below to the one line they also touch.
• Tests, all in test/js/bun/test/snapshot-tests/bun-snapshots.test.ts unless noted; the ones marked * fail on the released bun with the old messages:
  • • toMatchSnapshot inside a describe callback: outside-of-a-test message
  • • expect() created in beforeAll, used by later tests: toMatchSnapshot (with and without hint) and toThrowErrorMatchingSnapshot report the finished-test message; toMatchInlineSnapshot on such an object still works; holds under -t and --randomize
  • two tests in a describe.concurrent group still get the concurrent message (unchanged behaviour, pinned)
  • • spawned file where test 1 times out and later calls toMatchSnapshot() on an expect() created before the timeout: stdout carries the finished-test message and the .snap contains only test 2's entry
  • • spawned two-file run where file 1 creates the expect()s and file 2 uses both matchers: both print their own signature and the finished-test message, and __snapshots__ contains only file 2's own entry
  • • test/js/bun/test/bun_test.test.ts "cross-file safety": assertion updated from the old message to the new signature and message
• Verified with bun bd test test/js/bun/test/snapshot-tests/bun-snapshots.test.ts test/js/bun/test/bun_test.test.ts (18 pass), the same two files under USE_SYSTEM_BUN=1 (6 fail, the ones marked above), and FORCE_COLOR=1 bun bd test test/js/bun/test/snapshot-tests/ test/js/bun/test/ci-restrictions.test.ts test/js/bun/test/expect.test.js test/js/bun/test/test-test.test.ts (534 pass; FORCE_COLOR is needed by the pre-existing error snapshots case, as on main).
• Open PRs that touch the same get_snapshot_name lines, all compatible with this one, so whichever lands later needs a one-line rebase: bun test: name snapshots taken in beforeEach/afterEach after the running test #38820 (names a hook's snapshot after its test; this PR only changes what is reported when there is no test), bun test: detect obsolete snapshots and fix -u "added" mislabel #34042 (moves the name building into a helper), and test: allow onTestFinished() in concurrent tests #29238 (adds an earlier concurrent-group check in front of this classification).

Background

• expect(value) records the runner's state at the moment it is called (RefDataValue): during collection the active describe scope; during execution the group and, when exactly one sequence is running, the entry (test or hook) that is running. Snapshot matchers later resolve that record against the runner's current state to get the test name.
• A concurrent group runs several sequences at once, so the runner cannot tell which one an expect() belongs to and records no entry; file snapshots are refused there (limitation documented in Rewrite test/describe, add test.concurrent #22534), inline snapshots work because they are keyed by file position.
• Each test file gets its own BunTest; an expect() holds a weak reference to it, which stops resolving once the runner has moved on to the next file. That is the finished-file form above.
• Because the record is taken when expect() is called, an expect() created after its test timed out is attributed to whatever test is running then. Jest (currentTestName) and Vitest (getCurrentTest() inside expect()) work the same way; attributing such calls to the timed-out test would need a per-test async context propagated through every await, which is a separate feature and not attempted here.

Earlier shape of this PR

The first revision only replaced the .ok_or(SnapshotInConcurrentGroup) in get_snapshot_name with a match on the captured RefDataValue and added a NoTest arm to Expect::snapshot. Review pointed out that the finished-file form was still reported as "outside of a test" by the matchers' own guards, and that the in-process test used an assertion-less it as setup for its siblings, which broke under -t and --randomize. The current revision moves the classification into one type used by all three sites and captures in beforeAll instead.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: cb4a71ea-a49c-4fb1-94a3-201b59cc93b5

📥 Commits

Reviewing files that changed from the base of the PR and between 2c2ef7c and e46b2c1.

📒 Files selected for processing (5)

• src/runtime/test_runner/expect.rs
• src/runtime/test_runner/expect/toMatchSnapshot.rs
• src/runtime/test_runner/expect/toThrowErrorMatchingSnapshot.rs
• test/js/bun/test/bun_test.test.ts
• test/js/bun/test/snapshot-tests/bun-snapshots.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: ready for review (current head e46b2c1).

Reproduced on the released build in three forms, all of which refuse the snapshot but report the wrong reason: an expect() created in a test that then finished (for example one that timed out while awaiting) and used during a later test, and toMatchSnapshot() called directly in a describe callback, both say Snapshot matchers are not supported in concurrent tests; an expect() created in a test file that has already finished says cannot be used outside of a test under an expect(received).not.toMatchSnapshot() signature. With this branch they report "after the test has finished executing", "cannot be used outside of a test", and "after the test has finished executing" under the matcher's real signature, respectively. The concurrent-group message is unchanged and nothing is written to any .snap file in these cases, before or after.

Scope note: an expect() that is itself created after its test timed out is attributed to whatever test is running at that moment. That is the call-time attribution model bun:test shares with Jest and Vitest and is not changed here (see Background in the description).

Verification: test/js/bun/test/snapshot-tests/bun-snapshots.test.ts (five new cases) and the updated cross-file assertion in test/js/bun/test/bun_test.test.ts fail without the src/ change and pass with it.

[claude]

LGTM — focused error-message reclassification with thorough test coverage.

What was reviewed:

• Verified the exhaustive match on RefDataValue covers all variants and that entry_data: None under Execution is set exactly when the runner cannot name a single running entry (bun_test.rs:702-716), matching the PR's claim.
• Confirmed the new NoTest arm in Expect::snapshot reuses the existing message string already used by the inline-snapshot paths (expect.rs:1067, 1127, 1225).
• Checked the tests: describe-body error is caught and asserted in a real it; the timeout subprocess test drains stdout/stderr/exited concurrently, sets CI: "false" so snapshot creation is not blocked, and asserts the .snap file contains only the second test's entry.

Extended reasoning...

Overview

This PR refines the error message reported when a snapshot matcher (toMatchSnapshot, toThrowErrorMatchingSnapshot) is called on an expect() whose captured execution entry can no longer be resolved. Previously, get_snapshot_name mapped every unresolvable case to SnapshotInConcurrentGroup, so a stale expect from a finished/timed-out test or a describe body reported the misleading "not supported in concurrent tests" message. The 13-line Rust change replaces a single .ok_or() with an exhaustive match over RefDataValue, routing Execution { entry_data: Some(_) } that fails validation to TestNotActive, keeping Execution { entry_data: None } as SnapshotInConcurrentGroup, and routing Start | Collection | Done to NoTest. A new NoTest arm is added to the error-formatting match in Expect::snapshot. The test file gains ~106 lines covering all three new classifications plus a regression assertion that the concurrent case is unchanged.

Security risks

None. This is a diagnostic-message change in the test runner; no untrusted input parsing, no file I/O changes, no allocation or lifetime changes.

Level of scrutiny

Low-to-medium. The Rust change is small, mechanical, and self-evidently correct once cross-referenced with RefDataValue's definition and BunTest::get_current_state_data (bun_test.rs:691-729), which I did — entry_data is only None when sequences.len() != 1 or the single sequence's active_entry is None, and a captured Some(_) that no longer resolves via entry() means the runner advanced past it. The match is exhaustive over the enum, so no fallthrough. Behavior is strictly a refinement: every path that previously errored still errors, only the reported reason changes; no snapshot is written where one wasn't before. The NoTest and TestNotActive message strings both already existed in the file.

Other factors

The tests are well-constructed per the repo's review guidelines: the subprocess test drains all pipes concurrently, uses tempDir/bunEnv/bunExe, sets CI: "false" to allow snapshot creation, asserts stdout/stderr before exitCode, and verifies the on-disk .snap file to prove the late matcher wrote nothing. The describe-body case correctly captures the error during collection and defers the assertion to an it. The concurrent-group test preserves the existing behavior as a guard against regression. The inline-snapshot test confirms the intended carve-out (inline matchers ignore this error) still holds. The PR description states the four new non-concurrent tests fail on main with the old message and pass with the fix, satisfying the fails-for-the-right-reason requirement.

[robobun]

No changes needed from the review above; it raised no findings. The diff is unchanged since 7a17cd1 and is waiting on CI.

[robobun]

^{Updated 8:05 PM PT - Aug 14th, 2026}

❌ @robobun, your commit 7a17cd1 has some failures in Build #97056 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38799

That installs a local version of the PR into your bun-38799 executable, so you can run:

bun-38799 --bun

[robobun]

^{Updated 10:08 PM PT - Aug 14th, 2026}

@robobun, your commit 16f7195 is building: #97382

[robobun]

Two commits since the review: 16f7195 is an empty retrigger because the only non-passing jobs in the first build were the two darwin 14 aarch64 - test-bun lanes, which expired without running; f2b8e1f shortens the one code comment in get_snapshot_name (see the thread on expect.rs). The fix itself is unchanged from 7a17cd1.

[claude]

LGTM — this only refines which error message get_snapshot_name returns for the three unresolvable-entry cases; the reject-the-snapshot behavior itself is unchanged.

What was reviewed:

• Match in get_snapshot_name is exhaustive over RefDataValue (bun_test.rs:1436); entry_data: None is only produced when sequences.len() != 1 or the sequence has no active entry (bun_test.rs:702-717), so mapping it to SnapshotInConcurrentGroup is still the right classification.
• The new NoTest arm in snapshot() reuses the existing "cannot be used outside of a test" wording already present at expect.rs:1066/1126/1224; inline-snapshot's add_count path already ignores NoTest/TestNotActive/SnapshotInConcurrentGroup (expect.rs:1071-1073), so inline matchers stay unaffected as claimed.
• Timeout subprocess test uses a two-way withResolvers handshake (no sleep), drains stdout/stderr/exited concurrently, and asserts the .snap file contains only the second test's entry.

Extended reasoning...

Overview

Two files: src/runtime/test_runner/expect.rs replaces a single .ok_or(SnapshotInConcurrentGroup)? with an exhaustive match over the captured RefDataValue, mapping Execution { entry_data: Some } → TestNotActive when entry() no longer resolves, Execution { entry_data: None } → SnapshotInConcurrentGroup (unchanged), and Start|Collection|Done → NoTest. A NoTest arm is added to the error-to-message match in snapshot(). The test file gains a new describe block covering describe-body, finished-test (both matcher variants + hint + inline-still-works), concurrent (regression), and a spawned timeout scenario asserting stdout, stderr, .snap contents, and exit code.

Security risks

None. This is error-message classification in the test runner; no untrusted input parsing, no I/O path changes, no memory or FFI surface touched.

Level of scrutiny

Low-to-medium. The Rust change is ~15 lines and purely widens an error classification without altering whether a snapshot is written. I verified against the source that the match covers all four RefDataValue variants, that entry_data: None is only produced for the concurrent/completed-sequence case in get_current_state_data, that entry() returning None on a captured Some entry does mean the runner advanced past it (via get_current_and_valid_execution_sequence), and that the inline-snapshot add_count path already swallows all three error variants so it is unaffected.

Other factors

The tests follow harness conventions (tempDir, bunEnv/bunExe, normalizeBunSnapshot, await using on the spawn, concurrent pipe drain, exit-code asserted last). The timeout fixture uses a deterministic promise handshake rather than a sleep, so it should not flake. The comment-cop bot's note about the multi-line code comment was addressed in f2b8e1f (now one line). The bug-hunting pass found nothing. The first CI build's only non-passing jobs were expired darwin lanes per the author's note; the retriggered build is in flight.

[robobun]

Nothing further to change from this pass either. The expect.rs thread is resolved, and on the current head (f2b8e1f) every CI job has passed except the two darwin 14 aarch64 - test-bun lanes, which are still waiting for an agent (the same lanes expired unrun on the first build).

[claude]

I reviewed this PR and didn't find any bugs. The classification looks right — entry_data: None at capture time maps to the concurrent case per get_current_state_data, and a captured entry that entry() no longer resolves is exactly the finished-test case — but since it reworks how get_snapshot_name and the two matcher entrypoints derive their error from RefDataValue internals, a human look from someone familiar with the test-runner phase model would still be worthwhile.

What was reviewed:

• SnapshotContextError::from_phase against get_current_state_data and RefDataValue::entry() — the three-way split matches how those populate entry_data.
• check_snapshot_context replacing the inline bun_test() guard in both matchers — the old guard passed not = true to get_signature, so the new false is a small correctness improvement, and the cross-file-safety assertion update follows from the finished-file case now reporting TestNotActive.
• The three unresolved comment-cop threads fired before e46b2c1, which trimmed the flagged doc comments to single lines; the current diff has no multi-line comments left.

Extended reasoning...

Overview

The PR fixes misleading error messages when a snapshot matcher runs on an expect() whose captured test context can no longer name a snapshot. It introduces a SnapshotContextError enum in src/runtime/test_runner/expect.rs that classifies the captured RefDataValue into three cases (test finished / concurrent group / no test), replaces the blanket SnapshotInConcurrentGroup in get_snapshot_name with that classification, adds a shared check_snapshot_context helper that both toMatchSnapshot and toThrowErrorMatchingSnapshot now call in place of their inline bun_test().is_none() guard, and routes the three error variants through a single message() in Expect::snapshot. The bun_test.test.ts cross-file-safety assertion is updated to the new (more specific) message, and ~170 lines of new coverage in bun-snapshots.test.ts exercise describe-body, finished-hook, concurrent-group, timed-out-test, and finished-file cases.

Security risks

None. The change only affects which pre-existing error string is thrown on paths that were already errors; no new inputs are parsed and no snapshot is written where one wasn't before.

Level of scrutiny

Moderate. The functional surface is narrow — only user-visible error text changes, and the tests confirm the concurrent-group message is unchanged and no .snap entries are written for the rejected cases — but the classification hinges on invariants of RefDataValue capture (entry_data is None only for concurrent groups or a completed sequence; a captured entry that entry() fails to resolve means the runner advanced past it). I traced these against get_current_state_data (bun_test.rs:691) and RefDataValue::entry() (bun_test.rs:1465) and they hold, but this is the kind of subsystem-internal reasoning a maintainer of the test runner is best placed to confirm at a glance.

Other factors

Test coverage is thorough: each classification variant, both matchers, the with-hint form, the inline-snapshot exemption, and two spawned end-to-end scenarios (timeout and cross-file) with exact .snap file contents asserted. The get_signature third argument flipped from true to false in the extracted helper — this is correct (the .not branch already returned earlier) and the updated bun_test.test.ts assertion reflects the resulting signature. Three comment-cop bot threads from 08:00 UTC are unresolved in the timeline, but commit e46b2c1 ("trim SnapshotContextError doc comments") was pushed afterward and the current diff contains only single-line doc comments, so those appear addressed. Given the moderate refactor depth in a core file rather than a mechanical change, deferring to a human reviewer.

[robobun]

Reworked in 90ca6a2 and e46b2c1 after review: the classification now lives in one type (SnapshotContextError) used by get_snapshot_name, Expect::snapshot and a shared pre-check in both file snapshot matchers, so the finished-file form (the existing cross-file fixture) reports the same reason as the same-file form and prints the matcher's real signature; the in-process test captures in beforeAll so it holds under -t and --randomize, and a two-file fixture covers the finished-file form. Title and description are updated to describe the current diff, and the three comment threads are resolved. Nothing further to change from the latest review pass.