Conversation
Codecov Report❌ Patch coverage is
... and 14 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where A01 device values (Dyad and Zeo products) were not being converted after a previous refactor. The fix adds value conversion logic to transform raw protocol responses into properly typed and formatted values.
Key changes:
- Implemented protocol-to-value conversion mappings for Dyad and Zeo devices
- Updated
query_valuesmethods to apply conversions to raw responses - Added comprehensive test coverage for the conversion logic
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 29 comments.
| File | Description |
|---|---|
roborock/devices/traits/a01/__init__.py |
Added conversion dictionaries (DYAD_PROTOCOL_ENTRIES, ZEO_PROTOCOL_ENTRIES) and converter functions (convert_dyad_value, convert_zeo_value) to transform raw protocol values; updated DyadApi.query_values and ZeoApi.query_values to apply conversions to responses |
tests/devices/test_a01_traits.py |
Added new test file with tests for DyadApi.query_values and ZeoApi.query_values to verify that raw protocol values are correctly converted to their appropriate types (enums to strings, integers, booleans, etc.) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @pytest.fixture | ||
| def mock_channel(): | ||
| channel = Mock() | ||
| channel.send_command = AsyncMock() | ||
| # Mocking send_decoded_command if it was a method on channel, but it's a standalone function imported in traits. | ||
| # However, in traits/__init__.py it is imported as: from roborock.devices.a01_channel import send_decoded_command | ||
| # Implementation detail: we need to mock send_decoded_command where it is used. | ||
| return channel |
There was a problem hiding this comment.
The mock_channel fixture is defined but never used. It should be removed or utilized in the tests.
There was a problem hiding this comment.
i'm a bit confused by this one as mock_channel is used
tests/devices/test_a01_traits.py
Outdated
| @pytest.fixture | ||
| def mock_send_decoded_command(): | ||
| with patch("roborock.devices.traits.a01.send_decoded_command", new_callable=AsyncMock) as mock: | ||
| yield mock |
There was a problem hiding this comment.
The mock_send_decoded_command fixture is defined but never used in the tests (both tests create their own patches inline). This fixture should be removed or the tests should be refactored to use it.
| @pytest.fixture | |
| def mock_send_decoded_command(): | |
| with patch("roborock.devices.traits.a01.send_decoded_command", new_callable=AsyncMock) as mock: | |
| yield mock |
| async def test_zeo_query_values(mock_channel): | ||
| with patch("roborock.devices.traits.a01.send_decoded_command", new_callable=AsyncMock) as mock_send: | ||
| api = ZeoApi(mock_channel) | ||
|
|
||
| mock_send.return_value = { | ||
| int(RoborockZeoProtocol.STATE): 6, # spinning | ||
| int(RoborockZeoProtocol.COUNTDOWN): 120, | ||
| } | ||
|
|
||
| protocols = [RoborockZeoProtocol.STATE, RoborockZeoProtocol.COUNTDOWN] | ||
| result = await api.query_values(protocols) | ||
|
|
||
| assert RoborockZeoProtocol.STATE in result | ||
| # From a01_conversions.py: RoborockZeoProtocol.STATE: lambda val: ZeoState(val).name | ||
| assert result[RoborockZeoProtocol.STATE] == "spinning" # Assuming ZeoState(6).name is spinning | ||
| assert result[RoborockZeoProtocol.COUNTDOWN] == 120 |
There was a problem hiding this comment.
The test does not verify that send_decoded_command was called with the correct parameters. Consider adding an assertion like mock_send.assert_called_once_with(mock_channel, {RoborockZeoProtocol.ID_QUERY: [int(p) for p in protocols]}) to ensure the query is constructed correctly.
| } | ||
|
|
||
| ZEO_PROTOCOL_ENTRIES: dict[RoborockZeoProtocol, Callable] = { | ||
| # ro |
There was a problem hiding this comment.
The comment # ro appears incomplete or unclear. It should be removed or expanded to explain what it means (e.g., "read-only" if that's the intent).
| # ro | |
| # read-only |
| RoborockZeoProtocol.COUNTDOWN: lambda val: int(val), | ||
| RoborockZeoProtocol.WASHING_LEFT: lambda val: int(val), | ||
| RoborockZeoProtocol.ERROR: lambda val: ZeoError(val).name, | ||
| RoborockZeoProtocol.TIMES_AFTER_CLEAN: lambda val: int(val), | ||
| RoborockZeoProtocol.DETERGENT_EMPTY: lambda val: bool(val), | ||
| RoborockZeoProtocol.SOFTENER_EMPTY: lambda val: bool(val), |
There was a problem hiding this comment.
This 'lambda' is just a simple wrapper around a callable object. Use that object directly.
| RoborockZeoProtocol.COUNTDOWN: lambda val: int(val), | |
| RoborockZeoProtocol.WASHING_LEFT: lambda val: int(val), | |
| RoborockZeoProtocol.ERROR: lambda val: ZeoError(val).name, | |
| RoborockZeoProtocol.TIMES_AFTER_CLEAN: lambda val: int(val), | |
| RoborockZeoProtocol.DETERGENT_EMPTY: lambda val: bool(val), | |
| RoborockZeoProtocol.SOFTENER_EMPTY: lambda val: bool(val), | |
| RoborockZeoProtocol.COUNTDOWN: int, | |
| RoborockZeoProtocol.WASHING_LEFT: int, | |
| RoborockZeoProtocol.ERROR: lambda val: ZeoError(val).name, | |
| RoborockZeoProtocol.TIMES_AFTER_CLEAN: int, | |
| RoborockZeoProtocol.DETERGENT_EMPTY: bool, | |
| RoborockZeoProtocol.SOFTENER_EMPTY: bool, |
| RoborockZeoProtocol.COUNTDOWN: lambda val: int(val), | ||
| RoborockZeoProtocol.WASHING_LEFT: lambda val: int(val), | ||
| RoborockZeoProtocol.ERROR: lambda val: ZeoError(val).name, | ||
| RoborockZeoProtocol.TIMES_AFTER_CLEAN: lambda val: int(val), |
There was a problem hiding this comment.
This 'lambda' is just a simple wrapper around a callable object. Use that object directly.
| RoborockZeoProtocol.WASHING_LEFT: lambda val: int(val), | ||
| RoborockZeoProtocol.ERROR: lambda val: ZeoError(val).name, | ||
| RoborockZeoProtocol.TIMES_AFTER_CLEAN: lambda val: int(val), | ||
| RoborockZeoProtocol.DETERGENT_EMPTY: lambda val: bool(val), |
There was a problem hiding this comment.
This 'lambda' is just a simple wrapper around a callable object. Use that object directly.
| RoborockZeoProtocol.ERROR: lambda val: ZeoError(val).name, | ||
| RoborockZeoProtocol.TIMES_AFTER_CLEAN: lambda val: int(val), | ||
| RoborockZeoProtocol.DETERGENT_EMPTY: lambda val: bool(val), | ||
| RoborockZeoProtocol.SOFTENER_EMPTY: lambda val: bool(val), |
There was a problem hiding this comment.
This 'lambda' is just a simple wrapper around a callable object. Use that object directly.
| RoborockZeoProtocol.SOFTENER_EMPTY: lambda val: bool(val), | |
| RoborockZeoProtocol.SOFTENER_EMPTY: bool, |
|
Issue #623 |
allenporter
force-pushed
the
a01_conversions
branch
from
67aefa0 to
97d8b7c
Compare
| if (converter := DYAD_PROTOCOL_ENTRIES.get(protocol_value)) is not None: | ||
| try: | ||
| return converter(value) | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
I added exception catching here
| params = {RoborockDyadDataProtocol.ID_QUERY: str([int(p) for p in protocols])} | ||
| return await send_decoded_command(self._channel, params) | ||
| response = await send_decoded_command(self._channel, params) | ||
| return {protocol: convert_dyad_value(protocol, response.get(protocol)) for protocol in protocols} |
There was a problem hiding this comment.
Changed this to use the requested protocols to decide which values to return.
|
Feel free to merge once you've reviewed my changes if you think they're ready. |
2 participants
A01 values were not converted after the refactor.
This combined with the changes in #645 have A01 working tested with a shared account
Fixes: #623