Conversation
…ct or property matchers argument Bun__deepMatch only looked for asymmetric matchers while walking the properties of the expected object. When the expected value itself was a matcher (toMatchObject(expect.objectContaining(...)), or a matcher passed as the property matchers of toMatchSnapshot / toMatchInlineSnapshot) it was walked like a plain object. Matcher objects have no enumerable properties, so the walk was empty and the assertion always passed. The same applied to a matcher on the received side, which always failed. Check both values for a matcher before the property walk, the way Bun__deepEquals already does. expect.objectContaining() keeps walking its sample as a plain object, as in Jest, and Bun.deepMatch is unaffected because it does not enable asymmetric matchers.
|
Updated 3:06 PM PT - Aug 12th, 2026
❌ @robobun, your commit fb3cb9e has 3 failures in
🧪 To try this PR locally: bunx bun-pr 37912That installs a local version of the PR into your bun-37912 --bun |
|
Warning Review limit reached
Next review available in: 1 minute 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 |
|
Reproduced on the released binary ( Fix is in this PR (#37912); independent of #37904, which touches the lines just below this check. |
There was a problem hiding this comment.
LGTM — mirrors the existing top-level asymmetric-matcher check from Bun__deepEquals into Bun__deepMatch.
What was reviewed:
- New block at bindings.cpp:1978 matches the pattern at :748 and the per-property check at :2046;
.asCell()is guarded by theisCell()asserts above, and noRETURN_IF_EXCEPTIONaftermatchAsymmetricMatcheris consistent with both existing call sites. isMatchingObjectContainingguard keeps theexpect.objectContainingsample walk (bindings.cpp:539) unchanged;Bun.deepMatchis unaffected via theenableAsymmetricMatcherstemplate guard.- Tests cover built-in and
expect.extendmatchers on both sides,.not.*inversion, nesting, and both snapshot matcher variants including the written.snapfile.
Extended reasoning...
Overview
The PR adds a top-level asymmetric-matcher check to Bun__deepMatch in src/jsc/bindings/bindings.cpp, so toMatchObject(expect.objectContaining(...)) and the snapshot property-matchers argument are actually applied instead of being walked as an empty object. Three test files gain coverage: expect.test.js (built-in matchers on both sides), expect-extend.test.js (custom matchers), and snapshot.test.ts (spawned subprocess exercising toMatchSnapshot/toMatchInlineSnapshot).
Security risks
None. This is test-runner matcher logic; the only inputs are values already reachable from user test code, and matchAsymmetricMatcher was already invoked on the same values one frame deeper.
Level of scrutiny
Moderate. The C++ change is ~30 lines in a hot equality path shared by toMatchObject, objectContaining, and snapshot property matchers, so I checked every call site: the objectContaining sample path (bindings.cpp:539) passes isMatchingObjectContaining=true and is skipped by the new guard; the top-level entry (bindings.cpp:3154) passes false and hits it; Bun__deepMatch<false> (Bun.deepMatch) compiles the block out via if constexpr. The new code is byte-for-byte the same shape as Bun__deepEquals at :748 and the existing per-property check at :2046, including its exception-handling stance (throwScope is threaded through and checked by the caller / next RETURN_IF_EXCEPTION at :2013).
Other factors
Test coverage is thorough — pass/fail for each matcher family, .not.* inversion, received-side matchers, nested matchers inside a top-level objectContaining, and a subprocess snapshot test that asserts both stdout markers and the exact .snap contents. The PR description states verification against Jest/Vitest and BUN_JSC_validateExceptionChecks=1. No prior human review comments to address.
Problem
toMatchObjectignores an asymmetric matcher passed as the whole expected value, so these assertions pass in Bun 1.4.0 and on main (Jest and Vitest 4.1.9 fail both):toMatchSnapshot/toMatchInlineSnapshotgoes through the same code, sotoMatchInlineSnapshot(expect.any(Array), ...)on an object also skips straight to the snapshot comparison, andtoMatchSnapshot(expect.objectContaining({ a: 2 }))records a snapshot instead of failing.expect(expect.any(Object)).toMatchObject({ a: 1 })always fails, while Jest applies the received-side matcher.Bun__deepMatch(src/jsc/bindings/bindings.cpp:1960) only checks for matchers inside its per-property loop. When the matcher is thesubsetargument itself it is walked like a plain object; theExpect*matcher classes keep their state in internal fields and have no enumerable properties, so the walk is empty and the match is vacuously true.Bun__deepEquals(toEqual) already checks both values for a matcher before comparing structurally (bindings.cpp:748), which is whytoEqual(expect.objectContaining(...))works.Fix
matchAsymmetricMatcheron the expected value and, failing that, on the received value, and return its PASS/FAIL verdict;NOT_MATCHERfalls through to the existing walk. This is the same checkBun__deepEqualsdoes at its top and the per-property loop does for each property, so nesting behaves as before.toMatchObjectand the snapshot property matchers both callequals(received, expected, [iterableEquality, subsetEquality]), andequalsapplies an asymmetric matcher on either side before anything else, at the top level included.expect.objectContaining()(isMatchingObjectContaining). Jest'sObjectContainingalways walks its sample's keys, soobjectContainingbehavior is unchanged fortoEqual,toHaveBeenCalledWith, etc.Bun.deepMatchis unaffected because it instantiates the template with matchers disabled.replacePropsWithAsymmetricMatchershas nothing to do at the top level (there is no parent property to rewrite), so a passing top-level matcher in a snapshot assertion stores the received value as-is. Jest'sdeepMergecrashes inside pretty-format in that situation, so there is no behavior to mirror there; the failing case fails like Jest.test/js/bun/test/expect.test.js(toMatchObjectdescribe): two new tests, expected-side and received-side matchers, includingexpect.not.*inversion and nested matchers inside a top-levelobjectContaining. Both fail on the released binary and pass with this change; the same assertions pass under Vitest 4.1.9.test/js/bun/test/expect-extend.test.js: a custom (expect.extend) asymmetric matcher used as the wholetoMatchObjectargument, also verified under Vitest. Fails on the released binary.test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts: spawns a test file using a top-level matcher as the property matchers oftoMatchSnapshotandtoMatchInlineSnapshot, passing and failing each, and checks the resulting.snapfile. Fails on the released binary (all four cases pass there and the failingtoMatchSnapshotwrites a snapshot).expect.test.js(417 pass),expect-extend.test.js,spyMatchers.test.ts,jest-extended.test.js, the snapshot test directory and theobjectContainingregression tests are unchanged, also withBUN_JSC_validateExceptionChecks=1.toMatchObject(expect.any(Headers))on aHeadersapplies the matcher.Background
expect.any(),expect.objectContaining(), matchers created byexpect.extend) are instances of theExpect*classes defined insrc/runtime/test_runner/jest.classes.ts. They all carry theJSDOMWrapperTypeJSType, which is howBun__deepEquals/Bun__deepMatchcheaply decide whether to callmatchAsymmetricMatcher; that function returnsNOT_MATCHERfor any other wrapper object (Headers, Blob, ...) and handlesexpect.not.*inversion itself.Bun__deepMatch<enableAsymmetricMatchers>is the subset comparison behindtoMatchObject, the property matchers argument of the snapshot matchers,expect.objectContaining()(withisMatchingObjectContaining, which switches nested objects to exactdeepEquals) andBun.deepMatch(matchers disabled). It walks the enumerable properties of the expected value and compares each against the received value.replacePropsWithAsymmetricMatchersis set bytoMatchObjectand the snapshot matchers: when a property-level matcher passes, the received property is overwritten with the matcher so that the failure diff and the stored snapshot printAny<Date>instead of the volatile value.