From 5ef81f46693868b2ef367d6886490e2307e6a462 Mon Sep 17 00:00:00 2001 From: Teemu R Date: Thu, 2 May 2024 15:32:06 +0200 Subject: [PATCH] Improve feature setter robustness (#870) This adds a test to check that all feature.set_value() calls will cause a query, i.e., that there are no self.call()s that are not awaited, and fixes existing code in this context. This also fixes an issue where it was not possible to print out the feature if the value threw an exception. --- kasa/feature.py | 7 ++- kasa/iot/iotbulb.py | 1 + kasa/smart/modules/autooffmodule.py | 8 +-- kasa/smart/modules/colortemp.py | 1 + kasa/smart/modules/lighttransitionmodule.py | 4 +- kasa/smart/modules/temperature.py | 1 + kasa/tests/device_fixtures.py | 18 +++--- kasa/tests/test_feature.py | 70 +++++++++++++++++++-- 8 files changed, 91 insertions(+), 19 deletions(-) diff --git a/kasa/feature.py b/kasa/feature.py index 30acf362..b6e933ce 100644 --- a/kasa/feature.py +++ b/kasa/feature.py @@ -172,9 +172,14 @@ class Feature: return await getattr(container, self.attribute_setter)(value) def __repr__(self): - value = self.value + try: + value = self.value + except Exception as ex: + return f"Unable to read value ({self.id}): {ex}" + if self.precision_hint is not None and value is not None: value = round(self.value, self.precision_hint) + s = f"{self.name} ({self.id}): {value}" if self.unit is not None: s += f" {self.unit}" diff --git a/kasa/iot/iotbulb.py b/kasa/iot/iotbulb.py index 50c31f62..d9456e96 100644 --- a/kasa/iot/iotbulb.py +++ b/kasa/iot/iotbulb.py @@ -233,6 +233,7 @@ class IotBulb(IotDevice, Bulb): attribute_setter="set_color_temp", range_getter="valid_temperature_range", category=Feature.Category.Primary, + type=Feature.Type.Number, ) ) diff --git a/kasa/smart/modules/autooffmodule.py b/kasa/smart/modules/autooffmodule.py index 019d4235..8977719c 100644 --- a/kasa/smart/modules/autooffmodule.py +++ b/kasa/smart/modules/autooffmodule.py @@ -55,9 +55,9 @@ class AutoOffModule(SmartModule): """Return True if enabled.""" return self.data["enable"] - def set_enabled(self, enable: bool): + async def set_enabled(self, enable: bool): """Enable/disable auto off.""" - return self.call( + return await self.call( "set_auto_off_config", {"enable": enable, "delay_min": self.data["delay_min"]}, ) @@ -67,9 +67,9 @@ class AutoOffModule(SmartModule): """Return time until auto off.""" return self.data["delay_min"] - def set_delay(self, delay: int): + async def set_delay(self, delay: int): """Set time until auto off.""" - return self.call( + return await self.call( "set_auto_off_config", {"delay_min": delay, "enable": self.data["enable"]} ) diff --git a/kasa/smart/modules/colortemp.py b/kasa/smart/modules/colortemp.py index e0bfec6a..1392775c 100644 --- a/kasa/smart/modules/colortemp.py +++ b/kasa/smart/modules/colortemp.py @@ -34,6 +34,7 @@ class ColorTemperatureModule(SmartModule): attribute_setter="set_color_temp", range_getter="valid_temperature_range", category=Feature.Category.Primary, + type=Feature.Type.Number, ) ) diff --git a/kasa/smart/modules/lighttransitionmodule.py b/kasa/smart/modules/lighttransitionmodule.py index e7da22ef..1cb7f48a 100644 --- a/kasa/smart/modules/lighttransitionmodule.py +++ b/kasa/smart/modules/lighttransitionmodule.py @@ -88,9 +88,9 @@ class LightTransitionModule(SmartModule): return self.data["off_state"] - def set_enabled_v1(self, enable: bool): + async def set_enabled_v1(self, enable: bool): """Enable gradual on/off.""" - return self.call("set_on_off_gradually_info", {"enable": enable}) + return await self.call("set_on_off_gradually_info", {"enable": enable}) @property def enabled_v1(self) -> bool: diff --git a/kasa/smart/modules/temperature.py b/kasa/smart/modules/temperature.py index ea9b18e5..49ffe046 100644 --- a/kasa/smart/modules/temperature.py +++ b/kasa/smart/modules/temperature.py @@ -48,6 +48,7 @@ class TemperatureSensor(SmartModule): attribute_getter="temperature_unit", attribute_setter="set_temperature_unit", type=Feature.Type.Choice, + choices=["celsius", "fahrenheit"], ) ) # TODO: use temperature_unit for feature creation diff --git a/kasa/tests/device_fixtures.py b/kasa/tests/device_fixtures.py index 50dfbce7..83449a53 100644 --- a/kasa/tests/device_fixtures.py +++ b/kasa/tests/device_fixtures.py @@ -1,5 +1,7 @@ from __future__ import annotations +from typing import AsyncGenerator + import pytest from kasa import ( @@ -346,13 +348,13 @@ def device_for_fixture_name(model, protocol): raise Exception("Unable to find type for %s", model) -async def _update_and_close(d): +async def _update_and_close(d) -> Device: await d.update() await d.protocol.close() return d -async def _discover_update_and_close(ip, username, password): +async def _discover_update_and_close(ip, username, password) -> Device: if username and password: credentials = Credentials(username=username, password=password) else: @@ -361,7 +363,7 @@ async def _discover_update_and_close(ip, username, password): return await _update_and_close(d) -async def get_device_for_fixture(fixture_data: FixtureInfo): +async def get_device_for_fixture(fixture_data: FixtureInfo) -> Device: # if the wanted file is not an absolute path, prepend the fixtures directory d = device_for_fixture_name(fixture_data.name, fixture_data.protocol)( @@ -395,13 +397,14 @@ async def get_device_for_fixture_protocol(fixture, protocol): @pytest.fixture(params=filter_fixtures("main devices"), ids=idgenerator) -async def dev(request): +async def dev(request) -> AsyncGenerator[Device, None]: """Device fixture. Provides a device (given --ip) or parametrized fixture for the supported devices. The initial update is called automatically before returning the device. """ fixture_data: FixtureInfo = request.param + dev: Device ip = request.config.getoption("--ip") username = request.config.getoption("--username") @@ -412,13 +415,12 @@ async def dev(request): if not model: d = await _discover_update_and_close(ip, username, password) IP_MODEL_CACHE[ip] = model = d.model + if model not in fixture_data.name: pytest.skip(f"skipping file {fixture_data.name}") - dev: Device = ( - d if d else await _discover_update_and_close(ip, username, password) - ) + dev = d if d else await _discover_update_and_close(ip, username, password) else: - dev: Device = await get_device_for_fixture(fixture_data) + dev = await get_device_for_fixture(fixture_data) yield dev diff --git a/kasa/tests/test_feature.py b/kasa/tests/test_feature.py index f5de47d1..fe6ba7f2 100644 --- a/kasa/tests/test_feature.py +++ b/kasa/tests/test_feature.py @@ -1,7 +1,12 @@ -import pytest -from pytest_mock import MockFixture +import logging +import sys -from kasa import Feature +import pytest +from pytest_mock import MockerFixture + +from kasa import Device, Feature, KasaException + +_LOGGER = logging.getLogger(__name__) class DummyDevice: @@ -111,7 +116,7 @@ async def test_feature_action(mocker): mock_call_action.assert_called() -async def test_feature_choice_list(dummy_feature, caplog, mocker: MockFixture): +async def test_feature_choice_list(dummy_feature, caplog, mocker: MockerFixture): """Test the choice feature type.""" dummy_feature.type = Feature.Type.Choice dummy_feature.choices = ["first", "second"] @@ -138,3 +143,60 @@ async def test_precision_hint(dummy_feature, precision_hint): dummy_feature.attribute_getter = lambda x: dummy_value assert dummy_feature.value == dummy_value assert f"{round(dummy_value, precision_hint)} dummyunit" in repr(dummy_feature) + + +@pytest.mark.skipif( + sys.version_info < (3, 11), + reason="exceptiongroup requires python3.11+", +) +async def test_feature_setters(dev: Device, mocker: MockerFixture): + """Test that all feature setters query something.""" + + async def _test_feature(feat, query_mock): + if feat.attribute_setter is None: + return + + expecting_call = True + + if feat.type == Feature.Type.Number: + await feat.set_value(feat.minimum_value) + elif feat.type == Feature.Type.Switch: + await feat.set_value(True) + elif feat.type == Feature.Type.Action: + await feat.set_value("dummyvalue") + elif feat.type == Feature.Type.Choice: + await feat.set_value(feat.choices[0]) + elif feat.type == Feature.Type.Unknown: + _LOGGER.warning("Feature '%s' has no type, cannot test the setter", feat) + expecting_call = False + else: + raise NotImplementedError(f"set_value not implemented for {feat.type}") + + if expecting_call: + query_mock.assert_called() + + async def _test_features(dev): + exceptions = [] + query = mocker.patch.object(dev.protocol, "query") + for feat in dev.features.values(): + query.reset_mock() + try: + await _test_feature(feat, query) + # we allow our own exceptions to avoid mocking valid responses + except KasaException: + pass + except Exception as ex: + ex.add_note(f"Exception when trying to set {feat} on {dev}") + exceptions.append(ex) + + return exceptions + + exceptions = await _test_features(dev) + + for child in dev.children: + exceptions.extend(await _test_features(child)) + + if exceptions: + raise ExceptionGroup( + "Got exceptions while testing attribute_setters", exceptions + )