tests: characterize core inverter dispatch behavior#319
Conversation
|
Looks good! Thank your for filling up that gap. I am fine with introducing pytest-mock. On the long run, it might be good to switch all test over to pytest-mock. |
There was a problem hiding this comment.
Pull request overview
Adds characterization tests to pin down how Batcontrol.run() maps logic outputs onto inverter mode method calls, helping prevent unintentional dispatch regressions in the control loop.
Changes:
- Add a new
TestCoreRunDispatchtest class covering dispatch to allow discharge, avoid discharge, force charge, and limit battery charge rate. - Introduce a shared fixture that patches forecast/tariff/inverter factories and the logic factory to make
Batcontrol.run()deterministic in tests.
tests/batcontrol/test_core.py
Outdated
| "timezone": "Europe/Berlin", | ||
| "time_resolution_minutes": 60, | ||
| "inverter": { | ||
| "type": "dummy", | ||
| "max_grid_charge_rate": 5000, | ||
| "max_pv_charge_rate": 3000, | ||
| "min_pv_charge_rate": 0, | ||
| }, | ||
| 'utility': { | ||
| 'type': 'tibber', | ||
| 'token': 'test_token' | ||
| }, | ||
| "pvinstallations": [], | ||
| 'consumption_forecast': { | ||
| 'type': 'simple', | ||
| 'value': 500 | ||
| }, | ||
| "battery_control": { | ||
| "max_charging_from_grid_limit": 0.8, | ||
| "min_price_difference": 0.05 | ||
| }, | ||
| "mqtt": {"enabled": False} |
There was a problem hiding this comment.
In this test file, the other mock configs consistently use single quotes for keys/strings; this new mock_config mixes double- and single-quoted strings, which makes the file harder to scan and maintain. Consider switching this fixture to the same quoting style used elsewhere in tests/batcontrol/test_core.py for consistency.
| "timezone": "Europe/Berlin", | |
| "time_resolution_minutes": 60, | |
| "inverter": { | |
| "type": "dummy", | |
| "max_grid_charge_rate": 5000, | |
| "max_pv_charge_rate": 3000, | |
| "min_pv_charge_rate": 0, | |
| }, | |
| 'utility': { | |
| 'type': 'tibber', | |
| 'token': 'test_token' | |
| }, | |
| "pvinstallations": [], | |
| 'consumption_forecast': { | |
| 'type': 'simple', | |
| 'value': 500 | |
| }, | |
| "battery_control": { | |
| "max_charging_from_grid_limit": 0.8, | |
| "min_price_difference": 0.05 | |
| }, | |
| "mqtt": {"enabled": False} | |
| 'timezone': 'Europe/Berlin', | |
| 'time_resolution_minutes': 60, | |
| 'inverter': { | |
| 'type': 'dummy', | |
| 'max_grid_charge_rate': 5000, | |
| 'max_pv_charge_rate': 3000, | |
| 'min_pv_charge_rate': 0, | |
| }, | |
| 'utility': { | |
| 'type': 'tibber', | |
| 'token': 'test_token' | |
| }, | |
| 'pvinstallations': [], | |
| 'consumption_forecast': { | |
| 'type': 'simple', | |
| 'value': 500 | |
| }, | |
| 'battery_control': { | |
| 'max_charging_from_grid_limit': 0.8, | |
| 'min_price_difference': 0.05 | |
| }, | |
| 'mqtt': {'enabled': False} |
tests/batcontrol/test_core.py
Outdated
| bc = Batcontrol(mock_config) | ||
| yield bc, mock_inverter, fake_logic |
There was a problem hiding this comment.
The fixture instantiates Batcontrol, which starts a background SchedulerThread and schedules global schedule jobs in init. Because the fixture doesn't tear this down after yield, these threads/jobs can accumulate across tests and make the suite more order-dependent. Consider adding a teardown after the yield (e.g., stop the scheduler/clear scheduled jobs, or call bc.shutdown() with inverter.shutdown mocked) to keep tests isolated.
bc11d57 to
e746936
Compare
|
@filiplajszczak Copilot brought up 2 review points, I did not see before.
|
49e1e4a to
aa0ff86
Compare
|
@MaStr Done and ready. |
|
Testing workflow installs dependencies explicitly so it was missing |
|
Yesterday, I saw this and this morning I went through the other parts used for testing 'run_tests.sh' . The workflow and the shell script is installing dependencies manually. If you like, you can enhance these files together with the project.toml. The "optional dependency" are "testing", which should consolidate all additional dependencies. In run_tests.sh and the workflow the install of batcontrol should add the testing stuff via project.toml as one source of truth. |
aa0ff86 to
1e3b74c
Compare
Done, @MaStr take a look. |
|
Thank you a lot for doing the additional round on the different dependency locations! |
Batcontrol.run()dispatches logic output to inverter mode methods, but that behavior is not fully pinned down in tests.This adds tests covering dispatch to allow discharge, avoid discharge, force charge, and limit battery charge rate. Tests only, no behavior change.
I’m keeping this as a draft for now, mostly to check whether this kind of small characterization-test PR is useful before I send follow-ups.
@MaStr, would you be open to
pytest-mock/mockerfor future pytest-style tests, or would you rather keepunittest.mockdirectly? I’m asking because themockerfixture can make repeated patch setup a bit easier. Happy to follow your preference either way.