Conversation
|
Warning Review limit reached
Next review available in: 26 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 (2)
Comment |
|
Updated 12:05 AM PT - Jul 18th, 2026
❌ @robobun, your commit 49dc177 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34320That installs a local version of the PR into your bun-34320 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward fix baking the label literals into format_args! at four call sites, matching the pattern already used by toContain.rs and the other matchers. The inline nit about comment length is non-blocking.
Extended reasoning...
Overview
Restores the "Expected/Received" label text in toContainEqual and toBeArrayOfSize failure messages. Four call sites were passing the label literal to throw_fmt's ignored _fmt parameter instead of baking it into format_args!; they now call throw() directly with the full format string, matching toContain.rs. The throw_fmt debug assertion is tightened to probe args for a leading '\n' so this class of bug trips in debug builds. Tests added to the existing expect.test.js and jest-extended.test.js files.
Security risks
None. This only affects the text of test-runner assertion failure messages.
Level of scrutiny
Low. Mechanical fix following an established pattern; the debug-assertion change is #[cfg(debug_assertions)]-gated and compiles out of release builds. I audited every remaining throw_fmt caller (.pass(), .fail(), toBeCloseTo, snapshot matchers) — all either pass _fmt = "" (skipped by the guard) or already lead args with "\n\n", so the tightened assert won't false-positive on any of them. fmt::Arguments is Copy, so passing it to the probe and then to self.throw is fine.
Other factors
Tests are placed in the correct existing files per repo convention, cover both the .not and positive variants, and assert on message content (labels + values) rather than exact formatting. The one finding is a CLAUDE.md comment-length nit on the debug-assert block — purely stylistic and non-blocking.
|
Opened #34328 for the same bug before the dedupe bot flagged this one; closing mine in favor of this. One difference: this PR keeps |
There was a problem hiding this comment.
LGTM — the comment-length nit is addressed in 493a0e1 and no new issues found.
Checked: the four call-site fixes match the toContain.rs pattern exactly (throw() + format_args!(concat!(...))). Grepped all remaining throw_fmt callers (toBeCloseTo, snapshot matchers, .pass/.fail, inline-snapshot paths in expect.rs) — every one passes format_args! beginning with "\\n\\n", so the tightened debug probe won't false-positive. The probe is #[cfg(debug_assertions)]-gated so release builds are unaffected.
The alternative in #34328 (delete throw_fmt entirely) is a reasonable follow-up but doesn't block this fix.
Extended reasoning...
Overview
Restores the Expected:/Received: label text in toContainEqual and toBeArrayOfSize failure messages, which regressed when these matchers were ported to the throw_fmt calling convention. Four call sites in toContainEqual.rs and toBeArrayOfSize.rs are rewritten to bake the literal label text into format_args! and call throw() directly — the exact pattern already used by toContain.rs:117-140. The throw_fmt debug assertion in expect.rs is tightened from args.as_str() != Some("") (which format_args!("{}", v) trivially satisfies since it returns None) to a short-circuiting fmt::Write probe that checks the first non-empty write starts with '\n'. New message-content tests in expect.test.js and jest-extended.test.js cover both the not and non-not variants of each matcher.
Security risks
None. This touches only test-runner assertion-failure message formatting; no user input parsing, no I/O, no auth/crypto surface.
Level of scrutiny
Low-to-moderate. The call-site changes are mechanical pattern alignment with an existing sibling (toContain.rs). The debug-assert probe is slightly novel but is #[cfg(debug_assertions)]-gated, so it cannot affect release builds; I verified via grep that every remaining throw_fmt caller (in toBeCloseTo.rs, toMatchSnapshot.rs, toThrowErrorMatchingSnapshot.rs, and expect.rs itself for .pass/.fail/inline-snapshot) passes a format_args! literal beginning with \"\\n\\n\", so the new assert holds for all of them. The probe returning Err(fmt::Error) after the first write correctly short-circuits so dynamic args aren't rendered twice.
Other factors
- My prior nit (6-line comment vs. CLAUDE.md rule 13) was addressed in 493a0e1; the diff now shows the 3-line version and the thread is resolved.
- robobun noted an alternative approach in #34328 that deletes
throw_fmtentirely and migrates the remaining ~19 callers tothrow(). That's a valid but larger refactor; this PR's more conservative fix-the-callers-plus-tighten-the-guard approach is correct on its own and doesn't preclude the follow-up. Not a blocker. - Tests were added to the existing test files (
expect.test.js,jest-extended.test.js) rather than new files, per repo convention, and the PR description confirms they fail onmainand pass with the fix. - Bug hunter found no issues this run.
There was a problem hiding this comment.
LGTM — the comment-length nit from the previous pass is addressed in 493a0e1.
Checked: the four call-site rewrites match the throw() pattern already used by toContain and neighbors; the tightened throw_fmt debug-assert is #[cfg(debug_assertions)]-only and I verified every remaining throw_fmt caller (toBeCloseTo, pass/fail, snapshot matchers) either passes _fmt="" or leads args with "\n\n", so no new false positives. New tests cover all four fixed paths (both .not variants).
Extended reasoning...
Overview
Restores the "Expected to contain:" / "Received:" label text in toContainEqual and toBeArrayOfSize failure messages. Four call sites in toContainEqual.rs / toBeArrayOfSize.rs were passing label literals to throw_fmt's ignored _fmt param instead of baking them into format_args!; they now call throw() directly with the literal inlined, matching the pattern every other matcher uses. Also tightens the debug-only guard in Expect::throw_fmt to probe the first non-empty write of args for a leading '\n', and adds message-content tests in expect.test.js and jest-extended.test.js.
Security risks
None. This is pure test-runner error-message formatting on the failure path; no untrusted input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. The four call-site changes are mechanical throw_fmt(g, sig, LIT, format_args!("{}", v)) → throw(g, sig, format_args!(LIT, v)) rewrites. The only non-trivial piece is the new Probe writer in throw_fmt, which is gated behind #[cfg(debug_assertions)] so it has zero release-build impact. I traced its behavior: empty writes are skipped, the first non-empty write sets the flag and returns Err(fmt::Error) to short-circuit, and if args is entirely empty the flag stays false and the assert fires (correct — that's the original bug shape). I grepped every remaining throw_fmt caller and confirmed each either passes _fmt="" (check skipped) or leads format_args! with a "\n\n..." literal, so the tightened assert introduces no false positives.
Other factors
- My prior review's only finding (6-line comment vs. CLAUDE.md rule 13) was addressed in 493a0e1; the comment is now 3 lines and the thread is resolved.
- New tests use the same try/catch +
expect.unreachable()pattern as the existingtoBeOneOftest in the same file; if the matcher failed to throw, the positivetoContain("\n\nExpected to contain: ")assertion would fail on the unreachable-error message, so the tests are sound. - No CODEOWNERS match these paths (
.github/CODEOWNERSonly covers/test/expectations.txtfor anything expect-related). - The thread notes an alternative (#34328) that deletes
throw_fmtentirely. That's a valid follow-up but orthogonal — this PR's more conservative keep-and-tighten approach is correct on its own and has the smaller blast radius.
|
Rebased onto main. The src/ fix is no longer needed here: #34343 migrated every matcher to the Note: with no src/ diff the fail-before gate will see both runs pass identically; a maintainer merge is needed. |
…e labels The "Expected to contain:" / "Received:" labels were being dropped from these matchers' failure messages (the format literal was passed to a discarded parameter). #34343 fixed the underlying bug as part of migrating all matchers to the throw! macro; these tests lock the message shape so it can't regress again.
fb6d4bd to
49dc177
Compare
robobun
changed the title
There was a problem hiding this comment.
LGTM — test-only regression coverage for matcher failure-message labels.
What was reviewed:
- Verified the asserted label strings (
\n\nExpected to contain:,\n\nExpected to not contain:,\n\nReceived:) against the currentthrow!format literals intoContainEqual.rsandtoBeArrayOfSize.rs. - Checked the try/
expect.unreachable()/catch pattern is non-vacuous (matches the existingtoBeOneOfmessage test in the same file) and that the catch-block assertions would fail on the pre-#34343 output. - Confirmed tests are added alongside existing matcher tests in the correct files, no new files created.
Extended reasoning...
Overview
This PR adds two small regression tests (~36 lines total) to test/js/bun/test/expect.test.js and test/js/bun/test/jest-extended.test.js, asserting that toContainEqual / not.toContainEqual and toBeArrayOfSize / not.toBeArrayOfSize failure messages include their "Expected to contain:" / "Received:" label text. The underlying bug (labels dropped because a format literal went to a discarded _fmt parameter) was already fixed by #34343; this PR only adds coverage so it can't silently regress.
The PR originally also touched src/runtime/test_runner/expect.rs (my earlier inline nit about comment length applied there, and was addressed), but after rebasing onto #34343 the src/ change was dropped and the diff is now test-only.
Security risks
None. Pure test additions to the test-runner suite; no runtime, network, filesystem, or auth code touched.
Level of scrutiny
Low. Test-only, ~36 lines, follows the exact try/expect.unreachable()/catch pattern already used a few lines away for toBeOneOf in jest-extended.test.js. I cross-checked each asserted substring against the current concat! format literals in toContainEqual.rs:120,132-133 and toBeArrayOfSize.rs:56,66 — they match. The catch-block assertions are non-vacuous: if the matcher failed to throw, expect.unreachable() would throw an error whose message does not contain the asserted label, so the .toContain checks would fail.
Other factors
- Tests are placed in the existing files for these matchers (per REVIEW.md "add your test to the existing test file"), not new files.
- Both positive and
.notvariants are covered. - The PR description states both tests fail on the pre-#34343 1.4 canary and pass on current main; the author noted the fail-before CI gate can't distinguish because there's no src/ diff, which is expected for a test-only follow-up to an already-merged fix.
- No outstanding reviewer comments; my earlier nit was resolved and is now moot since the file it targeted is no longer in the diff.
This branch has not been deployed
1 participant
The "Expected to contain:" / "Received:" labels were being dropped from these matchers' failure messages (the format literal went to a discarded
_fmtparameter instead of being baked intoformat_args!):The underlying bug was fixed by #34343 as part of migrating every matcher to the
throw!macro (which takes the format literal as a first-class argument). This PR adds message-content assertions for all four affected paths (toContainEqual,not.toContainEqual,toBeArrayOfSize,not.toBeArrayOfSize) so the label text can't silently drift again.Both tests fail on the 1.4 canary that predates #34343 and pass on current
main.[stamp-90s] gate passed · iteration 2 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file