From 1cd1183b678a22959cf0e100183e366728f9c806 Mon Sep 17 00:00:00 2001 From: Rik Allen Date: Fri, 9 Oct 2026 18:33:40 +0100 Subject: [PATCH 1/4] fix(compare): restore the plan in force's pre-clip copy after the tariff comparison Plan selection scores the pre-clip version of each plan (#4403), keeping the plan in force's copy in plan_preclip for the next re-plan to score as the previous plan. The tariff comparison runs calculate_plan() for every tariff, each replacing plan_preclip with that tariff's 48-hour plan, and restored the windows and limits but not plan_preclip. The first re-plan after the midnight comparison therefore scored the last tariff's plan as the plan in force: on 9 Oct 2026 (Sigenergy, IOG) live scored its previous plan at 300.97p where the same plan scores 30.71p without the comparison, and so switched plans regardless of metric_min_improvement_plan. Found with the forward log replay (#5363): running the comparison inside the replay reproduced the inflated score, and restoring each attribute the comparison changes, group by group, isolated plan_preclip. run_all() now saves and restores it with the windows; a test runs run_all() with a tariff that replaces it and checks it comes back (it fails without the fix). Co-Authored-By: Claude Opus 5.5 --- apps/predbat/compare.py | 4 +++ apps/predbat/tests/test_compare.py | 44 ++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/apps/predbat/compare.py b/apps/predbat/compare.py index e15fb1d78..f35ea5b93 100644 --- a/apps/predbat/compare.py +++ b/apps/predbat/compare.py @@ -618,6 +618,9 @@ def run_all(self, debug=False, fetch_sensor=True): save_export_window_best = my_predbat.export_window_best save_export_limits_best = my_predbat.export_limits_best save_charge_limit_best = my_predbat.charge_limit_best + # The pre-clip copy of the plan in force, which the next re-plan scores as the previous plan; each tariff's + # calculate_plan() replaces it with its own plan + save_plan_preclip = my_predbat.plan_preclip save_cost_today_sofar = my_predbat.cost_today_sofar save_carbon_today_sofar = my_predbat.carbon_today_sofar save_iboost_today = my_predbat.iboost_today @@ -735,6 +738,7 @@ def run_all(self, debug=False, fetch_sensor=True): my_predbat.export_window_best = save_export_window_best my_predbat.export_limits_best = save_export_limits_best my_predbat.charge_limit_best = save_charge_limit_best + my_predbat.plan_preclip = save_plan_preclip my_predbat.cost_today_sofar = save_cost_today_sofar my_predbat.carbon_today_sofar = save_carbon_today_sofar my_predbat.iboost_today = save_iboost_today diff --git a/apps/predbat/tests/test_compare.py b/apps/predbat/tests/test_compare.py index ac7629f31..6f9375d6b 100644 --- a/apps/predbat/tests/test_compare.py +++ b/apps/predbat/tests/test_compare.py @@ -68,10 +68,54 @@ def _make_compare(): return cmp, pb +def test_run_all_restores_plan_preclip(my_predbat): + """run_all() puts back the pre-clip copy of the plan in force, which each tariff's calculate_plan() replaces. + + Left as the last tariff's 48-hour plan, the first re-plan after the midnight comparison scored that plan as the + previous one, so it always switched plans whatever metric_min_improvement_plan said (seen live on 9 Oct 2026: + previous plan 300.97p against 30.71p when the comparison is not run). + """ + from compare import Compare + + saved_compare_list = my_predbat.args.get("compare_list") + saved_preclip = my_predbat.plan_preclip + plan_in_force = ([18.08], [{"start": 0, "end": 90, "average": 7.62}], [], []) + my_predbat.plan_preclip = plan_in_force + my_predbat.args["compare_list"] = [{"id": "test_tariff", "name": "Test tariff"}] + comparison = Compare.__new__(Compare) + comparison.pb = my_predbat + comparison.log = my_predbat.log + comparison.comparisons = {} + + def run_single(*args, **kwargs): + """Stand in for a tariff's run: its calculate_plan() replaces plan_preclip with that tariff's plan.""" + my_predbat.plan_preclip = ([0.0], [{"start": 0, "end": 2880, "average": 30.0}], [], []) + return None + + comparison.run_single = run_single + comparison.select_best = lambda *args, **kwargs: None + comparison.save_yaml = lambda *args, **kwargs: None + comparison.publish_data = lambda *args, **kwargs: None + try: + comparison.run_all(fetch_sensor=False) + if my_predbat.plan_preclip is not plan_in_force: + print("ERROR: run_all() should restore the plan in force's pre-clip copy, got {}".format(my_predbat.plan_preclip)) + return 1 + print("PASS: run_all() restores plan_preclip") + return 0 + finally: + my_predbat.plan_preclip = saved_preclip + if saved_compare_list is None: + my_predbat.args.pop("compare_list", None) + else: + my_predbat.args["compare_list"] = saved_compare_list + + def test_compare(my_predbat): """Run all compare unit tests.""" failed = 0 print("**** Running compare tests ****\n") + failed += test_run_all_restores_plan_preclip(my_predbat) # ------------------------------------------------------------------ # T1: apply_hardware_overrides – soc_max override From 20ad859662e834eb99d8bfda503c996e9c233e34 Mon Sep 17 00:00:00 2001 From: Rik Allen Date: Fri, 9 Oct 2026 19:30:39 +0100 Subject: [PATCH 2/4] refactor(compare): save and restore a declared set of state, and plan each tariff from scratch The comparison kept a hand-written list of attributes to save and restore, and plan_preclip leaked because it was missing from it; end_record, the two rate_best_cost thresholds and plan_valid / plan_last_updated were missing too, so after the comparison the next cycle published the last tariff's thresholds and treated the restored plan as freshly computed. run_all() now saves and restores the attributes declared in COMPARE_PLAN_STATE, COMPARE_HARDWARE_STATE (also reset after each tariff) and the rest of COMPARE_RESTORED_STATE, with the car state in COMPARE_RESTORED_COPIES deep-copied as before. Each tariff now plans from scratch (plan_valid False before its run): scored against the previous tariff's plan, the keep-or-switch threshold in calculate_plan() could leave one tariff's result as another tariff's plan. Tests: run_all() with a tariff run that changes every saved attribute must restore all of them, reset the hardware values between tariffs and start each tariff with plan_valid False; and every attribute calculate_plan() assigns must be either in COMPARE_PLAN_STATE or listed as rebuilt every run, so the next new piece of plan state fails a test rather than leaking (checked: dropping end_record from the set fails it). T19 and T25 now check the declared sets rather than the old source text. Co-Authored-By: Claude Opus 5.5 --- apps/predbat/compare.py | 144 +++++++++++++---------------- apps/predbat/tests/test_compare.py | 120 +++++++++++++++--------- 2 files changed, 141 insertions(+), 123 deletions(-) diff --git a/apps/predbat/compare.py b/apps/predbat/compare.py index f35ea5b93..59d311c00 100644 --- a/apps/predbat/compare.py +++ b/apps/predbat/compare.py @@ -27,6 +27,60 @@ # 3. Consider Octopus API key to access current tariff info and switch links +# What Compare.run_all() saves before comparing tariffs and restores afterwards: every tariff fetches, plans and +# predicts on the live instance. The plan state is what calculate_plan() writes that outlives the run - leaving +# plan_preclip as the last tariff's plan once made the first re-plan after midnight score the wrong plan as the +# plan in force. Add any new plan state calculate_plan() keeps across runs here. +COMPARE_PLAN_STATE = ( + "charge_window_best", + "charge_limit_best", + "export_window_best", + "export_limits_best", + "plan_preclip", + "end_record", + "plan_valid", + "plan_last_updated", + "plan_last_updated_minutes", + "rate_best_cost_threshold_charge", + "rate_best_cost_threshold_export", +) +# Battery and inverter values a tariff can override, reset after each tariff as well as at the end +COMPARE_HARDWARE_STATE = ("soc_max", "battery_rate_max_charge", "battery_rate_max_charge_dc", "battery_rate_max_discharge", "battery_rate_max_export", "inverter_limit") +COMPARE_RESTORED_STATE = ( + COMPARE_PLAN_STATE + + COMPARE_HARDWARE_STATE + + ( + "forecast_plan_hours", + "forecast_minutes", + "forecast_days", + "manual_charge_times", + "manual_export_times", + "manual_freeze_charge_times", + "manual_freeze_export_times", + "manual_demand_times", + "manual_all_times", + "cost_today_sofar", + "carbon_today_sofar", + "iboost_today", + "iboost_plan", + "import_today_now", + "export_today_now", + "octopus_intelligent_charging", + "soc_kw", + ) +) +# Car state the comparison rebuilds per tariff, deep-copied as the lists are changed in place +COMPARE_RESTORED_COPIES = ( + "car_charging_plan_smart", + "car_charging_limit", + "car_charging_limit_model", + "car_charging_soc", + "car_charging_battery_size", + "car_charging_slots", + "car_charging_now_slots", +) + + class Compare: """Tariff comparison engine. @@ -605,43 +659,9 @@ def run_all(self, debug=False, fetch_sensor=True): my_predbat = self.pb - save_forecast_plan_hours = my_predbat.forecast_plan_hours - save_forecast_minutes = my_predbat.forecast_minutes - save_forecast_days = my_predbat.forecast_days - save_manual_charge_times = my_predbat.manual_charge_times - save_manual_export_times = my_predbat.manual_export_times - save_manual_freeze_charge_times = my_predbat.manual_freeze_charge_times - save_manual_freeze_export_times = my_predbat.manual_freeze_export_times - save_manual_demand_times = my_predbat.manual_demand_times - save_manual_all_times = my_predbat.manual_all_times - save_charge_window_best = my_predbat.charge_window_best - save_export_window_best = my_predbat.export_window_best - save_export_limits_best = my_predbat.export_limits_best - save_charge_limit_best = my_predbat.charge_limit_best - # The pre-clip copy of the plan in force, which the next re-plan scores as the previous plan; each tariff's - # calculate_plan() replaces it with its own plan - save_plan_preclip = my_predbat.plan_preclip - save_cost_today_sofar = my_predbat.cost_today_sofar - save_carbon_today_sofar = my_predbat.carbon_today_sofar - save_iboost_today = my_predbat.iboost_today - save_iboost_plan = my_predbat.iboost_plan - save_import_today_now = my_predbat.import_today_now - save_export_today_now = my_predbat.export_today_now - save_octopus_intelligent_charging = my_predbat.octopus_intelligent_charging - save_car_charging_plan_smart = copy.deepcopy(my_predbat.car_charging_plan_smart) - save_car_charging_limit = copy.deepcopy(my_predbat.car_charging_limit) - save_car_charging_limit_model = copy.deepcopy(my_predbat.car_charging_limit_model) - save_car_charging_soc = copy.deepcopy(my_predbat.car_charging_soc) - save_car_charging_battery_size = copy.deepcopy(my_predbat.car_charging_battery_size) - save_car_charging_slots = copy.deepcopy(my_predbat.car_charging_slots) - save_car_charging_now_slots = copy.deepcopy(my_predbat.car_charging_now_slots) - save_soc_kw = my_predbat.soc_kw - save_soc_max = my_predbat.soc_max - save_battery_rate_max_charge = my_predbat.battery_rate_max_charge - save_battery_rate_max_charge_dc = my_predbat.battery_rate_max_charge_dc - save_battery_rate_max_discharge = my_predbat.battery_rate_max_discharge - save_battery_rate_max_export = my_predbat.battery_rate_max_export - save_inverter_limit = my_predbat.inverter_limit + # Everything the comparison changes on the live instance, saved here and put back in the finally below + saved = {name: getattr(my_predbat, name) for name in COMPARE_RESTORED_STATE} + saved.update({name: copy.deepcopy(getattr(my_predbat, name)) for name in COMPARE_RESTORED_COPIES}) # Final reports, cut end_record back to 24 hours to ignore the dump at end of day end_record = int((my_predbat.minutes_now + 24 * 60 + 29) / 30) * 30 - my_predbat.minutes_now @@ -688,19 +708,18 @@ def run_all(self, debug=False, fetch_sensor=True): # First ever run for this tariff: start from actual midnight SoC start_soc = soc_midnight_fallback self.log("Compare tariff {} starting SoC: {:.2f} kWh".format(tariff.get("id", ""), start_soc)) + # Each tariff plans from scratch: scored against the previous tariff's plan, the keep-or-switch threshold + # in calculate_plan() could leave one tariff's result as another tariff's plan + my_predbat.plan_valid = False try: - result_data = self.run_single(tariff, rate_import_base, rate_export_base, end_record, debug=debug, fetch_sensor=fetch_sensor, car_charging_slots=save_car_charging_slots, start_soc=start_soc, io_adjusted_base=io_adjusted_base) + result_data = self.run_single(tariff, rate_import_base, rate_export_base, end_record, debug=debug, fetch_sensor=fetch_sensor, car_charging_slots=saved["car_charging_slots"], start_soc=start_soc, io_adjusted_base=io_adjusted_base) if result_data is not None: results[tariff["id"]] = result_data finally: # Restore hardware settings after each tariff so overrides don't bleed into the next tariff; # in a finally so a raised run_single() can't leave compare values applied to the live instance - my_predbat.soc_max = save_soc_max - my_predbat.battery_rate_max_charge = save_battery_rate_max_charge - my_predbat.battery_rate_max_charge_dc = save_battery_rate_max_charge_dc - my_predbat.battery_rate_max_discharge = save_battery_rate_max_discharge - my_predbat.battery_rate_max_export = save_battery_rate_max_export - my_predbat.inverter_limit = save_inverter_limit + for name in COMPARE_HARDWARE_STATE: + setattr(my_predbat, name, saved[name]) # Restore config: overrides after each tariff too, for the same reason - otherwise a # tariff's config override stays applied to config_index for every subsequent tariff, # even ones with no config block of their own (issue #4156) @@ -725,39 +744,6 @@ def run_all(self, debug=False, fetch_sensor=True): my_predbat.fetch_config_options() # Restore original settings - my_predbat.forecast_plan_hours = save_forecast_plan_hours - my_predbat.forecast_minutes = save_forecast_minutes - my_predbat.forecast_days = save_forecast_days - my_predbat.manual_charge_times = save_manual_charge_times - my_predbat.manual_export_times = save_manual_export_times - my_predbat.manual_freeze_charge_times = save_manual_freeze_charge_times - my_predbat.manual_freeze_export_times = save_manual_freeze_export_times - my_predbat.manual_demand_times = save_manual_demand_times - my_predbat.manual_all_times = save_manual_all_times - my_predbat.charge_window_best = save_charge_window_best - my_predbat.export_window_best = save_export_window_best - my_predbat.export_limits_best = save_export_limits_best - my_predbat.charge_limit_best = save_charge_limit_best - my_predbat.plan_preclip = save_plan_preclip - my_predbat.cost_today_sofar = save_cost_today_sofar - my_predbat.carbon_today_sofar = save_carbon_today_sofar - my_predbat.iboost_today = save_iboost_today - my_predbat.iboost_plan = save_iboost_plan - my_predbat.import_today_now = save_import_today_now - my_predbat.export_today_now = save_export_today_now - my_predbat.octopus_intelligent_charging = save_octopus_intelligent_charging - my_predbat.car_charging_limit = save_car_charging_limit - my_predbat.car_charging_limit_model = save_car_charging_limit_model - my_predbat.car_charging_soc = save_car_charging_soc - my_predbat.car_charging_battery_size = save_car_charging_battery_size - my_predbat.car_charging_slots = save_car_charging_slots - my_predbat.car_charging_now_slots = save_car_charging_now_slots - my_predbat.car_charging_plan_smart = save_car_charging_plan_smart - my_predbat.soc_kw = save_soc_kw - my_predbat.soc_max = save_soc_max - my_predbat.battery_rate_max_charge = save_battery_rate_max_charge - my_predbat.battery_rate_max_charge_dc = save_battery_rate_max_charge_dc - my_predbat.battery_rate_max_discharge = save_battery_rate_max_discharge - my_predbat.battery_rate_max_export = save_battery_rate_max_export - my_predbat.inverter_limit = save_inverter_limit + for name in COMPARE_RESTORED_STATE + COMPARE_RESTORED_COPIES: + setattr(my_predbat, name, saved[name]) my_predbat.io_adjusted = io_adjusted_base diff --git a/apps/predbat/tests/test_compare.py b/apps/predbat/tests/test_compare.py index 6f9375d6b..ae3d399e7 100644 --- a/apps/predbat/tests/test_compare.py +++ b/apps/predbat/tests/test_compare.py @@ -16,11 +16,14 @@ - hardware isolation between tariffs: overrides applied for tariff N must not bleed into tariff N+1 in run_all() (regression for the bug where agile_fixed reported a final SOC larger than the normal battery size) + - run_all() restores every attribute it saves, the plan state calculate_plan() keeps across runs + included (regression for plan_preclip left as the last tariff's plan), and plans each tariff from scratch + - every attribute calculate_plan() assigns is either restored by run_all() or rebuilt on every run """ import inspect -from compare import Compare +from compare import Compare, COMPARE_HARDWARE_STATE, COMPARE_PLAN_STATE, COMPARE_RESTORED_STATE, COMPARE_RESTORED_COPIES from const import MINUTE_WATT @@ -68,28 +71,45 @@ def _make_compare(): return cmp, pb -def test_run_all_restores_plan_preclip(my_predbat): - """run_all() puts back the pre-clip copy of the plan in force, which each tariff's calculate_plan() replaces. - - Left as the last tariff's 48-hour plan, the first re-plan after the midnight comparison scored that plan as the - previous one, so it always switched plans whatever metric_min_improvement_plan said (seen live on 9 Oct 2026: - previous plan 300.97p against 30.71p when the comparison is not run). +# Assigned by calculate_plan() but rebuilt on every run before use, so the comparison need not restore them +CALCULATE_PLAN_REBUILT = ( + "load_minutes_step", + "load_minutes_step10", + "load_minutes_step90", + "pv_forecast_minute_step", + "pv_forecast_minute10_step", + "pv_forecast_minute90_step", + "prediction", + "metric_cloud_coverage", + "metric_load_divergence", + "plan_last_duration_seconds", +) + + +def test_run_all_restores_state(my_predbat): + """run_all() puts back everything it saves however a tariff's run changed it, resets the hardware values between + tariffs, and plans each tariff from scratch. + + Each tariff fetches, plans and predicts on the live instance. Leaving plan_preclip as the last tariff's 48-hour + plan made the first re-plan after the midnight comparison score that plan as the plan in force, so it always + switched plans whatever metric_min_improvement_plan said (seen live on 9 Oct 2026: 300.97p against 30.71p). """ - from compare import Compare - + names = COMPARE_RESTORED_STATE + COMPARE_RESTORED_COPIES + original = {name: getattr(my_predbat, name) for name in names} saved_compare_list = my_predbat.args.get("compare_list") - saved_preclip = my_predbat.plan_preclip - plan_in_force = ([18.08], [{"start": 0, "end": 90, "average": 7.62}], [], []) - my_predbat.plan_preclip = plan_in_force - my_predbat.args["compare_list"] = [{"id": "test_tariff", "name": "Test tariff"}] + my_predbat.args["compare_list"] = [{"id": "tariff_a", "name": "Tariff A"}, {"id": "tariff_b", "name": "Tariff B"}] comparison = Compare.__new__(Compare) comparison.pb = my_predbat comparison.log = my_predbat.log comparison.comparisons = {} + seen_at_start = [] def run_single(*args, **kwargs): - """Stand in for a tariff's run: its calculate_plan() replaces plan_preclip with that tariff's plan.""" - my_predbat.plan_preclip = ([0.0], [{"start": 0, "end": 2880, "average": 30.0}], [], []) + """Stand in for a tariff's run, changing every saved attribute as fetching and planning a tariff can.""" + seen_at_start.append((my_predbat.plan_valid, all(getattr(my_predbat, name) == original[name] for name in COMPARE_HARDWARE_STATE))) + for name in names: + setattr(my_predbat, name, ("changed by", name)) + my_predbat.plan_valid = True return None comparison.run_single = run_single @@ -98,24 +118,50 @@ def run_single(*args, **kwargs): comparison.publish_data = lambda *args, **kwargs: None try: comparison.run_all(fetch_sensor=False) - if my_predbat.plan_preclip is not plan_in_force: - print("ERROR: run_all() should restore the plan in force's pre-clip copy, got {}".format(my_predbat.plan_preclip)) - return 1 - print("PASS: run_all() restores plan_preclip") - return 0 + failed = 0 + not_restored = [name for name in names if getattr(my_predbat, name) != original[name]] + if not_restored: + print("ERROR: run_all() did not restore {}".format(not_restored)) + failed = 1 + if seen_at_start != [(False, True), (False, True)]: + print("ERROR: each tariff should start with plan_valid False and the real hardware values, got {}".format(seen_at_start)) + failed = 1 + if not failed: + print("PASS: run_all() restores all {} saved attributes, resets hardware between tariffs and plans each from scratch".format(len(names))) + return failed finally: - my_predbat.plan_preclip = saved_preclip + for name, value in original.items(): + setattr(my_predbat, name, value) if saved_compare_list is None: my_predbat.args.pop("compare_list", None) else: my_predbat.args["compare_list"] = saved_compare_list +def test_plan_state_covered(my_predbat): + """Every attribute calculate_plan() assigns is restored by the comparison or rebuilt on every run. + + A new piece of plan state that outlives a run (as plan_preclip did) must be added to COMPARE_PLAN_STATE, or the + comparison will leave the last tariff's value behind. + """ + import re + + source = inspect.getsource(type(my_predbat).calculate_plan) + assigned = set(re.findall(r"self\.([a-z_0-9]+)\s*=(?!=)", source)) + uncovered = sorted(assigned - set(COMPARE_PLAN_STATE) - set(CALCULATE_PLAN_REBUILT)) + if uncovered: + 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)) + return 1 + print("PASS: every attribute calculate_plan() assigns is restored or rebuilt") + return 0 + + def test_compare(my_predbat): """Run all compare unit tests.""" failed = 0 print("**** Running compare tests ****\n") - failed += test_run_all_restores_plan_preclip(my_predbat) + failed += test_run_all_restores_state(my_predbat) + failed += test_plan_state_covered(my_predbat) # ------------------------------------------------------------------ # T1: apply_hardware_overrides – soc_max override @@ -588,23 +634,13 @@ def _mock_plan_iboost_smart(): print("PASS T18: bad export override value falls back to the discharge override") # ------------------------------------------------------------------ - # T19: run_all() actually saves and restores battery_rate_max_export, - # both mid-loop and at the end, so an export override can't bleed - # into the next tariff (T7 only proves the pattern in isolation). - # Positional checks (T12-style) so the snapshot must sit before the - # tariff loop, not just anywhere in the source. + # T19: run_all() saves and restores battery_rate_max_export, after each + # tariff and at the end, so an export override can't bleed into the + # next tariff (T7 only proves the pattern in isolation; the restore + # itself is exercised by test_run_all_restores_state) # ------------------------------------------------------------------ - run_all_source = inspect.getsource(Compare.run_all) - loop_idx = run_all_source.find("for tariff in compare_list:") - snapshot_idx = run_all_source.find("save_battery_rate_max_export = my_predbat.battery_rate_max_export") - if loop_idx < 0 or snapshot_idx < 0: - print("ERROR T19: could not locate the tariff loop or the battery_rate_max_export snapshot in run_all() source - test may need updating") - failed += 1 - elif not (snapshot_idx < loop_idx): - print("ERROR T19: battery_rate_max_export snapshot must appear before the tariff loop in run_all()") - failed += 1 - elif run_all_source.count("my_predbat.battery_rate_max_export = save_battery_rate_max_export") < 2: - print("ERROR T19: run_all() must restore battery_rate_max_export both inside the tariff loop and at the end") + if "battery_rate_max_export" not in COMPARE_HARDWARE_STATE or "battery_rate_max_export" not in COMPARE_RESTORED_STATE: + print("ERROR T19: battery_rate_max_export must be reset after each tariff (COMPARE_HARDWARE_STATE) and at the end") failed += 1 else: print("PASS T19: run_all() snapshots and restores battery_rate_max_export") @@ -708,12 +744,8 @@ def _mock_plan_iboost_smart(): # restores it after it, as it does car_charging_slots, so the live plan # gets its modelled slots back after a comparison (#5245 review) # ------------------------------------------------------------------ - snapshot_idx = run_all_source.find("save_car_charging_now_slots = copy.deepcopy(my_predbat.car_charging_now_slots)") - if snapshot_idx < 0 or not (snapshot_idx < loop_idx): - print("ERROR T25: car_charging_now_slots must be snapshotted before the tariff loop in run_all()") - failed += 1 - elif run_all_source.find("my_predbat.car_charging_now_slots = save_car_charging_now_slots") < loop_idx: - print("ERROR T25: run_all() must restore car_charging_now_slots after the tariff loop") + if "car_charging_now_slots" not in COMPARE_RESTORED_COPIES: + print("ERROR T25: run_all() must snapshot (deep copy) and restore car_charging_now_slots") failed += 1 else: print("PASS T25: run_all() snapshots and restores car_charging_now_slots") From 53fa6bd946cbc9dce4b655b0c8360f9efab8d1a3 Mon Sep 17 00:00:00 2001 From: Rik Allen Date: Fri, 9 Oct 2026 20:03:09 +0100 Subject: [PATCH 3/4] 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 --- apps/predbat/compare.py | 27 ++++--- apps/predbat/tests/test_compare.py | 116 ++++++++++++++++++++++++++--- 2 files changed, 122 insertions(+), 21 deletions(-) diff --git a/apps/predbat/compare.py b/apps/predbat/compare.py index 59d311c00..c7fe8845c 100644 --- a/apps/predbat/compare.py +++ b/apps/predbat/compare.py @@ -735,15 +735,18 @@ def run_all(self, debug=False, fetch_sensor=True): self.save_yaml() self.publish_data() finally: - # Restore config values overridden by any tariff's fetch_config() call - if config_snapshot: - for key, orig_value in config_snapshot.items(): - item = my_predbat.config_index.get(key) - if item is not None: - item["value"] = orig_value - my_predbat.fetch_config_options() - - # Restore original settings - for name in COMPARE_RESTORED_STATE + COMPARE_RESTORED_COPIES: - setattr(my_predbat, name, saved[name]) - my_predbat.io_adjusted = io_adjusted_base + try: + # Restore config values overridden by any tariff's fetch_config() call + if config_snapshot: + for key, orig_value in config_snapshot.items(): + item = my_predbat.config_index.get(key) + if item is not None: + item["value"] = orig_value + my_predbat.fetch_config_options() + finally: + # Restore original settings, last so they win over fetch_config_options(), and even if it raised + for name in COMPARE_RESTORED_STATE + COMPARE_RESTORED_COPIES: + setattr(my_predbat, name, saved[name]) + # A re-plan requested while the comparison ran (a setting or override changed) must still happen + my_predbat.plan_valid = saved["plan_valid"] and not my_predbat.update_pending + my_predbat.io_adjusted = io_adjusted_base diff --git a/apps/predbat/tests/test_compare.py b/apps/predbat/tests/test_compare.py index ae3d399e7..067fbb96d 100644 --- a/apps/predbat/tests/test_compare.py +++ b/apps/predbat/tests/test_compare.py @@ -18,7 +18,9 @@ reported a final SOC larger than the normal battery size) - run_all() restores every attribute it saves, the plan state calculate_plan() keeps across runs included (regression for plan_preclip left as the last tariff's plan), and plans each tariff from scratch - - every attribute calculate_plan() assigns is either restored by run_all() or rebuilt on every run + - every attribute calculate_plan() and its optimise passes assign is either restored by run_all() or rebuilt + on every run + - a re-plan requested during the comparison survives it, and the state is restored even if the config restore raises """ import inspect @@ -139,29 +141,125 @@ def run_single(*args, **kwargs): def test_plan_state_covered(my_predbat): - """Every attribute calculate_plan() assigns is restored by the comparison or rebuilt on every run. + """Every attribute calculate_plan() and the optimise passes it runs assign is restored by the comparison or + rebuilt on every run. A new piece of plan state that outlives a run (as plan_preclip did) must be added to COMPARE_PLAN_STATE, or the - comparison will leave the last tariff's value behind. + comparison will leave the last tariff's value behind. Read from the source with ast, so plain, augmented, + annotated and tuple assignments to self. are all found. """ - import re - - source = inspect.getsource(type(my_predbat).calculate_plan) - assigned = set(re.findall(r"self\.([a-z_0-9]+)\s*=(?!=)", source)) + import ast + import textwrap + + plan_class = type(my_predbat) + methods = ["calculate_plan"] + sorted(name for name in dir(plan_class) if name.startswith("optimise")) + assigned = set() + for name in methods: + tree = ast.parse(textwrap.dedent(inspect.getsource(getattr(plan_class, name)))) + for node in ast.walk(tree): + targets = node.targets if isinstance(node, ast.Assign) else [node.target] if isinstance(node, (ast.AugAssign, ast.AnnAssign)) else [] + for target in targets: + for part in ast.walk(target): + if isinstance(part, ast.Attribute) and isinstance(part.value, ast.Name) and part.value.id == "self": + assigned.add(part.attr) uncovered = sorted(assigned - set(COMPARE_PLAN_STATE) - set(CALCULATE_PLAN_REBUILT)) if uncovered: - 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)) + 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)) return 1 - print("PASS: every attribute calculate_plan() assigns is restored or rebuilt") + print("PASS: every attribute calculate_plan() and its {} optimise passes assign is restored or rebuilt".format(len(methods) - 1)) return 0 +def test_run_all_keeps_pending_replan(my_predbat): + """A re-plan requested while the comparison runs (a setting or override changed) still invalidates the plan.""" + saved_compare_list = my_predbat.args.get("compare_list") + saved_plan_valid, saved_pending = my_predbat.plan_valid, my_predbat.update_pending + my_predbat.args["compare_list"] = [{"id": "tariff_a", "name": "Tariff A"}] + my_predbat.plan_valid, my_predbat.update_pending = True, False + comparison = Compare.__new__(Compare) + comparison.pb = my_predbat + comparison.log = my_predbat.log + comparison.comparisons = {} + + def run_single(*args, **kwargs): + """Stand in for a tariff's run during which the user changes a setting.""" + my_predbat.update_pending = True + return None + + comparison.run_single = run_single + comparison.select_best = lambda *args, **kwargs: None + comparison.save_yaml = lambda *args, **kwargs: None + comparison.publish_data = lambda *args, **kwargs: None + try: + comparison.run_all(fetch_sensor=False) + if my_predbat.plan_valid: + print("ERROR: a re-plan requested during the comparison should leave the plan invalid") + return 1 + print("PASS: run_all() keeps a re-plan requested during the comparison") + return 0 + finally: + my_predbat.plan_valid, my_predbat.update_pending = saved_plan_valid, saved_pending + if saved_compare_list is None: + my_predbat.args.pop("compare_list", None) + else: + my_predbat.args["compare_list"] = saved_compare_list + + +def test_run_all_restores_when_config_restore_raises(my_predbat): + """The saved state is put back even when restoring a tariff's config raises.""" + item = my_predbat.config_index.get("metric_battery_cycle") + saved_compare_list = my_predbat.args.get("compare_list") + saved_preclip, saved_value = my_predbat.plan_preclip, item["value"] + plan_in_force = ([18.08], [{"start": 0, "end": 90, "average": 7.62}], [], []) + my_predbat.plan_preclip = plan_in_force + my_predbat.args["compare_list"] = [{"id": "tariff_a", "name": "Tariff A", "config": {"metric_battery_cycle": 9.0}}] + comparison = Compare.__new__(Compare) + comparison.pb = my_predbat + comparison.log = my_predbat.log + comparison.comparisons = {} + + def run_single(*args, **kwargs): + """Stand in for a tariff's run, which replaces the plan.""" + my_predbat.plan_preclip = ("tariff plan",) + return None + + def fetch_config_options(): + """Fail as a broken config refresh would.""" + raise ValueError("config refresh failed") + + comparison.run_single = run_single + comparison.select_best = lambda *args, **kwargs: None + comparison.save_yaml = lambda *args, **kwargs: None + comparison.publish_data = lambda *args, **kwargs: None + my_predbat.fetch_config_options = fetch_config_options + try: + try: + comparison.run_all(fetch_sensor=False) + except ValueError: + pass + if my_predbat.plan_preclip is not plan_in_force: + print("ERROR: run_all() should restore the plan even when the config restore raises, got {}".format(my_predbat.plan_preclip)) + return 1 + print("PASS: run_all() restores the saved state when the config restore raises") + return 0 + finally: + del my_predbat.fetch_config_options + my_predbat.plan_preclip = saved_preclip + item["value"] = saved_value + if saved_compare_list is None: + my_predbat.args.pop("compare_list", None) + else: + my_predbat.args["compare_list"] = saved_compare_list + + def test_compare(my_predbat): """Run all compare unit tests.""" failed = 0 print("**** Running compare tests ****\n") failed += test_run_all_restores_state(my_predbat) failed += test_plan_state_covered(my_predbat) + failed += test_run_all_keeps_pending_replan(my_predbat) + failed += test_run_all_restores_when_config_restore_raises(my_predbat) # ------------------------------------------------------------------ # T1: apply_hardware_overrides – soc_max override From 9fee9cd4781af3f5fa59da8cbd759a2cd9acfe56 Mon Sep 17 00:00:00 2001 From: Rik Allen Date: Sun, 11 Oct 2026 11:12:13 +0100 Subject: [PATCH 4/4] test(compare): pin plan_valid and update_pending in the run_all() tests run_all() now ends with plan_valid = saved and not update_pending. The shared fixture starts with update_pending True and several earlier tests leave plan_valid True, so test_run_all_restores_state failed or passed depending on run order. Set a valid plan with no re-plan pending before it, and save and restore both flags in it and in test_run_all_restores_when_config_restore_raises, which left plan_valid False for every later test. Co-Authored-By: Claude Opus 5.5 --- apps/predbat/tests/test_compare.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/apps/predbat/tests/test_compare.py b/apps/predbat/tests/test_compare.py index 067fbb96d..de3c235ed 100644 --- a/apps/predbat/tests/test_compare.py +++ b/apps/predbat/tests/test_compare.py @@ -97,6 +97,9 @@ def test_run_all_restores_state(my_predbat): switched plans whatever metric_min_improvement_plan said (seen live on 9 Oct 2026: 300.97p against 30.71p). """ names = COMPARE_RESTORED_STATE + COMPARE_RESTORED_COPIES + saved_plan_valid, saved_pending = my_predbat.plan_valid, my_predbat.update_pending + # A valid plan with no re-plan pending, so plan_valid must come back True whatever earlier tests left behind + my_predbat.plan_valid, my_predbat.update_pending = True, False original = {name: getattr(my_predbat, name) for name in names} saved_compare_list = my_predbat.args.get("compare_list") my_predbat.args["compare_list"] = [{"id": "tariff_a", "name": "Tariff A"}, {"id": "tariff_b", "name": "Tariff B"}] @@ -134,6 +137,7 @@ def run_single(*args, **kwargs): finally: for name, value in original.items(): setattr(my_predbat, name, value) + my_predbat.plan_valid, my_predbat.update_pending = saved_plan_valid, saved_pending if saved_compare_list is None: my_predbat.args.pop("compare_list", None) else: @@ -210,6 +214,7 @@ def test_run_all_restores_when_config_restore_raises(my_predbat): item = my_predbat.config_index.get("metric_battery_cycle") saved_compare_list = my_predbat.args.get("compare_list") saved_preclip, saved_value = my_predbat.plan_preclip, item["value"] + saved_plan_valid, saved_pending = my_predbat.plan_valid, my_predbat.update_pending plan_in_force = ([18.08], [{"start": 0, "end": 90, "average": 7.62}], [], []) my_predbat.plan_preclip = plan_in_force my_predbat.args["compare_list"] = [{"id": "tariff_a", "name": "Tariff A", "config": {"metric_battery_cycle": 9.0}}] @@ -245,6 +250,7 @@ def fetch_config_options(): finally: del my_predbat.fetch_config_options my_predbat.plan_preclip = saved_preclip + my_predbat.plan_valid, my_predbat.update_pending = saved_plan_valid, saved_pending item["value"] = saved_value if saved_compare_list is None: my_predbat.args.pop("compare_list", None)