test(predict-pv-power): delete fixture attrs on restore instead of setting None - #4378
Open
chalfontchubby wants to merge 1 commit into
Open
Conversation
…tting None load_minutes_step/load_minutes_step10/prediction aren't set until a real update_pred() cycle or calculate_plan() runs, so on a fresh my_predbat fixture they may not exist as attributes at all. The finally-block restore was unconditionally assigning the captured (possibly None) value back, which left them present-but-None when they hadn't existed before - worse than the original missing-attribute state, and can cause order-dependent failures in later tests that expect a dict/Prediction object or use hasattr() to guard access. Track presence up front and delete the attribute on restore when it was absent to begin with, matching what a later test would actually see on a truly fresh fixture (Copilot review finding on springfall2008#4374, merged before the review landed).
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.
Summary
Follow-up to #4374 (merged) - a Copilot review comment landed after the merge, flagging a real fixture-hygiene bug in the new regression test:
Verified directly:
load_minutes_step/load_minutes_step10are only ever set at the end of a realupdate_pred()cycle (predbat.py:1114-1115), andself.predictionis only ever assigned when a realPredictionobject gets constructed (plan.py:1080) - neither exists on a freshly-created test fixture before this test runs. Thefinallyblock'sgetattr(..., None)+ unconditional restore left them present-but-Noneafterwards, worse than the original missing-attribute state.Change
Track attribute presence up front (
hasattr), and on restore,delattrrather than assignNonefor any attribute that didn't exist beforehand - matching what a genuinely fresh fixture would look like.Test plan
unit_test.py --test predict_pv_power- passesunit_test.py --quick- full suite passes, no order-dependent failures🤖 Generated with Claude Code