fix 14250 - #14256
Merged
Merged
fix 14250#14256
Conversation
Collaborator
robobun
added a commit
that referenced
this pull request
Jun 18, 2026
The stress test allocates hundreds of thousands of Set/Map iterators and runs far past the default 5s per-test timeout under a debug + ASAN build (~170s on slower hardware), so it timed out on every ASAN run of this file and effectively provided no coverage there. Give it an explicit timeout so it runs to completion. The workload (and thus the regression coverage from #14256) is unchanged; release builds finish in a couple of seconds.
robobun
added a commit
that referenced
this pull request
Jun 18, 2026
…test Put the deepEquals undefined-key regression tests back in expect.test.js (the module test file for toEqual / expect matchers) instead of a separate file. To keep that file within the default per-test timeout under a debug + ASAN build, shrink the pre-existing deepEquals Set/Map stress test's element count (150 -> 25) and iteration counts (2000/1000 -> 500/250) rather than raising its timeout. The test still allocates thousands of Set/Map iterators, which is what it exercises (see #14256); on a debug + ASAN build it now runs in ~2s instead of timing out.
This was referenced Jun 28, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jun 28, 2026
### Problem
`expect().toContain()` compares array and iterable elements with
SameValue (`Object.is`), but Jest uses `Array.prototype.indexOf`, which
is strict equality. The two only differ on `NaN` and signed zero, and
both directions diverge today:
```js
test("a", () => expect([-0]).toContain(0)); // bun: fail, jest 30.4.1: pass
test("b", () => expect([NaN]).toContain(NaN)); // bun: pass, jest 30.4.1: fail
```
```
error: expect(received).toContain(expected)
Expected to contain: 0
Received: [ -0 ]
```
The first direction is the painful one: any numeric result that happens
to be `-0` breaks an otherwise valid `toContain(0)` only under `bun
test`. Jest's own failure output names the comparison it uses
(`expect(received).toContain(expected) // indexOf`), and the docs say
`toContain` "uses ===, a strict equality check".
### Fix
`toContain.rs` now uses `JSC::JSValue::strictEqual` instead of
`sameValue` on both the array-like and iterable paths. `toBe`, which
Jest documents as `Object.is`, is unchanged, and `toContainEqual` still
does deep equality.
The existing `JSC__JSValue__isStrictEqual` binding is surfaced as
`JSValue::is_strict_equal`, and the napi-local shim that duplicated it
is removed. The new helper deliberately has no encoded-bits fast path:
two NaNs share an encoding but are not strictly equal.
### Tests
Added `-0` / `NaN` variants to the existing `toContain` and
`not.toContain` `test.each` lists in `test/js/bun/test/expect.test.js`,
covering both the array-like path (`Array`, `Float64Array`) and the
iterable path (`Set`, generator). All 7 fail on current bun and match
Jest 30.4.1 on Node 26.
That file also contains a pre-existing `deepEquals Set/Map stress test`
that takes about 4 minutes under a debug+ASAN build, far past the
default 5s per-test timeout, so `bun bd test
test/js/bun/test/expect.test.js` was already red on main. It is the GC
stress regression test for #14256 and its iteration count is
load-bearing, so it now carries an explicit timeout rather than a
smaller workload. CI's ASAN lane already gives it a 270s budget through
the runner's 3x multiplier, so only plain `bun bd test` was affected.
`test/napi/napi.test.ts -t napi_strict_equals` still matches Node after
the shim removal.
robobun
added a commit
that referenced
this pull request
Jun 28, 2026
The expect.test.js suite could not go green on a debug+ASAN build because the deepEquals Set/Map stress test took over four minutes and timed out at 5000ms (1s on a release build). Its cost is quadratic in the element count: every distinct array key misses the Set/Map identity lookup and takes the JSSetIterator/JSMapIterator linear fallback, and 150 elements across 3000 iterations is ~45M nested deepEquals calls. It is loose mode over Sets and untouched by the Proxy change; the timeout is pre-existing. Shrink the element count (the quadratic term) from 150 to 20 and the iterations from 2000/1000 to 500/250, which still constructs the fallback iterators thousands of times and runs in under 2 seconds on a debug+ASAN build. To confirm nothing is lost, the exact structure-confusion bug from the fix this test was added for (#14256: passing the Set's own Structure to JSSetIterator::create) was re-injected and the test passes both before and after this change, so the shrink does not reduce what it catches. The test also had zero assertions; every deepEquals result is now asserted, so a wrong answer on the Set/Map fallback path fails it too.
robobun
added a commit
that referenced
this pull request
Jun 28, 2026
Jest accepts any object with a callable .then for .resolves/.rejects, and for .rejects it also accepts a function returning a promise. Bun only accepted real Promise instances, so awaiting a query builder or ORM result with expect(...).resolves failed with "Expected promise". In process_promise, wrap a thenable in a promise via the spec resolve algorithm so wait_for_promise can await it, and for .rejects call the received function first. The error message for .rejects now mentions functions, matching Jest. Also shrink the deepEquals Set/Map stress test so this file's other tests stay within the default per-test timeout on a debug + ASAN build; the workload it exercises (thousands of Set/Map iterator allocations, #14256) is unchanged. Fixes #18857
robobun
added a commit
that referenced
this pull request
Aug 1, 2026
The expect.test.js suite could not go green on a debug+ASAN build because the deepEquals Set/Map stress test took over four minutes and timed out at 5000ms (1s on a release build). Its cost is quadratic in the element count: every distinct array key misses the Set/Map identity lookup and takes the JSSetIterator/JSMapIterator linear fallback, and 150 elements across 3000 iterations is ~45M nested deepEquals calls. It is loose mode over Sets and untouched by the Proxy change; the timeout is pre-existing. Shrink the element count (the quadratic term) from 150 to 20 and the iterations from 2000/1000 to 500/250, which still constructs the fallback iterators thousands of times and runs in under 2 seconds on a debug+ASAN build. To confirm nothing is lost, the exact structure-confusion bug from the fix this test was added for (#14256: passing the Set's own Structure to JSSetIterator::create) was re-injected and the test passes both before and after this change, so the shrink does not reduce what it catches. The test also had zero assertions; every deepEquals result is now asserted, so a wrong answer on the Set/Map fallback path fails it too.
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
3 participants
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.
What does this PR do?
fixes #14250
How did you verify your code works?
added a stress test for
MapandSetwithdeepEquals