test: give each test its own PredBat rather than sharing one (#5079) - #5102
Conversation
|
CI red here is not this change - it is a pre-existing bug this branch is missing the fix for. The run failed at 22:13 UTC, and There is a second, opposite instance of the same mistake in Test 5, which builds an "already expired" timestamp the same way and so fails before about 11:00 local - fixed in #5104.
Will restack this on top of #5100 and #5104 so CI here is meaningful. |
Every registered test ran against a single PredBat built once at startup, so a test that mutated instance state and did not restore it broke whichever unrelated test happened to follow - depending only on the order they ran in. That has produced a long series of order-dependent failures (#5079), each previously fixed by hand-writing a snapshot/restore block naming the fields that particular test touched. Building the fixture per test removes the whole class instead. Measured on the 53 known-bad (culprit, victim) orderings collected so far: 53 of 53 fail before this change, 0 of 53 after (with #5100, which fixes the one test that also fails on its own). The reason this was not done sooner appears to be an assumption that rebuilding would be too slow. It is not - create_predbat() is about 22ms, and the full suite measures 174s shared against 173s per-test, i.e. no measurable difference, since the suite's cost is dominated by the prediction work inside the tests rather than by fixture setup. Also confirms the tests themselves are sound: all 316 pass when run individually in their own process, so every ordering failure was shared-fixture contamination rather than a test depending on an earlier one to set it up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
65db75f to
89bf8e9
Compare
59e0e5a
into
fix/plan-persistence-real-clock-expiry
Copilot review on #5100: the two fixes made the tests pass, but not fail when the bug returns. Both clocks normally read close to the same value, and an expiry of now_utc + 8h is only wrong once they have drifted more than 8 hours apart - so between roughly 11:00 and 20:00 the buggy implementations passed as well, and a revert would go unnoticed for most of the working day. Confirmed by mutation test rather than assumed: reinstating the old save_plan() left plan_persistence passing. Each test now pins now_utc a year away from real time, in the direction that exposes its own mistake - back for the round trip (a now_utc-derived expiry is then already past, so the plan fails to load) and forward for the stale-plan check (a now_utc-derived "past" expiry is then still ahead of real time, so the plan loads when it should not). Both mutations are now caught at any hour. now_utc needs no restoring here: each test gets a fresh PredBat (#5102). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…, drop dead test save/restore car_charging_now_confirmed_slots/streak_last_read store minutes-since- midnight_utc, but midnight_utc is recomputed fresh from real time every cycle. A slot confirmed yesterday (e.g. 840 for 14:00) survived the 24h prune unchanged - the prune bound is itself derived from today's minutes_now - and became indistinguishable from today's slot 840, letting "started" trust a future slot on a sensor reading from the day before (Copilot review on PR #5110). get_car_charging_planned() now tracks the midnight_utc both structures are keyed against and rebases every stored slot number by the day's elapsed minutes whenever it changes. Also drops test_car_charging_now_confirmed_slots.py's old save/restore-state block and its trailing get_car_charging_planned() cleanup call (also flagged by Copilot): both were protecting a since-removed shared PredBat fixture that PR #5102 replaced with a fresh instance per test, so there is nothing left for a later test to inherit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…, drop dead test save/restore car_charging_now_confirmed_slots/streak_last_read store minutes-since- midnight_utc, but midnight_utc is recomputed fresh from real time every cycle. A slot confirmed yesterday (e.g. 840 for 14:00) survived the 24h prune unchanged - the prune bound is itself derived from today's minutes_now - and became indistinguishable from today's slot 840, letting "started" trust a future slot on a sensor reading from the day before (Copilot review on PR #5110). get_car_charging_planned() now tracks the midnight_utc both structures are keyed against and rebases every stored slot number by the day's elapsed minutes whenever it changes. Also drops test_car_charging_now_confirmed_slots.py's old save/restore-state block and its trailing get_car_charging_planned() cleanup call (also flagged by Copilot): both were protecting a since-removed shared PredBat fixture that PR #5102 replaced with a fresh instance per test, so there is nothing left for a later test to inherit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
(Written by Claude, on behalf of Rik.)
Test infrastructure only - no user-facing or product code changed. One six-line change in
apps/predbat/unit_test.py.Problem
Every registered test runs against a single
PredBatbuilt once at startup. A test that mutates instance state and doesn't restore it therefore breaks whichever unrelated test happens to run next - depending only on the order they ran in. That's the pattern tracked in #5079, and it has produced a long series of confusing failures where the test that reports the error is not the test at fault.Until now each one was fixed by hand-writing a snapshot/restore block naming the fields that particular test touched. That approach scales with the number of tests and has proven error-prone: the first attempt at one such fix missed 13 fields, and a follow-up audit found 7 more.
Change
Build the fixture per test instead, so leaked state cannot carry over at all.
Measurements
Known-bad orderings (the 53 (culprit, victim) pairs collected so far by the fuzzer in #5096):
(The two involving
plan_persistenceneed #5100 as well - that test also fails on its own, for an unrelated product bug.)Runtime - the assumption that rebuilding would be too expensive turns out not to hold.
create_predbat()is ~22ms, and the suite's cost is dominated by the prediction work inside the tests, not fixture setup:No measurable difference either way.
The tests themselves are sound: all 316 pass when run individually, each in its own process. So every ordering failure found was shared-fixture contamination, not a test quietly depending on an earlier one to set it up. (The single exception was
plan_persistence, which failed alone because of the product bug fixed in #5100 - found precisely because isolation was checked.)Consequences
Test plan
./run_pre_commitpasses