Fix swallowed exceptions in deconz actions - #175646
Conversation
|
Hey there @Kane610, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #170606 by fixing swallowed exceptions in the deCONZ integration's action handlers. Previously, several except blocks in services.py only logged errors and returned, so failed actions gave the user no UI feedback and automations continued as if the action succeeded. The change re-raises HomeAssistantError with translated messages instead, and removes the associated pylint: disable-next=home-assistant-action-swallowed-exception suppressions.
Changes:
- Replaced log-and-return error handling in
async_call_deconz_service,async_configure_service, andasync_refresh_devices_servicewith raisedHomeAssistantErrors using new translation keys. - Added an
exceptionsblock tostrings.jsonwith five translation keys (gateway_not_found,no_master_gateway,entity_not_found,configure_failed,device_refresh_failed). - Updated existing service tests to assert that
HomeAssistantErroris now raised (usingblocking=Trueandpytest.raises).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| homeassistant/components/deconz/services.py | Re-raises HomeAssistantError instead of logging-and-returning; refactors bridge lookup and wraps API calls in try/except; drops unused LOGGER import. |
| homeassistant/components/deconz/strings.json | Adds exceptions translation keys referenced by the new error handling. |
| tests/components/deconz/test_services.py | Updates failure-path tests to expect HomeAssistantError with blocking=True. |
Notes for the author: the two except Exception catches are broader than the integration's established convention (hub/api.py:35, config_flow.py:161 catch (TimeoutError, errors.RequestError, errors.ResponseError)) and will mask unexpected errors; moving load_ignored_devices() into finally changes behavior on refresh failures; and the new configure_failed/device_refresh_failed branches are not yet covered by tests (consistent with the PR description noting it is untested).
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@Kane610 Copilot is satisfied for now, and the tests are running successfully, so we can move on to the next step. What do you think? |
| @@ -122,9 +125,15 @@ async def test_configure_service_with_faulty_bridgeid( | |||
| SERVICE_DATA: {"on": True}, | |||
| } | |||
|
|
|||
| await hass.services.async_call(DOMAIN, SERVICE_CONFIGURE_DEVICE, service_data=data) | |||
| await hass.async_block_till_done() | |||
| with pytest.raises(HomeAssistantError) as err: | |||
| await hass.services.async_call( | |||
| DOMAIN, | |||
| SERVICE_CONFIGURE_DEVICE, | |||
| service_data=data, | |||
| blocking=True, | |||
| ) | |||
|
|
|||
| assert err.value.translation_key == "gateway_not_found" | |||
| assert len(aioclient_mock.mock_calls) == 0 | |||
|
|
|||
|
|
|||
| @@ -141,26 +150,35 @@ async def test_configure_service_with_faulty_field(hass: HomeAssistant) -> None: | |||
|
|
|||
| @pytest.mark.usefixtures("config_entry_setup") | |||
| async def test_configure_service_with_faulty_entity( | |||
| hass: HomeAssistant, aioclient_mock: AiohttpClientMocker | |||
| hass: HomeAssistant, | |||
| aioclient_mock: AiohttpClientMocker, | |||
| ) -> None: | |||
| """Test that service on a non existing entity.""" | |||
| """Test that service fails on a non-existing entity.""" | |||
| aioclient_mock.clear_requests() | |||
|
|
|||
| data = { | |||
| SERVICE_ENTITY: "light.nonexisting", | |||
| SERVICE_DATA: {}, | |||
| } | |||
|
|
|||
| await hass.services.async_call(DOMAIN, SERVICE_CONFIGURE_DEVICE, service_data=data) | |||
| await hass.async_block_till_done() | |||
| with pytest.raises(HomeAssistantError) as err: | |||
| await hass.services.async_call( | |||
| DOMAIN, | |||
| SERVICE_CONFIGURE_DEVICE, | |||
| service_data=data, | |||
| blocking=True, | |||
| ) | |||
|
|
|||
| assert err.value.translation_key == "entity_not_found" | |||
| assert err.value.translation_placeholders == {"entity_id": "light.nonexisting"} | |||
| assert len(aioclient_mock.mock_calls) == 0 | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("config_entry_options", [{CONF_MASTER_GATEWAY: False}]) | |||
| @pytest.mark.usefixtures("config_entry_setup") | |||
| async def test_calling_service_with_no_master_gateway_fails( | |||
| hass: HomeAssistant, aioclient_mock: AiohttpClientMocker | |||
| hass: HomeAssistant, | |||
| aioclient_mock: AiohttpClientMocker, | |||
| ) -> None: | |||
| """Test that service call fails when no master gateway exist.""" | |||
| aioclient_mock.clear_requests() | |||
| @@ -170,9 +188,15 @@ async def test_calling_service_with_no_master_gateway_fails( | |||
| SERVICE_DATA: {"on": True}, | |||
| } | |||
|
|
|||
| await hass.services.async_call(DOMAIN, SERVICE_CONFIGURE_DEVICE, service_data=data) | |||
| await hass.async_block_till_done() | |||
| with pytest.raises(HomeAssistantError) as err: | |||
| await hass.services.async_call( | |||
| DOMAIN, | |||
| SERVICE_CONFIGURE_DEVICE, | |||
| service_data=data, | |||
| blocking=True, | |||
| ) | |||
|
|
|||
| assert err.value.translation_key == "no_master_gateway" | |||
| assert len(aioclient_mock.mock_calls) == 0 | |||
There was a problem hiding this comment.
These could be merged/parameterized
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
@justanotherariel Thank you very much for your help, and I hope it's okay now. |
justanotherariel
left a comment
There was a problem hiding this comment.
You did a bit more than I asked for regarding the error types - not everything should be a ServiceValidationError. Fixed it for you. Looks good now.
Breaking change
Proposed change
This PR is the starting point for fixing issue #170606, as I haven't tested it yet.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: