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

@@ -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}"

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,
                 )
             )
 

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"]}
         )
 

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,
             )
         )
 

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:

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

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 = (
+        dev = d if d else await _discover_update_and_close(ip, username, password)
-            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
 

kasa/tests/test_feature.py

@@ -1,7 +1,12 @@
-import pytest
+import logging
-from pytest_mock import MockFixture
+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
+        )