Event: implement timeStamp instead of always returning 0#34894
Open
robobun wants to merge 3 commits into
Open
Claude / Claude Code Review
completed
Jul 21, 2026 in 19m 55s
Code review found 2 potential issues
Found 4 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/js/web/web-globals.test.js:148-153 |
timeStamp and performance.now() are on different time bases — exact >= bounds can flake |
| 🟡 Nit | test/js/web/web-globals.test.js:145 |
Test asserts stderr is exactly empty — flaky under ASAN/debug |
Annotations
Check warning on line 153 in test/js/web/web-globals.test.js
claude / Claude Code Review
timeStamp and performance.now() are on different time bases — exact >= bounds can flake
`event.timeStamp` and `performance.now()` are on subtly different time bases: `GlobalObject::performance()` samples `Bun__readOriginTimer` first and `MonotonicTime::now()` second, so `m_timeOrigin` overshoots the true origin by δ ≥ 0 and every `timeStamp` reads δ ms lower than `performance.now()` would at the same instant. The zero-tolerance `toBeGreaterThanOrEqual(before)` / `first >= after` assertions here therefore rely on JS overhead exceeding δ — normally trivial, but a preemption between t
Check warning on line 145 in test/js/web/web-globals.test.js
claude / Claude Code Review
Test asserts stderr is exactly empty — flaky under ASAN/debug
`expect(stderr).toBe("")` will flake on ASAN/debug CI lanes, which emit benign warnings on stderr — REVIEW.md's subprocess-test rule explicitly says to assert a combined `{ stdout, stderr, exitCode }` object instead. It also runs before `JSON.parse(stdout)`, so a spurious stderr failure hides the actual output; the `navigator.%s is a getter-only accessor` test just below already shows the preferred pattern (`stderr: expect.any(String)` inside a combined `toEqual`).
Loading