bun:test: give each VM its own fake clock - #38740
Open
dylan-conway wants to merge 10 commits into
Open
Claude / Claude Code Review
completed
Aug 15, 2026 in 36m 41s
Code review found 2 potential issues
Found 2 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/bun_core/util.rs:5148-5150 |
wall_ms() can now return Some(NaN); NaN was the old 'unset' sentinel |
| 🟡 Nit | src/runtime/test_runner/timers/FakeTimers.rs:140-145 |
Mid-fire setInterval survives the useFakeTimers() reinstall that drops everything else |
Annotations
Check warning on line 5150 in src/bun_core/util.rs
claude / Claude Code Review
wall_ms() can now return Some(NaN); NaN was the old 'unset' sentinel
The old `AtomicU64` encoding used NaN as the "not mocked" sentinel, so `set_wall_ms(NaN)` left `wall_ms()` returning `None`; the new `Cell<Option<f64>>` stores `Some(NaN)`, so `useFakeTimers({now: NaN})` / `{now: new Date('invalid')}` now makes `milli_timestamp_allow_mocked_time()` return NaN (feeding `Bun.cron`) while `JSMock__setOverridenDateNow(NaN)` still means "no override" — the two sides now disagree on whether the wall clock is mocked. Consider either short-circuiting NaN → `None` in `se
Check warning on line 145 in src/runtime/test_runner/timers/FakeTimers.rs
claude / Claude Code Review
Mid-fire setInterval survives the useFakeTimers() reinstall that drops everything else
nit: a `setInterval` that is *currently firing* when its own callback calls `useFakeTimers()` survives this drop — `clear()` only walks the heap, and the interval was already popped by `execute_until`, so `run_impl` still sees `state == FIRED` after the callback and reschedules it into the new heap at the pre-callback `time_before_call`. The reinstall-inside-callback test uses `setTimeout` so it doesn't hit this. Very much an edge case, but the doc comment slightly overstates what is dropped.
Loading