Repository navigation
test(js): drive the remaining fake-timer tests through Manual - #7076
Conversation
Quest-Claim: 71214b016ea054dd
Bun's fake timers cannot see a host-task yield, so these tests passed only while nothing yielded at the wrong moment. They now install a Manual clock and restore the ambient one when they finish. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 seconds. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 5b4abe3
No actionable bugs found. Reusing Manual.advance instead of fixed drain loops is a simpler, more deterministic direction; the explicit close hook meaningfully exercises the linger-expiry reentrancy window. Clock restoration and the converted deadline/backoff assertions look consistent with the surrounding implementations. Host-yield accounting still depends on #6988 as documented, rather than being implemented here.
Verification: static inspection of the changed tests and relevant clock/forwarding/deadline helpers, including an independent lifecycle pass. I did not run tests, mutation checks, or the claimed forced-yield integration checks.
|
Merged
Stress run of (Written by Claude Opus 5.5) |
Finishes quest/m0/test-flakes-2/fake-timers.md.
Problem
The last
@moq/netand@moq/hangtests on bun'sjest.useFakeTimers()drained work with hand-rolled loops (64 microtask turns, 20setImmediateturns, or one MessageChannel task). Neither fake timers nor those loops can see a host-task yield, so the tests passed only because nothing yielded at the wrong moment.Changes
Each test installs a
Manualclock and closes it when it finishes (inafterEach,finally,onTestFinished, orusing), so no clock leaks into the next test.clock.advance(...)replacesjest.advanceTimersByTimeplus its drain loop, andclock.advance(Nano.zero)replaces the drain on its own.connection/forward.test.ts: the whole file.ietf/publisher.test.ts: the clustered tests (clustered()now owns the clock), plus two epoch tests that shared theirflush/takehelpers.ietf/subscriber.test.ts: "returning demand preserves pending setup".lite/subscriber.test.ts: the three setup-deadline tests.lite/publisher.test.ts: the PROBEReporttest, plus the malformed SUBSCRIBE_UPDATE test that shared itssettleControlsdrain.integration.test.ts: "a request outlives the listing it resolved through" runs onManualfrom the start. It no longer switches fake timers on and off mid-test, andexpect(ended).toBe(1)replaces a wall-clock poll.connection/connect.test.ts: "SETUP preserves a delayed session close code".hang/src/container/consumer.startup.test.ts:setup()owns the clock and returnsflush.test/audio-quality/clients/js/src/probe.test.ts: already usedManual. Drops its redundantjest.useFakeTimers()and its private import ofjs/clock/src/host.ts'sdrain.Together with moq-dev/moq#6988, which converts
pool.test.tsandstats.test.ts, nouseFakeTimersoradvanceTimersByTimeis left in the repo.forward.test.ts's "a watch arriving as the linger expires opens a fresh request" was vacuous. With fake timers it still passed after removing the linger callback's eagerinterests.delete(prefix), which is the behavior it names. A plainManualport can't place the watch at that moment either:jumplands it before the callback, andadvancelands it after the stream wound down.FakeSessionnow has anonClosehook, so the test opens the watch exactly as the forwarder closes the old request. The test fails without thatdelete.Verification
just checkpasses.forward.ts:LINGER±1ms, and the eagerinterests.delete.ietf/publisher.ts: the advertise timeout, the refusal's retry interval, and the backoff doubling and wait.util/deadline.ts:REQUEST_TIMEOUT_MSand the deadline's stream aborts.lite/publisher.ts:PROBE_INTERVAL.error.ts:sessionCausereporting the session's close.forward.tsagain for the integration test:LINGER, and the hold-beneath check.hang/consumer.ts: the hole-elapsed and skip-hole checks.SAMPLE_INTERVAL_MS.Budget.pollforced to yield on every call, every converted file passes. They also pass with that PR'sManualyield accounting disabled, so none of them races a yield today. This change is preventive, as the quest expected.Impact
Public API: none. Wire: none. Tests only.
Choices made without asking
ManualAPIs already on main, and the two branches merge cleanly. Stacking would have put fix(js): yield host tasks through the injected clock #6988's unreviewed diff in this PR. The converted tests start seeing yields once fix(js): yield host tasks through the injected clock #6988 lands.first.close()is nowawait clock.advance(...), which drains, instead of a synchronousjump. The test and its timeout mutation still behave the same.flush/advancehelpers intoclock.advance(...), which the rest of that file already uses.Follow-ups
broadcast.test.ts"a stalled consumer does not pin evicted groups" has an inert time step. It moves wall time withsetSystemTime, but the cache ages groups on the monotonic clock (Timestamp.now()readsnow(), andsetSystemTimedoes not moveperformance.now()). The test passes with thesetSystemTime(new Date(12_000))line removed. It should drive aManualclock pastmaxAge, and be shown to fail when eviction breaks.track.test.ts'ssetSystemTimeuse is intentional: it proves wall-clock changes don't affect the budget.just checkon main: "a server refuses a client offering only IETF versions in the moql SETUP" failed. fix(js): yield host tasks through the injected clock #6988 already fixes it.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)