Repository navigation
fix(tests): restore forecast_minutes/minutes_now leaked by two tests (#5079) - #5097
Closed
chalfontchubby wants to merge 1 commit into
Closed
chalfontchubby wants to merge 1 commit into
chalfontchubby wants to merge 1 commit into
Conversation
chalfontchubby
added
unit_test
INFRA
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The forecast-history test still leaves shared fixture state mutated, so order-dependent failures can persist.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This test-only PR repairs shared PredBat fixture cleanup to reduce order-dependent failures.
Changes:
- Adds snapshot/restore cleanup for rate-scan state.
- Extends forecast-history cleanup for modified fields.
File summaries
| File | Summary |
|---|---|
apps/predbat/tests/test_rate_text_scan.py |
Restores modified rate-scan state with cleanup handling. |
apps/predbat/tests/test_load_forecast_history.py |
Cleanup still omits several mutated fixture attributes, including time and charging fields. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+91
to
+93
| original_minutes_now = my_predbat.minutes_now | ||
| original_forecast_minutes = my_predbat.forecast_minutes | ||
| original_car_charging_energy = my_predbat.car_charging_energy |
4 tasks done
…5079) test_load_forecast_history and test_rate_text_scan both mutate forecast_minutes/minutes_now (and, for the latter, end_record/rate_min/ rate_max/rate_import) on the shared PredBat fixture without restoring them, and each leaked value poisons whichever unrelated test happens to run next depending on shuffle order. load_forecast_history's leak specifically comes from its own trailing restore block calling fetch_config_options(), which re-derives forecast_minutes from apps.yaml (2880) rather than putting back the fixture's overridden value (1440) - a bug in the cleanup itself, not just a missing one. rate_text_scan restored nothing at all. Snapshotting and restoring these fields (following the try/finally pattern already used in test_inverter.py) retires 8 of the 27 known ordering-dependent pairs tracked on #5079 in one structural change per file, rather than patching each victim individually: load_forecast_history -> model_kernel load_forecast_history -> iboost_smart load_forecast_history -> model load_forecast_history -> optimise_windows_kernel rate_text_scan -> cloud_modulation rate_text_scan -> optimise_swap_charge rate_text_scan -> window_sort manual_soc -> cloud_modulation Verified against all 27 known pairs (not just these 8) via ./run_shuffle, and the full ./run_all --quick suite still passes. 19 pairs remain, tracked on #5079. Amended per Copilot review on this PR: test_load_forecast_history's cleanup was still missing several other mutated fixture attributes (now_utc, midnight_utc, load_minutes, load_minutes_age, car_charging_threshold, car_charging_rate, base_load, car_charging_hold, days_previous, days_previous_weight, forecast_days, holiday_days_left, prefix) - audited the whole function (every my_predbat.X assignment now has a matching snapshot+restore) rather than only the fields needed for the 8 pairs above. Doesn't retire any additional known pairs on its own; it closes latent gaps that could surface as a future one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
chalfontchubby
force-pushed
the
fix/test-order-leak-forecast-minutes
branch
from
September 14, 2026 20:32
2cd9bd2 to
e1be239
Compare
chalfontchubby
added a commit
that referenced
this pull request
Sep 14, 2026
…5079) test_edf_freephase_dynamic_url pins midnight_utc/now_utc to the test data's date (2025-12-17) and never restores them - already flagged in the code's own comment ("this pin is left behind for later test modules") added by PR #5054 (ComponentBase.midnight_utc now derives from now_utc, so this leak is more consequential than before: any later test that reads midnight_utc via a component picks up the wrong value too, not just now_utc readers). Snapshot both fields and restore them at the end of the test function, following the same pattern as PR #5097. Note: this does NOT retire octopus_url -> plan_persistence (still tracked on #5079) - that pair's real cause, confirmed separately, is storage.py checking a saved plan's expiry against real wall-clock time rather than the fixture's now_utc, which is a genuine time-of-day- dependent bug independent of what test ran before it. This commit is still worth landing on its own: the leak is real, and PR #5054 widened its blast radius by routing midnight_utc through now_utc. Amended following the same audit applied to #5097 (every my_predbat.X assignment in the function must have a matching snapshot+restore, not just the fields needed for the one known pair): also restores debug_enable, which was leaked the same way. failures_total is deliberately left alone - it's a pre-existing bug in the test itself (writing an attribute PredBat doesn't have; the OctopusAPI instance under test owns failures_total, not my_predbat), not a real leak, and snapshotting it would crash on the first run since the attribute doesn't exist yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
chalfontchubby
marked this pull request as draft
September 14, 2026 21:44
Collaborator
Author
|
Moved to draft while the approach is decided. These fix the leaks one test at a time, by naming the attributes each test mutates and restoring them. That has proven error-prone in review already (the first version of #5097 missed 13 fields, and an audit then found 7 more), and it scales with the number of tests rather than fixing the cause. A cheaper alternative looks viable: build a fresh fixture per test in the runner, so leaked state cannot carry over at all. Holding here until that is settled - and until there is maintainer agreement that order-dependent tests are worth fixing in the first place (#5079 has had no maintainer response yet). |
Collaborator
Author
|
Closing - keyhole fixing individual tests is not worth it with a holistic fix available |
This was referenced Sep 14, 2026
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.
(Written by Claude, on behalf of Rik.)
Test-only fix - no user-facing or production code changed. Fixes two test files that leave shared fixture state mutated, which was making unrelated later tests fail depending on run order (see #5079, and #5096 for the shuffle-fuzzer tooling this was found with).
Root cause
test_load_forecast_history.py: mutatesforecast_minutes/minutes_now/car_charging_energyon the sharedPredBatfixture. Its own trailing "restore" block callsfetch_config_options(), which re-derivesforecast_minutesfromapps.yaml(2880) rather than putting back the fixture's overridden value (1440) - the bug is in the cleanup itself, not just a missing one.test_rate_text_scan.py: overwritesminutes_now,end_record,forecast_minutes(down to9),rate_min,rate_max, and the entirerate_importdict, and restores none of it.Both now snapshot the fields they touch and restore them (
test_rate_text_scanin atry/finally;test_load_forecast_history's existing restore block extended), following the same pattern already used intest_inverter.py.Retires 8 of 27 known pairs, verified against all of them
One structural fix per file, not a patch per victim - re-ran every pair on
tools/shuffle_fuzz_known_pairs.txt(not just the two culprits' own known victims) against this branch:19 pairs remain (
kernel_static_cache,plot,octopus_url,metric_battery_cycle_fractional,multi_car_iog,calculate_yesterday, and several one-off culprits) - will keep chipping away and posting to #5079 as they're understood.Test plan
./run_pre_commitpasses./run_all --quick(full suite, registry order) still passes