fix(tests): the cron no-fire test failed for being right — anchor the clock instead of sampling one - #23
Merged
Merged
Conversation
… clock instead of sampling one test_timer_cron_does_not_fire_immediately started a real `* * * * *` source, slept 0.1s, and asserted nothing had fired. A minute cron fires ON the minute, so a run beginning within 100ms of the boundary saw a fire that was entirely CORRECT and the test failed for it: a 0.1s window out of every 60s is ~0.17% per run, per leg. It duly red-X'd a PR whose diff was four workflow COMMENT blocks. The old comment gave it away -- "its next fire is up to ~60s away". Up to. It can also be 50ms away. Three changes: 1. A PURE test of the invariant. "Does not fire at t=0" means `next_after(t) > t`, which needs no clock and no sleeping. Asserted across offsets INCLUDING :59.999, where the next fire is a millisecond out and that is correct -- the exact case the old test treated as failure. 2. The loop test now anchors the clock's ORIGIN mid-minute (:30) and lets it advance in real time, so the fire is provably 30s away regardless of where the wall clock happens to be. 3. A falsifiability guard, and it is the reason this commit is not two lines. MY FIRST FIX WAS WRONG IN THE MOST INSTRUCTIVE WAY. I froze `_now()` outright. That is deterministic and it is unfalsifiable: with a constant clock `remaining` never decreases, so `_run_cron` waits forever and cannot fire at ANY pinned time -- including :59.95, where firing is exactly what should happen. I only caught it because I probed the mechanism rather than trusting 40 green runs: frozen at :59.95 the source fired 0 times, which should have been impossible. A test made deterministic by being unable to fail is worse than the flake it replaced, and that is precisely the failure mode this codebase keeps producing. So test_the_cron_no_fire_probe_can_actually_fail anchors at :59.90, putting a scheduled fire 100ms inside the window, and REQUIRES the fire. Mutation-verified: wedge the loop (`if self._may_fire():` -> `if False:`) and the guard fails while the no-fire test still passes -- demonstrating that the no-fire test alone would accept a completely broken cron loop. Verified: 42 pass; the guard fails under the wedge mutation and passes without it.
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
1 participant
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.
test_timer_cron_does_not_fire_immediatelystarted a real* * * * *source, slept 0.1s, and asserted nothing had fired.A minute cron fires on the minute. A run beginning within 100 ms of the boundary saw a fire that was entirely correct — and the test failed for it. A 0.1s window out of every 60s is ~0.17% per run, per leg, and it duly red-X'd #21, whose diff was four workflow comment blocks.
Its own comment gave it away: "its next fire is up to ~60s away." Up to. It can also be 50 ms away.
Three changes
1. A pure test of the invariant. "Does not fire at t=0" means
next_after(t) > t— no clock, no sleeping. Asserted across offsets including:59.999, where the next fire is a millisecond out and that is correct: the exact case the old test treated as a failure.2. The loop test anchors the clock's origin mid-minute (
:30) and lets it advance in real time, so the fire is provably 30s away regardless of where the wall clock happens to be.3. A falsifiability guard — and it's the reason this isn't a two-line diff.
My first fix was wrong in the most instructive way
I froze
_now()outright. That's deterministic, and it's unfalsifiable: with a constant clockremainingnever decreases, so_run_cronwaits forever and cannot fire at any pinned time — including:59.95, where firing is exactly what should happen.I only caught it by probing the mechanism instead of trusting 40 green runs:
A test made deterministic by being unable to fail is worse than the flake it replaced — and that is precisely the failure mode this codebase keeps producing.
So
test_the_cron_no_fire_probe_can_actually_failanchors at:59.90, putting a scheduled fire 100 ms inside the window, and requires it.Verification
Mutation-verified. Wedge the loop (
if self._may_fire():→if False:) and the guard fails while the no-fire test still passes — demonstrating that the no-fire test alone would accept a completely broken cron loop.42 tests pass; the guard fails under the wedge mutation and passes without it.
🤖 Generated with Claude Code