Fix solcast and inverter crashes #3205
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the Solcast solar forecasting code that causes a KeyError crash when day 0 (today) has missing or stale forecast data. The bug was caused by incorrect initialization logic that prevented daily totals from being properly initialized when forecast data was sparse.
Key changes:
- Fixed initialization loop to properly initialize all 7 days of forecast data
- Corrected indentation of
daystracking variable - Added regression test for missing day 0 scenario
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/predbat/solcast.py | Fixed initialization bug where days was set to 0 immediately after declaration, preventing the initialization loop from running; now properly initializes all 7 days upfront |
| apps/predbat/tests/test_solcast.py | Added regression test test_publish_pv_stats_missing_day_zero to verify the fix handles missing day 0 forecast data; also added debug logging to existing test |
| apps/predbat/predbat.py | Bumped version from v8.31.13 to v8.31.14 |
| """Mock log - silent""" | ||
| pass | ||
| """Mock log - print for debugging""" | ||
| if "DEBUG:" in str(message): |
There was a problem hiding this comment.
The change to the mock log method could cause it to print unintended debug messages. The condition checks if "DEBUG:" is in the message, but this will print any log message containing the substring "DEBUG:", not just the intentional debug prints added in this PR. This could cause noise in test output if the actual code logs messages with "DEBUG:" in them.
Consider either:
- Using a more specific prefix like "TEST_DEBUG:" that is unlikely to appear in actual code
- Adding a flag to control debug output instead of string matching
- Reverting this change and only adding the explicit print statements where needed
| if "DEBUG:" in str(message): | |
| if str(message).startswith("DEBUG:"): |
apps/predbat/tests/test_solcast.py
Outdated
| print(f"DEBUG: Before publish_pv_stats, midnight_utc={test_api.solar.midnight_utc}") | ||
| test_api.solar.publish_pv_stats(pv_forecast_data, divide_by=1.0, period=30) | ||
| print(f"DEBUG: After publish_pv_stats, dashboard_items keys={list(test_api.dashboard_items.keys())}") |
There was a problem hiding this comment.
These debug print statements appear to be temporary debugging code that was left in. They don't add value to the test and will clutter test output. Consider removing them or wrapping them in a conditional flag for debug mode only.
| """Mock log - print for debugging""" | ||
| if "DEBUG:" in str(message): | ||
| print(message) | ||
|
|
There was a problem hiding this comment.
The debug logging added to the mock log method will cause all messages containing "DEBUG:" to be printed during test runs. This debug logging should be removed before merging as it's not needed in production and will clutter test output. If debugging is needed during development, it should be controlled by a test flag or removed after debugging is complete.
| """Mock log - print for debugging""" | |
| if "DEBUG:" in str(message): | |
| print(message) | |
| """Mock log - no-op to avoid noisy debug output in tests""" | |
| return |
| if start_time is None or end_time is None: | ||
| # Invalid time, return 0,0 | ||
| return 0, 0 |
There was a problem hiding this comment.
Consider adding a test case to verify that compute_window_minutes and window2minutes correctly handle None inputs, as this defensive check was added to prevent crashes. This would ensure the fix continues to work and prevent regression.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
#3199