Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds and reformats many JSON fixtures (new large Adam/Jvonk datasets and widespread array reflows), introduces defensive guards in plugwise/helper.py for missing thermostat data, adds an async Adam connection test, and updates CHANGELOG.md. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #860 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 3443 3456 +13
=========================================
+ Hits 3443 3456 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugwise/helper.py (1)
867-872: Avoid warning on every refresh for the same broken zone.
_get_location_data()calls_regulation_control()on each update, so a permanently missing thermostat will emit this warning every poll cycle. Consider logging once per zone or downgrading this to debug to avoid log spam.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise/helper.py` around lines 867 - 872, The current LOGGER.warning in _get_location_data() / _regulation_control() will spam logs every poll for zones missing a thermostat; change this to warn only once per zone by tracking seen broken zones (e.g., add a set attribute like self._warned_missing_thermostat and use data["name"] as the key), log the warning only if the zone is not in that set, add the zone to the set after warning, and remove the zone from the set when a thermostat is present again; alternatively, if you prefer no persistent state, change LOGGER.warning to LOGGER.debug in the thermostat None branch to stop repeated warnings.tests/data/adam/adam_bad_thermostat.json (1)
1-532: Avoid maintaining a third copy of this payload.This file appears identical to
fixtures/adam_bad_thermostat/data.jsonandfixtures/adam_jvonk/data.jsonin the same PR. Keeping three large copies synchronized is easy to miss, and the regression can drift away from the source fixture that reproduces the bug. Consider loading a single canonical JSON fixture in the test helper, or generating this test payload from that source instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data/adam/adam_bad_thermostat.json` around lines 1 - 532, This JSON payload is a duplicate of the fixture used in fixtures/adam_bad_thermostat/data.json and fixtures/adam_jvonk/data.json; remove this redundant file and update tests to load the single canonical fixture instead (or generate it from that source) via the test helper so all tests reference one source of truth (search for uses of this file name in tests and replace them to load the canonical fixture, or add a loader function in the test helper to return the shared fixture).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_adam.py`:
- Around line 19-25: The test test_connect_adam_jvonk is misconfigured: it sets
self.smile_setup = "adam_bad_thermostat" so load_testdata() loads the
bad-thermostat fixture instead of the intended JVonk dataset; fix by either
renaming the test to reflect the bad-thermostat fixture or change
self.smile_setup to the correct JVonk fixture name (the JVonk JSON identifier
used by load_testdata), ensuring test_connect_adam_jvonk actually loads the
JVonk data before calling connect_wrapper.
---
Nitpick comments:
In `@plugwise/helper.py`:
- Around line 867-872: The current LOGGER.warning in _get_location_data() /
_regulation_control() will spam logs every poll for zones missing a thermostat;
change this to warn only once per zone by tracking seen broken zones (e.g., add
a set attribute like self._warned_missing_thermostat and use data["name"] as the
key), log the warning only if the zone is not in that set, add the zone to the
set after warning, and remove the zone from the set when a thermostat is present
again; alternatively, if you prefer no persistent state, change LOGGER.warning
to LOGGER.debug in the thermostat None branch to stop repeated warnings.
In `@tests/data/adam/adam_bad_thermostat.json`:
- Around line 1-532: This JSON payload is a duplicate of the fixture used in
fixtures/adam_bad_thermostat/data.json and fixtures/adam_jvonk/data.json; remove
this redundant file and update tests to load the single canonical fixture
instead (or generate it from that source) via the test helper so all tests
reference one source of truth (search for uses of this file name in tests and
replace them to load the canonical fixture, or add a loader function in the test
helper to return the shared fixture).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0039a41-b4d5-48f0-96ef-ab945a9cbe70
📒 Files selected for processing (38)
fixtures/adam_bad_thermostat/data.jsonfixtures/adam_heatpump_cooling/data.jsonfixtures/adam_jip/data.jsonfixtures/adam_jvonk/data.jsonfixtures/adam_multiple_devices_per_zone/data.jsonfixtures/adam_onoff_cooling_fake_firmware/data.jsonfixtures/adam_plus_anna/data.jsonfixtures/adam_plus_anna_new/data.jsonfixtures/adam_plus_anna_new_regulation_off/data.jsonfixtures/adam_zone_per_device/data.jsonfixtures/anna_elga_2/data.jsonfixtures/anna_elga_2_cooling/data.jsonfixtures/anna_elga_2_schedule_off/data.jsonfixtures/anna_elga_no_cooling/data.jsonfixtures/anna_heatpump_cooling/data.jsonfixtures/anna_heatpump_cooling_fake_firmware/data.jsonfixtures/anna_heatpump_heating/data.jsonfixtures/anna_loria_cooling_active/data.jsonfixtures/anna_loria_driessens/data.jsonfixtures/anna_loria_heating_idle/data.jsonfixtures/anna_p1/data.jsonfixtures/anna_v4/data.jsonfixtures/anna_v4_dhw/data.jsonfixtures/anna_v4_no_tag/data.jsonfixtures/anna_without_boiler_fw441/data.jsonfixtures/legacy_anna/data.jsonfixtures/legacy_anna_2/data.jsonfixtures/m_adam_cooling/data.jsonfixtures/m_adam_heating/data.jsonfixtures/m_adam_jip/data.jsonfixtures/m_adam_multiple_devices_per_zone/data.jsonfixtures/m_anna_heatpump_cooling/data.jsonfixtures/m_anna_heatpump_idle/data.jsonfixtures/stretch_v23/data.jsonplugwise/helper.pytests/data/adam/adam_bad_thermostat.jsontests/test_adam.pyuserdata/adam_bad_thermostat/core.domain_objects.xml
| @pytest.mark.asyncio | ||
| async def test_connect_adam_jvonk(self): | ||
| """Test Adam with missing thermostat data.""" | ||
| self.smile_setup = "adam_bad_thermostat" | ||
|
|
||
| testdata = await self.load_testdata(SMILE_TYPE, self.smile_setup) | ||
| server, api, client = await self.connect_wrapper(raise_timeout=True) |
There was a problem hiding this comment.
This test never exercises the JVonk fixture.
Line 22 sets self.smile_setup to adam_bad_thermostat, and load_testdata() resolves the JSON path from that value. So test_connect_adam_jvonk() is currently validating the bad-thermostat fixture instead of a JVonk dataset. Either rename the test to match what it covers or point self.smile_setup at the intended JVonk fixture.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_adam.py` around lines 19 - 25, The test test_connect_adam_jvonk is
misconfigured: it sets self.smile_setup = "adam_bad_thermostat" so
load_testdata() loads the bad-thermostat fixture instead of the intended JVonk
dataset; fix by either renaming the test to reflect the bad-thermostat fixture
or change self.smile_setup to the correct JVonk fixture name (the JVonk JSON
identifier used by load_testdata), ensuring test_connect_adam_jvonk actually
loads the JVonk data before calling connect_wrapper.
CoMPaTech
left a comment
CoMPaTech
left a comment
There was a problem hiding this comment.
Remark before merging, CRAI is about right to call the naming of the test - for others directories are tagged with @githubname file in the directories?
|
|
@CoMPaTech Yes I missed that one, implemented via fb63431 |
2 participants
Partly a solution for plugwise/plugwise-beta#1030
Summary by CodeRabbit
Bug Fixes
Tests
Documentation