From 40fba72b9f92a79bdc3162a74bc20358e2b15f1d Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 28 Jul 2026 13:51:37 -0500 Subject: [PATCH] =?UTF-8?q?fix(tests):=20the=20cron=20no-fire=20test=20fai?= =?UTF-8?q?led=20for=20being=20right=20=E2=80=94=20anchor=20the=20clock=20?= =?UTF-8?q?instead=20of=20sampling=20one?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/test_timer_source.py | 70 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 67 insertions(+), 3 deletions(-) diff --git a/tests/test_timer_source.py b/tests/test_timer_source.py index a25597bf..d1eeb385 100644 --- a/tests/test_timer_source.py +++ b/tests/test_timer_source.py @@ -11,6 +11,7 @@ from __future__ import annotations import asyncio +import time from datetime import datetime, timedelta from zoneinfo import ZoneInfo @@ -356,15 +357,52 @@ def test_timer_timezone_requires_cron() -> None: ) -async def test_timer_cron_does_not_fire_immediately() -> None: - # With a real cron the first fire is the next scheduled minute — never t=0. Over a short window the - # every-minute cron must not have fired (its next fire is up to ~60s away). +def test_cron_next_fire_is_always_strictly_future() -> None: + """The actual invariant, asserted purely: ``next_after`` never returns its own input. + + This is what "a cron does not fire at t=0" MEANS, and it needs no clock and no sleeping. The + boundary offsets are the point — at :59.999 the next fire is a millisecond away, and that is + correct behaviour, not a bug to sleep through. + """ + sched = _CronSchedule.parse("* * * * *") + base = datetime(2026, 7, 27, 12, 0) + for offset in (0.0, 0.001, 1.0, 30.0, 59.0, 59.999): + t = base + timedelta(seconds=offset) + nxt = sched.next_after(t) + assert nxt > t, f"next_after({t}) must be strictly future, got {nxt}" + assert (nxt.second, nxt.microsecond) == (0, 0), ( + f"a minute cron fires on the minute, got {nxt}" + ) + + +async def test_timer_cron_does_not_fire_immediately(monkeypatch: pytest.MonkeyPatch) -> None: + """The running loop must not fire at t=0 either — on a clock ANCHORED mid-minute. + + This used to start a real ``* * * * *`` source, sleep 0.1s, and assert nothing had fired. That is + only reliable mid-minute: 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 being right — ~0.17% per run + per leg (a 0.1s window out of every 60s). It duly red-X'd a comments-only PR. Its own comment gave + it away: "its next fire is up to ~60s away". Up to. It can also be 50ms away. + + THE CLOCK ADVANCES; ONLY ITS ORIGIN IS PINNED. A frozen clock was tried first and is WRONG: with + ``_now()`` constant, ``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. That version was + deterministic because it could not fail, which is worse than the flake it replaced. Anchoring the + origin at :30 and letting it advance in real time keeps the failure mode reachable (the sibling test + below pins that) while removing the dependence on where the wall clock happened to be. + + The source is naive-clocked here (no ``timezone`` setting), so a naive datetime is the right shape. + """ fired: list[bytes] = [] async def handler(raw: bytes) -> None: fired.append(raw) src = _timer(body="X", cron_expression="* * * * *") + origin, t0 = datetime(2026, 7, 27, 12, 0, 30), time.monotonic() # :30 — 30s short of a fire + monkeypatch.setattr( + type(src), "_now", lambda _self: origin + timedelta(seconds=time.monotonic() - t0) + ) await src.start(handler) try: await asyncio.sleep(0.1) @@ -373,6 +411,32 @@ async def handler(raw: bytes) -> None: await src.stop() +async def test_the_cron_no_fire_probe_can_actually_fail(monkeypatch: pytest.MonkeyPatch) -> None: + """Guards the guard: anchored just BEFORE a fire, the same setup must fire. + + Without this, the test above passes whether the loop is correct, broken, or wedged — and the first + attempt at fixing it (a frozen clock) was exactly that: unfalsifiable. Anchoring at :59.90 puts a + scheduled fire 100ms out, inside the window, so a fire is the CORRECT outcome and its absence means + the probe has stopped observing anything. + """ + fired: list[bytes] = [] + + async def handler(raw: bytes) -> None: + fired.append(raw) + + src = _timer(body="X", cron_expression="* * * * *") + origin, t0 = datetime(2026, 7, 27, 12, 0, 59, 900_000), time.monotonic() + monkeypatch.setattr( + type(src), "_now", lambda _self: origin + timedelta(seconds=time.monotonic() - t0) + ) + await src.start(handler) + try: + await asyncio.sleep(0.6) # generous: the fire is ~100ms in, the slack absorbs a slow runner + assert fired, "the no-fire probe above cannot fail — it would pass on a wedged loop too" + finally: + await src.stop() + + async def test_timer_cron_fires_when_due() -> None: # Exercise the cron loop end-to-end without waiting a real minute by injecting a fast schedule. fired: list[bytes] = []