Repository navigation
fix(tests): restore now_utc/midnight_utc leaked by test_octopus_url (#5079) - #5098
Closed
chalfontchubby wants to merge 2 commits into
Closed
chalfontchubby wants to merge 2 commits into
chalfontchubby wants to merge 2 commits into
Conversation
chalfontchubby
added
unit_test
INFRA
…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
force-pushed
the
fix/test-order-leak-octopus-url-now-utc
branch
from
September 14, 2026 20:33
7ddb4c0 to
059e84d
Compare
…nctions (#5079) test_async_get_day_night_rates, _test_get_saving_session_data, and test_async_intelligent_update_sensor each pin my_predbat.now_utc_exact to a fixed test date and never clear it - unlike the other fields already fixed in this file (#5079 PR), now_utc_exact isn't normally present on the fixture at all (mock_base.py only sets it as part of a real PredBat's construction, which this test fixture bypasses), so the correct restore is removing the instance attribute entirely when this test is what introduced it, not reassigning it to some prior value. Checked every other test file that assigns now_utc_exact (test_solis, test_solcast, test_ge_cloud, test_db_manager, open_meteo_live) - all of them set it on a private per-test mock object, not the shared my_predbat fixture, so none of those are a leak of this kind. 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
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.
(Written by Claude, on behalf of Rik.)
Test-only fix - no user-facing or production code changed.
What and why
test_edf_freephase_dynamic_url(intest_octopus_url.py) pinsmidnight_utc/now_utcto the test data's date (2025-12-17) and never restores them - already flagged by the code's own comment, added in #5054: "this pin is left behind for later test modules".That #5054 change also made this leak more consequential than it used to be:
ComponentBase.midnight_utcnow derives fromnow_utcrather than being read independently, so any later test that readsmidnight_utcthrough a component now inherits the wrong value too, not just directnow_utcreaders.Snapshot both fields at the top of the function and restore them at the end - same pattern as #5097.
Does not retire
octopus_url -> plan_persistenceWorth being explicit about this rather than implying the fix closes that pair: confirmed by testing the pair after this fix, it still fails. That pair's real cause (found separately, not by this branch) is
storage.pychecking a saved plan's expiry against real wall-clock time rather than the fixture'snow_utc- a genuine time-of-day-dependent bug, unrelated to what ran beforeplan_persistence. That's separate follow-up work, tracked on #5079.This commit is still worth landing on its own regardless: the leak is real, and #5054 widened its blast radius.
Test plan
./run_pre_commitpassestest_octopus_urlstill passes standalone./run_all --quick(full suite) still passes--test octopus_url --test plan_persistence) that this fix does not (and isn't claimed to) retire that pair