test(js): stop asserting exact timer tick counts in flaky JS timer tests#4767
Merged
Conversation
`Timer.Elapsed.Subscribe works` and `Timer with AutoReset = true works` both started a 50ms timer, slept 125ms, and asserted the handler had run exactly twice. How many ticks land inside a fixed sleep is a wall-clock race: the second tick is due at 100ms, leaving 25ms of slack, and on a loaded CI runner — where timer resolution is coarse — it slips past the window and only one arrives. `Timer.Elapsed.Subscribe works` failed this way on the windows-latest JS leg. Assert the semantics the tests are named for instead: the handler fires repeatedly, and stops firing once the timer is stopped or the subscription is disposed. The counter is read after Stop/Dispose so a tick cannot land between the read and the unsubscribe. This is the same tolerance the neighbouring `Assigning an event to a variable works` already uses (200ms sleep, at least two ticks), which has been stable. `Timer with AutoReset = false works` is left alone: exactly one tick is the invariant it exists to check, not an artifact of timing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dbrattli
force-pushed
the
fix/flaky-timer-tests
branch
from
July 13, 2026 07:28
7588072 to
fa6a406
Compare
`equal` is `equal expected actual`, so passing the observed counter first made failure messages print the two sides reversed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Timer.Elapsed.Subscribe worksfailed on thewindows-latestJavaScript leg of an unrelated PR (run) withExpected: 10 - Actual: 5. It is a wall-clock race, and so is its siblingTimer with AutoReset = true works.The race
The test asserts the handler ran exactly twice inside a 125ms sleep. The second tick is due at 100ms, so there are 25ms of slack. On a loaded CI runner — where Windows timer resolution is ~15.6ms and scheduling is coarse — that tick slips past the window, one tick arrives, and
resis 5. The same job also reported the Main.js file itself hitting its 20s timeout, which is the signature of a runner that was simply slow at that moment.Nothing about the count of 2 is part of the contract being tested; it is an artifact of the sleep being 2.5× the interval.
The fix
Assert the semantics each test is named for:
AutoReset = truemeans, and what a live subscription means), andThe counter is read after
Stop()/Dispose(), so a tick cannot land between the read and the unsubscribe and make the second assertion flaky in turn.This is the same tolerance the neighbouring
Assigning an event to a variable works(#1863) already uses — 200ms sleep, at least two ticks (acc > 2 |> equal true) — which has been stable, so the new budget is a proven one in this file rather than a guess.Timer with AutoReset = false worksis deliberately left alone: it asserts exactly one tick, which is the invariant it exists to check (AutoReset off ⇒ fires once), not a timing artifact.Testing
Full JavaScript suite: 3049 passing, 0 failed. The two rewritten tests take ~300ms each, up from ~175ms — negligible against the 20s per-file budget.
🤖 Generated with Claude Code