From d21aa7ab0f3ec074c0cb9acde69eb7610803fdc1 Mon Sep 17 00:00:00 2001 From: Stefanie Jane Date: Wed, 14 Aug 2024 09:08:18 -0700 Subject: [PATCH] =?UTF-8?q?=E2=9A=A1=EF=B8=8F=20Implement=20delayed=20refr?= =?UTF-8?q?esh=20for=20SignalRGB=20light?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add _schedule_delayed_refresh and _delayed_refresh methods to SignalRGBLight class. These methods ensure effect application is verified after a short delay, with retry logic for failed applications. Update tests to accommodate new functionality. --- custom_components/signalrgb/light.py | 86 ++++++++++++++++++++++++---- tests/conftest.py | 2 + tests/test_light.py | 50 ++++++++++++++-- 3 files changed, 123 insertions(+), 15 deletions(-) diff --git a/custom_components/signalrgb/light.py b/custom_components/signalrgb/light.py index 1b1e02d..0247cec 100644 --- a/custom_components/signalrgb/light.py +++ b/custom_components/signalrgb/light.py @@ -4,6 +4,7 @@ from __future__ import annotations +import asyncio from datetime import timedelta from typing import Any @@ -112,7 +113,10 @@ def __init__( self._current_effect: Effect | None = None self._is_on: bool = False self._brightness: int = 0 # This is now 0-100 - self._pending_effect: str | None = None + self._requested_effect: str | None = None + self._retry_count: int = 0 + self._max_retries: int = 3 + self._refresh_task: asyncio.Task | None = None LOGGER.debug("SignalRGBLight initialized: %s", self.entity_id) async def async_added_to_hass(self) -> None: @@ -179,6 +183,7 @@ def extra_state_attributes(self) -> dict[str, Any]: async def async_turn_on(self, **kwargs: Any) -> None: """Instruct the light to turn on.""" LOGGER.debug("Turning on %s with kwargs: %s", self.entity_id, kwargs) + if not self.is_on: LOGGER.debug("Light was off, turning on") await self.hass.async_add_executor_job( @@ -201,11 +206,12 @@ async def async_turn_on(self, **kwargs: Any) -> None: if ATTR_EFFECT in kwargs: effect = kwargs[ATTR_EFFECT] - LOGGER.debug("Applying effect: %s", effect) + LOGGER.debug("Requesting effect: %s", effect) + self._requested_effect = effect + self._retry_count = 0 await self._apply_effect(effect) - # Schedule a delayed refresh - await self.coordinator.async_request_refresh() + self._schedule_delayed_refresh() async def async_turn_off(self, **kwargs: Any) -> None: """Instruct the light to turn off.""" @@ -230,15 +236,50 @@ async def _apply_effect(self, effect: str) -> None: # Update state immediately self._current_effect = effect_obj - self._pending_effect = effect self.async_write_ha_state() LOGGER.debug("Effect applied and state updated immediately: %s", effect) - # await self.coordinator.async_request_refresh() except SignalRGBException as err: LOGGER.error("Failed to apply effect %s: %s", effect, err) raise HomeAssistantError(f"Failed to apply effect: {err}") from err + def _schedule_delayed_refresh(self) -> None: + """Schedule a delayed refresh to verify the effect was applied correctly.""" + if self._refresh_task: + self._refresh_task.cancel() + + self._refresh_task = asyncio.create_task(self._delayed_refresh()) + + async def _delayed_refresh(self) -> None: + """Perform a delayed refresh and retry if necessary.""" + await asyncio.sleep(2) # Wait for 2 seconds before refreshing + await self.coordinator.async_request_refresh() + + if self._requested_effect and self.effect != self._requested_effect: + LOGGER.warning( + "Applied effect doesn't match requested effect. " + "Requested: %s, Applied: %s", + self._requested_effect, + self.effect, + ) + if self._retry_count < self._max_retries: + self._retry_count += 1 + LOGGER.debug( + "Retrying effect application (Attempt %s of %s)", + self._retry_count, + self._max_retries, + ) + await self._apply_effect(self._requested_effect) + self._schedule_delayed_refresh() + else: + LOGGER.error( + "Failed to apply effect %s after %s attempts", + self._requested_effect, + self._max_retries, + ) + self._requested_effect = None + self._retry_count = 0 + async def async_update_effect_list(self) -> None: """Update the list of available effects.""" LOGGER.debug("Updating effect list for %s", self.entity_id) @@ -266,16 +307,25 @@ def _handle_coordinator_update(self) -> None: ): self._current_effect = new_effect if ( - self._pending_effect - and new_effect.attributes.name != self._pending_effect + self._requested_effect + and new_effect.attributes.name != self._requested_effect ): LOGGER.warning( "Applied effect doesn't match requested effect. " "Requested: %s, Applied: %s", - self._pending_effect, + self._requested_effect, new_effect.attributes.name, ) - self._pending_effect = None + elif ( + self._requested_effect + and new_effect.attributes.name == self._requested_effect + ): + LOGGER.info( + "Requested effect %s successfully applied", + self._requested_effect, + ) + self._requested_effect = None + self._retry_count = 0 LOGGER.debug( "Updated state - Effect: %s, Is On: %s, Brightness: %s", @@ -289,3 +339,19 @@ def _handle_coordinator_update(self) -> None: LOGGER.warning("No data received from coordinator for %s", self.entity_id) self.async_write_ha_state() LOGGER.debug("State updated after coordinator update") + + async def async_will_remove_from_hass(self) -> None: + """Clean up resources when entity is removed.""" + if self._refresh_task and not self._refresh_task.done(): + self._refresh_task.cancel() + try: + # Wait for the task to be cancelled, but don't wait indefinitely + await asyncio.wait([self._refresh_task], timeout=1) + except asyncio.TimeoutError: + pass # The task didn't finish cancelling in time, but that's okay + await super().async_will_remove_from_hass() + + # For testing delayed refresh + def _cancel_refresh_task(self): + if self._refresh_task: + self._refresh_task.cancel() diff --git a/tests/conftest.py b/tests/conftest.py index b224385..6d0bf39 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,7 @@ """Pytest configuration file for the tests directory.""" +# pylint: disable=unused-argument + import os import sys from pathlib import Path diff --git a/tests/test_light.py b/tests/test_light.py index b8dc56b..4e0b608 100644 --- a/tests/test_light.py +++ b/tests/test_light.py @@ -2,6 +2,7 @@ # pylint: disable=protected-access, redefined-outer-name +import asyncio from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -64,6 +65,7 @@ def mock_coordinator(): """Mock DataUpdateCoordinator.""" coordinator = MagicMock(spec=DataUpdateCoordinator) coordinator.data = None + coordinator.async_request_refresh = AsyncMock() return coordinator @@ -103,17 +105,27 @@ async def test_brightness(self, mock_light): async def test_turn_on(self, mock_light, mock_coordinator): """Test turning on the light.""" - await mock_light.async_turn_on() + with patch("asyncio.sleep", new_callable=AsyncMock): + await mock_light.async_turn_on() + mock_light.hass.async_add_executor_job.assert_any_call( setattr, mock_light._client, "enabled", True ) assert mock_light._is_on is True assert mock_light.async_write_ha_state.call_count == 1 # Once for turning on + + # Run the delayed refresh + await mock_light._delayed_refresh() mock_coordinator.async_request_refresh.assert_called_once() + # Clean up + mock_light._cancel_refresh_task() + async def test_turn_on_with_brightness(self, mock_light, mock_coordinator): """Test turning on the light with brightness.""" - await mock_light.async_turn_on(**{ATTR_BRIGHTNESS: 128}) + with patch("asyncio.sleep", new_callable=AsyncMock): + await mock_light.async_turn_on(**{ATTR_BRIGHTNESS: 128}) + mock_light.hass.async_add_executor_job.assert_any_call( setattr, mock_light._client, "enabled", True ) @@ -125,8 +137,14 @@ async def test_turn_on_with_brightness(self, mock_light, mock_coordinator): assert ( mock_light.async_write_ha_state.call_count == 2 ) # Once for on, once for brightness + + # Run the delayed refresh + await mock_light._delayed_refresh() mock_coordinator.async_request_refresh.assert_called_once() + # Clean up + mock_light._cancel_refresh_task() + async def test_turn_on_with_effect(self, mock_light, mock_coordinator): """Test turning on the light with an effect.""" mock_effect = "Rainbow Wave" @@ -140,18 +158,25 @@ async def test_turn_on_with_effect(self, mock_light, mock_coordinator): None, # For apply_effect ] - await mock_light.async_turn_on(**{ATTR_EFFECT: mock_effect}) + with patch("asyncio.sleep", new_callable=AsyncMock): + await mock_light.async_turn_on(**{ATTR_EFFECT: mock_effect}) + assert mock_light._is_on is True assert mock_light._current_effect == mock_effect_obj assert ( mock_light.async_write_ha_state.call_count == 2 ) # Once for on, once for effect + + # Run the delayed refresh + await mock_light._delayed_refresh() mock_coordinator.async_request_refresh.assert_called_once() + # Clean up + mock_light._cancel_refresh_task() + async def test_turn_off(self, mock_light, mock_coordinator): """Test turning off the light.""" - with patch("asyncio.sleep", new_callable=AsyncMock): - await mock_light.async_turn_off() + await mock_light.async_turn_off() mock_light.hass.async_add_executor_job.assert_called_with( setattr, mock_light._client, "enabled", False ) @@ -241,3 +266,18 @@ async def test_extra_state_attributes(self, mock_light): # Test when light is off mock_light._is_on = False assert mock_light.extra_state_attributes == {} + + async def test_async_will_remove_from_hass(self, mock_light): + """Test the async_will_remove_from_hass method.""" + + # Create a real asyncio.Task for _refresh_task + async def mock_refresh(): + await asyncio.sleep(10) # Simulate a long-running task + + mock_light._refresh_task = asyncio.create_task(mock_refresh()) + + # Call the method we're testing + await mock_light.async_will_remove_from_hass() + + # Assert that the task was cancelled + assert mock_light._refresh_task.cancelled()