Skip to content

Commit 53fa6bd

Browse files
fix(compare): keep a re-plan requested during the comparison, and restore state even if the config restore raises
Two gaps a cold review found in the restore: - Restoring plan_valid put back the True from before the comparison, overwriting a re-plan requested while it ran (an HA event, the web UI or an agent tool changing a setting sets update_pending and clears plan_valid). The next run would then treat the pre-change plan as valid. plan_valid is now restored as it was only if no re-plan is pending. - fetch_config_options() runs first in the final restore; if it raised, none of the saved state was put back - the leak this branch fixes. The attribute restore now runs in a finally around the config restore, still last so it wins. The coverage check now reads calculate_plan() and every optimise pass with ast, so augmented, annotated and tuple assignments to self are found too (nothing new is uncovered today). New tests: a re-plan requested during the comparison leaves the plan invalid, and the plan is restored when the config restore raises; both fail without this change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 20ad859 commit 53fa6bd

2 files changed

Lines changed: 122 additions & 21 deletions

File tree

‎apps/predbat/compare.py‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -735,15 +735,18 @@ def run_all(self, debug=False, fetch_sensor=True):
735735
self.save_yaml()
736736
self.publish_data()
737737
finally:
738-
# Restore config values overridden by any tariff's fetch_config() call
739-
if config_snapshot:
740-
for key, orig_value in config_snapshot.items():
741-
item = my_predbat.config_index.get(key)
742-
if item is not None:
743-
item["value"] = orig_value
744-
my_predbat.fetch_config_options()
745-
746-
# Restore original settings
747-
for name in COMPARE_RESTORED_STATE + COMPARE_RESTORED_COPIES:
748-
setattr(my_predbat, name, saved[name])
749-
my_predbat.io_adjusted = io_adjusted_base
738+
try:
739+
# Restore config values overridden by any tariff's fetch_config() call
740+
if config_snapshot:
741+
for key, orig_value in config_snapshot.items():
742+
item = my_predbat.config_index.get(key)
743+
if item is not None:
744+
item["value"] = orig_value
745+
my_predbat.fetch_config_options()
746+
finally:
747+
# Restore original settings, last so they win over fetch_config_options(), and even if it raised
748+
for name in COMPARE_RESTORED_STATE + COMPARE_RESTORED_COPIES:
749+
setattr(my_predbat, name, saved[name])
750+
# A re-plan requested while the comparison ran (a setting or override changed) must still happen
751+
my_predbat.plan_valid = saved["plan_valid"] and not my_predbat.update_pending
752+
my_predbat.io_adjusted = io_adjusted_base

‎apps/predbat/tests/test_compare.py‎

Lines changed: 107 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@
1818
reported a final SOC larger than the normal battery size)
1919
- run_all() restores every attribute it saves, the plan state calculate_plan() keeps across runs
2020
included (regression for plan_preclip left as the last tariff's plan), and plans each tariff from scratch
21-
- every attribute calculate_plan() assigns is either restored by run_all() or rebuilt on every run
21+
- every attribute calculate_plan() and its optimise passes assign is either restored by run_all() or rebuilt
22+
on every run
23+
- a re-plan requested during the comparison survives it, and the state is restored even if the config restore raises
2224
"""
2325

2426
import inspect
@@ -139,29 +141,125 @@ def run_single(*args, **kwargs):
139141

140142

141143
def test_plan_state_covered(my_predbat):
142-
"""Every attribute calculate_plan() assigns is restored by the comparison or rebuilt on every run.
144+
"""Every attribute calculate_plan() and the optimise passes it runs assign is restored by the comparison or
145+
rebuilt on every run.
143146
144147
A new piece of plan state that outlives a run (as plan_preclip did) must be added to COMPARE_PLAN_STATE, or the
145-
comparison will leave the last tariff's value behind.
148+
comparison will leave the last tariff's value behind. Read from the source with ast, so plain, augmented,
149+
annotated and tuple assignments to self.<name> are all found.
146150
"""
147-
import re
148-
149-
source = inspect.getsource(type(my_predbat).calculate_plan)
150-
assigned = set(re.findall(r"self\.([a-z_0-9]+)\s*=(?!=)", source))
151+
import ast
152+
import textwrap
153+
154+
plan_class = type(my_predbat)
155+
methods = ["calculate_plan"] + sorted(name for name in dir(plan_class) if name.startswith("optimise"))
156+
assigned = set()
157+
for name in methods:
158+
tree = ast.parse(textwrap.dedent(inspect.getsource(getattr(plan_class, name))))
159+
for node in ast.walk(tree):
160+
targets = node.targets if isinstance(node, ast.Assign) else [node.target] if isinstance(node, (ast.AugAssign, ast.AnnAssign)) else []
161+
for target in targets:
162+
for part in ast.walk(target):
163+
if isinstance(part, ast.Attribute) and isinstance(part.value, ast.Name) and part.value.id == "self":
164+
assigned.add(part.attr)
151165
uncovered = sorted(assigned - set(COMPARE_PLAN_STATE) - set(CALCULATE_PLAN_REBUILT))
152166
if uncovered:
153-
print("ERROR: calculate_plan() assigns {} - add each to COMPARE_PLAN_STATE (compare.py) if it outlives the run, or to CALCULATE_PLAN_REBUILT here if it is rebuilt before use".format(uncovered))
167+
print("ERROR: calculate_plan() or an optimise pass assigns {} - add each to COMPARE_PLAN_STATE (compare.py) if it outlives the run, or to CALCULATE_PLAN_REBUILT here if it is rebuilt before use".format(uncovered))
154168
return 1
155-
print("PASS: every attribute calculate_plan() assigns is restored or rebuilt")
169+
print("PASS: every attribute calculate_plan() and its {} optimise passes assign is restored or rebuilt".format(len(methods) - 1))
156170
return 0
157171

158172

173+
def test_run_all_keeps_pending_replan(my_predbat):
174+
"""A re-plan requested while the comparison runs (a setting or override changed) still invalidates the plan."""
175+
saved_compare_list = my_predbat.args.get("compare_list")
176+
saved_plan_valid, saved_pending = my_predbat.plan_valid, my_predbat.update_pending
177+
my_predbat.args["compare_list"] = [{"id": "tariff_a", "name": "Tariff A"}]
178+
my_predbat.plan_valid, my_predbat.update_pending = True, False
179+
comparison = Compare.__new__(Compare)
180+
comparison.pb = my_predbat
181+
comparison.log = my_predbat.log
182+
comparison.comparisons = {}
183+
184+
def run_single(*args, **kwargs):
185+
"""Stand in for a tariff's run during which the user changes a setting."""
186+
my_predbat.update_pending = True
187+
return None
188+
189+
comparison.run_single = run_single
190+
comparison.select_best = lambda *args, **kwargs: None
191+
comparison.save_yaml = lambda *args, **kwargs: None
192+
comparison.publish_data = lambda *args, **kwargs: None
193+
try:
194+
comparison.run_all(fetch_sensor=False)
195+
if my_predbat.plan_valid:
196+
print("ERROR: a re-plan requested during the comparison should leave the plan invalid")
197+
return 1
198+
print("PASS: run_all() keeps a re-plan requested during the comparison")
199+
return 0
200+
finally:
201+
my_predbat.plan_valid, my_predbat.update_pending = saved_plan_valid, saved_pending
202+
if saved_compare_list is None:
203+
my_predbat.args.pop("compare_list", None)
204+
else:
205+
my_predbat.args["compare_list"] = saved_compare_list
206+
207+
208+
def test_run_all_restores_when_config_restore_raises(my_predbat):
209+
"""The saved state is put back even when restoring a tariff's config raises."""
210+
item = my_predbat.config_index.get("metric_battery_cycle")
211+
saved_compare_list = my_predbat.args.get("compare_list")
212+
saved_preclip, saved_value = my_predbat.plan_preclip, item["value"]
213+
plan_in_force = ([18.08], [{"start": 0, "end": 90, "average": 7.62}], [], [])
214+
my_predbat.plan_preclip = plan_in_force
215+
my_predbat.args["compare_list"] = [{"id": "tariff_a", "name": "Tariff A", "config": {"metric_battery_cycle": 9.0}}]
216+
comparison = Compare.__new__(Compare)
217+
comparison.pb = my_predbat
218+
comparison.log = my_predbat.log
219+
comparison.comparisons = {}
220+
221+
def run_single(*args, **kwargs):
222+
"""Stand in for a tariff's run, which replaces the plan."""
223+
my_predbat.plan_preclip = ("tariff plan",)
224+
return None
225+
226+
def fetch_config_options():
227+
"""Fail as a broken config refresh would."""
228+
raise ValueError("config refresh failed")
229+
230+
comparison.run_single = run_single
231+
comparison.select_best = lambda *args, **kwargs: None
232+
comparison.save_yaml = lambda *args, **kwargs: None
233+
comparison.publish_data = lambda *args, **kwargs: None
234+
my_predbat.fetch_config_options = fetch_config_options
235+
try:
236+
try:
237+
comparison.run_all(fetch_sensor=False)
238+
except ValueError:
239+
pass
240+
if my_predbat.plan_preclip is not plan_in_force:
241+
print("ERROR: run_all() should restore the plan even when the config restore raises, got {}".format(my_predbat.plan_preclip))
242+
return 1
243+
print("PASS: run_all() restores the saved state when the config restore raises")
244+
return 0
245+
finally:
246+
del my_predbat.fetch_config_options
247+
my_predbat.plan_preclip = saved_preclip
248+
item["value"] = saved_value
249+
if saved_compare_list is None:
250+
my_predbat.args.pop("compare_list", None)
251+
else:
252+
my_predbat.args["compare_list"] = saved_compare_list
253+
254+
159255
def test_compare(my_predbat):
160256
"""Run all compare unit tests."""
161257
failed = 0
162258
print("**** Running compare tests ****\n")
163259
failed += test_run_all_restores_state(my_predbat)
164260
failed += test_plan_state_covered(my_predbat)
261+
failed += test_run_all_keeps_pending_replan(my_predbat)
262+
failed += test_run_all_restores_when_config_restore_raises(my_predbat)
165263

166264
# ------------------------------------------------------------------
167265
# T1: apply_hardware_overrides – soc_max override

0 commit comments

Comments
 (0)