From 8d0e9eb8ac1dcdf9e86e936fc727176888f8249d Mon Sep 17 00:00:00 2001 From: Allen Porter Date: Tue, 24 Sep 2024 13:38:40 -0700 Subject: [PATCH] Improve Roborock error handling (#124267) --- homeassistant/components/roborock/number.py | 15 ++++++-- .../components/roborock/strings.json | 3 ++ homeassistant/components/roborock/switch.py | 28 ++++++++++---- homeassistant/components/roborock/time.py | 15 ++++++-- tests/components/roborock/test_button.py | 38 ++++++++++++++++++- tests/components/roborock/test_number.py | 35 +++++++++++++++++ tests/components/roborock/test_select.py | 2 +- tests/components/roborock/test_switch.py | 36 ++++++++++++++++++ tests/components/roborock/test_time.py | 34 +++++++++++++++++ 9 files changed, 189 insertions(+), 17 deletions(-) diff --git a/homeassistant/components/roborock/number.py b/homeassistant/components/roborock/number.py index 9f0d578cae4..7f568ae824b 100644 --- a/homeassistant/components/roborock/number.py +++ b/homeassistant/components/roborock/number.py @@ -13,9 +13,10 @@ from roborock.version_1_apis.roborock_client_v1 import AttributeCache from homeassistant.components.number import NumberEntity, NumberEntityDescription from homeassistant.const import PERCENTAGE, EntityCategory from homeassistant.core import HomeAssistant +from homeassistant.exceptions import HomeAssistantError from homeassistant.helpers.entity_platform import AddEntitiesCallback -from . import RoborockConfigEntry +from . import DOMAIN, RoborockConfigEntry from .coordinator import RoborockDataUpdateCoordinator from .entity import RoborockEntityV1 @@ -107,6 +108,12 @@ class RoborockNumberEntity(RoborockEntityV1, NumberEntity): async def async_set_native_value(self, value: float) -> None: """Set number value.""" - await self.entity_description.update_value( - self.get_cache(self.entity_description.cache_key), value - ) + try: + await self.entity_description.update_value( + self.get_cache(self.entity_description.cache_key), value + ) + except RoborockException as err: + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="update_options_failed", + ) from err diff --git a/homeassistant/components/roborock/strings.json b/homeassistant/components/roborock/strings.json index d1fc50f27e8..8ff82cae393 100644 --- a/homeassistant/components/roborock/strings.json +++ b/homeassistant/components/roborock/strings.json @@ -419,6 +419,9 @@ }, "no_coordinators": { "message": "No devices were able to successfully setup" + }, + "update_options_failed": { + "message": "Failed to update Roborock options" } }, "services": { diff --git a/homeassistant/components/roborock/switch.py b/homeassistant/components/roborock/switch.py index 407ec51103c..b0c8c880188 100644 --- a/homeassistant/components/roborock/switch.py +++ b/homeassistant/components/roborock/switch.py @@ -9,14 +9,16 @@ import logging from typing import Any from roborock.command_cache import CacheableAttribute +from roborock.exceptions import RoborockException from roborock.version_1_apis.roborock_client_v1 import AttributeCache from homeassistant.components.switch import SwitchEntity, SwitchEntityDescription from homeassistant.const import EntityCategory from homeassistant.core import HomeAssistant +from homeassistant.exceptions import HomeAssistantError from homeassistant.helpers.entity_platform import AddEntitiesCallback -from . import RoborockConfigEntry +from . import DOMAIN, RoborockConfigEntry from .coordinator import RoborockDataUpdateCoordinator from .entity import RoborockEntityV1 @@ -149,15 +151,27 @@ class RoborockSwitch(RoborockEntityV1, SwitchEntity): async def async_turn_off(self, **kwargs: Any) -> None: """Turn off the switch.""" - await self.entity_description.update_value( - self.get_cache(self.entity_description.cache_key), False - ) + try: + await self.entity_description.update_value( + self.get_cache(self.entity_description.cache_key), False + ) + except RoborockException as err: + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="update_options_failed", + ) from err async def async_turn_on(self, **kwargs: Any) -> None: """Turn on the switch.""" - await self.entity_description.update_value( - self.get_cache(self.entity_description.cache_key), True - ) + try: + await self.entity_description.update_value( + self.get_cache(self.entity_description.cache_key), True + ) + except RoborockException as err: + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="update_options_failed", + ) from err @property def is_on(self) -> bool | None: diff --git a/homeassistant/components/roborock/time.py b/homeassistant/components/roborock/time.py index a705eb69ea1..1dd681dff1f 100644 --- a/homeassistant/components/roborock/time.py +++ b/homeassistant/components/roborock/time.py @@ -15,9 +15,10 @@ from roborock.version_1_apis.roborock_client_v1 import AttributeCache from homeassistant.components.time import TimeEntity, TimeEntityDescription from homeassistant.const import EntityCategory from homeassistant.core import HomeAssistant +from homeassistant.exceptions import HomeAssistantError from homeassistant.helpers.entity_platform import AddEntitiesCallback -from . import RoborockConfigEntry +from . import DOMAIN, RoborockConfigEntry from .coordinator import RoborockDataUpdateCoordinator from .entity import RoborockEntityV1 @@ -172,6 +173,12 @@ class RoborockTimeEntity(RoborockEntityV1, TimeEntity): async def async_set_value(self, value: time) -> None: """Set the time.""" - await self.entity_description.update_value( - self.get_cache(self.entity_description.cache_key), value - ) + try: + await self.entity_description.update_value( + self.get_cache(self.entity_description.cache_key), value + ) + except RoborockException as err: + raise HomeAssistantError( + translation_domain=DOMAIN, + translation_key="update_options_failed", + ) from err diff --git a/tests/components/roborock/test_button.py b/tests/components/roborock/test_button.py index 88cf5beab15..43ef043f79c 100644 --- a/tests/components/roborock/test_button.py +++ b/tests/components/roborock/test_button.py @@ -3,9 +3,11 @@ from unittest.mock import patch import pytest +import roborock from homeassistant.components.button import SERVICE_PRESS from homeassistant.core import HomeAssistant +from homeassistant.exceptions import HomeAssistantError from tests.common import MockConfigEntry @@ -16,7 +18,7 @@ from tests.common import MockConfigEntry ("button.roborock_s7_maxv_reset_sensor_consumable"), ("button.roborock_s7_maxv_reset_air_filter_consumable"), ("button.roborock_s7_maxv_reset_side_brush_consumable"), - "button.roborock_s7_maxv_reset_main_brush_consumable", + ("button.roborock_s7_maxv_reset_main_brush_consumable"), ], ) @pytest.mark.freeze_time("2023-10-30 08:50:00") @@ -41,3 +43,37 @@ async def test_update_success( ) assert mock_send_message.assert_called_once assert hass.states.get(entity_id).state == "2023-10-30T08:50:00+00:00" + + +@pytest.mark.parametrize( + ("entity_id"), + [ + ("button.roborock_s7_maxv_reset_air_filter_consumable"), + ], +) +@pytest.mark.freeze_time("2023-10-30 08:50:00") +@pytest.mark.usefixtures("entity_registry_enabled_by_default") +async def test_update_failure( + hass: HomeAssistant, + bypass_api_fixture, + setup_entry: MockConfigEntry, + entity_id: str, +) -> None: + """Test failure while pressing the button entity.""" + # Ensure that the entity exist, as these test can pass even if there is no entity. + assert hass.states.get(entity_id).state == "unknown" + with ( + patch( + "homeassistant.components.roborock.coordinator.RoborockLocalClientV1.send_message", + side_effect=roborock.exceptions.RoborockTimeout, + ) as mock_send_message, + pytest.raises(HomeAssistantError, match="Error while calling RESET_CONSUMABLE"), + ): + await hass.services.async_call( + "button", + SERVICE_PRESS, + blocking=True, + target={"entity_id": entity_id}, + ) + assert mock_send_message.assert_called_once + assert hass.states.get(entity_id).state == "2023-10-30T08:50:00+00:00" diff --git a/tests/components/roborock/test_number.py b/tests/components/roborock/test_number.py index 3291dd2a7dc..7e87b49253e 100644 --- a/tests/components/roborock/test_number.py +++ b/tests/components/roborock/test_number.py @@ -3,9 +3,11 @@ from unittest.mock import patch import pytest +import roborock from homeassistant.components.number import ATTR_VALUE, SERVICE_SET_VALUE from homeassistant.core import HomeAssistant +from homeassistant.exceptions import HomeAssistantError from tests.common import MockConfigEntry @@ -37,3 +39,36 @@ async def test_update_success( target={"entity_id": entity_id}, ) assert mock_send_message.assert_called_once + + +@pytest.mark.parametrize( + ("entity_id", "value"), + [ + ("number.roborock_s7_maxv_volume", 3.0), + ], +) +async def test_update_failed( + hass: HomeAssistant, + bypass_api_fixture, + setup_entry: MockConfigEntry, + entity_id: str, + value: float, +) -> None: + """Test allowed changing values for number entities.""" + # Ensure that the entity exist, as these test can pass even if there is no entity. + assert hass.states.get(entity_id) is not None + with ( + patch( + "homeassistant.components.roborock.coordinator.RoborockLocalClientV1.send_message", + side_effect=roborock.exceptions.RoborockTimeout, + ) as mock_send_message, + pytest.raises(HomeAssistantError, match="Failed to update Roborock options"), + ): + await hass.services.async_call( + "number", + SERVICE_SET_VALUE, + service_data={ATTR_VALUE: value}, + blocking=True, + target={"entity_id": entity_id}, + ) + assert mock_send_message.assert_called_once diff --git a/tests/components/roborock/test_select.py b/tests/components/roborock/test_select.py index ce846107d93..784150e24c7 100644 --- a/tests/components/roborock/test_select.py +++ b/tests/components/roborock/test_select.py @@ -59,7 +59,7 @@ async def test_update_failure( "homeassistant.components.roborock.coordinator.RoborockLocalClientV1.send_message", side_effect=RoborockException(), ), - pytest.raises(HomeAssistantError), + pytest.raises(HomeAssistantError, match="Error while calling SET_MOP_MOD"), ): await hass.services.async_call( "select", diff --git a/tests/components/roborock/test_switch.py b/tests/components/roborock/test_switch.py index 3afa72b319d..5de3c208c1e 100644 --- a/tests/components/roborock/test_switch.py +++ b/tests/components/roborock/test_switch.py @@ -3,9 +3,11 @@ from unittest.mock import patch import pytest +import roborock from homeassistant.components.switch import SERVICE_TURN_OFF, SERVICE_TURN_ON from homeassistant.core import HomeAssistant +from homeassistant.exceptions import HomeAssistantError from tests.common import MockConfigEntry @@ -49,3 +51,37 @@ async def test_update_success( target={"entity_id": entity_id}, ) assert mock_send_message.assert_called_once + + +@pytest.mark.parametrize( + ("entity_id", "service"), + [ + ("switch.roborock_s7_maxv_status_indicator_light", SERVICE_TURN_ON), + ("switch.roborock_s7_maxv_status_indicator_light", SERVICE_TURN_OFF), + ], +) +async def test_update_failed( + hass: HomeAssistant, + bypass_api_fixture, + setup_entry: MockConfigEntry, + entity_id: str, + service: str, +) -> None: + """Test a failure while updating a switch.""" + # Ensure that the entity exist, as these test can pass even if there is no entity. + assert hass.states.get(entity_id) is not None + with ( + patch( + "homeassistant.components.roborock.coordinator.RoborockLocalClientV1._send_command", + side_effect=roborock.exceptions.RoborockTimeout, + ) as mock_send_message, + pytest.raises(HomeAssistantError, match="Failed to update Roborock options"), + ): + await hass.services.async_call( + "switch", + service, + service_data=None, + blocking=True, + target={"entity_id": entity_id}, + ) + assert mock_send_message.assert_called_once diff --git a/tests/components/roborock/test_time.py b/tests/components/roborock/test_time.py index ca6507f887b..836a86bd114 100644 --- a/tests/components/roborock/test_time.py +++ b/tests/components/roborock/test_time.py @@ -4,9 +4,11 @@ from datetime import time from unittest.mock import patch import pytest +import roborock from homeassistant.components.time import SERVICE_SET_VALUE from homeassistant.core import HomeAssistant +from homeassistant.exceptions import HomeAssistantError from tests.common import MockConfigEntry @@ -38,3 +40,35 @@ async def test_update_success( target={"entity_id": entity_id}, ) assert mock_send_message.assert_called_once + + +@pytest.mark.parametrize( + ("entity_id"), + [ + ("time.roborock_s7_maxv_do_not_disturb_begin"), + ], +) +async def test_update_failure( + hass: HomeAssistant, + bypass_api_fixture, + setup_entry: MockConfigEntry, + entity_id: str, +) -> None: + """Test turning switch entities on and off.""" + # Ensure that the entity exist, as these test can pass even if there is no entity. + assert hass.states.get(entity_id) is not None + with ( + patch( + "homeassistant.components.roborock.coordinator.RoborockLocalClientV1._send_command", + side_effect=roborock.exceptions.RoborockTimeout, + ) as mock_send_message, + pytest.raises(HomeAssistantError, match="Failed to update Roborock options"), + ): + await hass.services.async_call( + "time", + SERVICE_SET_VALUE, + service_data={"time": time(hour=1, minute=1)}, + blocking=True, + target={"entity_id": entity_id}, + ) + assert mock_send_message.assert_called_once