From 9ba504cd7841640625960da4a2ec06ce8f4d3988 Mon Sep 17 00:00:00 2001 From: epenet <6771947+epenet@users.noreply.github.com> Date: Fri, 27 Aug 2021 07:22:23 +0200 Subject: [PATCH] Address late review for Renault integration (#55230) * Add type hints * Fix isort * tweaks to state attributes * Move lambdas to regular functions * Split CHECK_ATTRIBUTES into DYNAMIC_ATTRIBUTES and FIXED_ATTRIBUTES * Clarify tests * Fix typo --- .../components/renault/renault_entities.py | 11 +-- homeassistant/components/renault/sensor.py | 81 +++++++++++-------- tests/components/renault/__init__.py | 10 ++- tests/components/renault/const.py | 14 +++- .../components/renault/test_binary_sensor.py | 34 ++++---- tests/components/renault/test_init.py | 7 +- tests/components/renault/test_sensor.py | 60 +++++--------- 7 files changed, 112 insertions(+), 105 deletions(-) diff --git a/homeassistant/components/renault/renault_entities.py b/homeassistant/components/renault/renault_entities.py index 2a23d1de8f6..4e364c9d5d1 100644 --- a/homeassistant/components/renault/renault_entities.py +++ b/homeassistant/components/renault/renault_entities.py @@ -63,11 +63,8 @@ class RenaultDataEntity(CoordinatorEntity[Optional[T]], Entity): @property def extra_state_attributes(self) -> Mapping[str, Any] | None: """Return the state attributes of this entity.""" - if self.entity_description.coordinator == "battery": - last_update = ( - getattr(self.coordinator.data, "timestamp") - if self.coordinator.data - else None - ) - return {ATTR_LAST_UPDATE: last_update} + if self.entity_description.coordinator == "battery" and self.coordinator.data: + timestamp = getattr(self.coordinator.data, "timestamp") + if timestamp: + return {ATTR_LAST_UPDATE: timestamp} return None diff --git a/homeassistant/components/renault/sensor.py b/homeassistant/components/renault/sensor.py index 56101cd378b..982e16f3b13 100644 --- a/homeassistant/components/renault/sensor.py +++ b/homeassistant/components/renault/sensor.py @@ -102,20 +102,53 @@ class RenaultSensor(RenaultDataEntity[T], SensorEntity): return self.entity_description.value_lambda(self) -def _get_formatted_charging_status( - data: KamereonVehicleBatteryStatusData, -) -> str | None: +def _get_charge_mode_icon(entity: RenaultDataEntity[T]) -> str: + """Return the icon of this entity.""" + if entity.data == "schedule_mode": + return "mdi:calendar-clock" + return "mdi:calendar-remove" + + +def _get_charging_power(entity: RenaultDataEntity[T]) -> StateType: + """Return the charging_power of this entity.""" + if entity.vehicle.details.reports_charging_power_in_watts(): + return cast(float, entity.data) / 1000 + return entity.data + + +def _get_charge_state_formatted(entity: RenaultDataEntity[T]) -> str | None: """Return the charging_status of this entity.""" + data = cast(KamereonVehicleBatteryStatusData, entity.coordinator.data) charging_status = data.get_charging_status() if data else None return charging_status.name.lower() if charging_status else None -def _get_formatted_plug_status(data: KamereonVehicleBatteryStatusData) -> str | None: +def _get_charge_state_icon(entity: RenaultDataEntity[T]) -> str: + """Return the icon of this entity.""" + if entity.data == ChargeState.CHARGE_IN_PROGRESS.value: + return "mdi:flash" + return "mdi:flash-off" + + +def _get_plug_state_formatted(entity: RenaultDataEntity[T]) -> str | None: """Return the plug_status of this entity.""" + data = cast(KamereonVehicleBatteryStatusData, entity.coordinator.data) plug_status = data.get_plug_status() if data else None return plug_status.name.lower() if plug_status else None +def _get_plug_state_icon(entity: RenaultDataEntity[T]) -> str: + """Return the icon of this entity.""" + if entity.data == PlugState.PLUGGED.value: + return "mdi:power-plug" + return "mdi:power-plug-off" + + +def _get_rounded_value(entity: RenaultDataEntity[T]) -> float: + """Return the icon of this entity.""" + return round(cast(float, entity.data)) + + SENSOR_TYPES: tuple[RenaultSensorEntityDescription, ...] = ( RenaultSensorEntityDescription( key="battery_level", @@ -133,17 +166,9 @@ SENSOR_TYPES: tuple[RenaultSensorEntityDescription, ...] = ( data_key="chargingStatus", device_class=DEVICE_CLASS_CHARGE_STATE, entity_class=RenaultSensor[KamereonVehicleBatteryStatusData], - icon_lambda=lambda x: ( - "mdi:flash" - if x.data == ChargeState.CHARGE_IN_PROGRESS.value - else "mdi:flash-off" - ), + icon_lambda=_get_charge_state_icon, name="Charge State", - value_lambda=lambda x: ( - _get_formatted_charging_status( - cast(KamereonVehicleBatteryStatusData, x.coordinator.data) - ) - ), + value_lambda=_get_charge_state_formatted, ), RenaultSensorEntityDescription( key="charging_remaining_time", @@ -164,11 +189,7 @@ SENSOR_TYPES: tuple[RenaultSensorEntityDescription, ...] = ( name="Charging Power", native_unit_of_measurement=POWER_KILO_WATT, state_class=STATE_CLASS_MEASUREMENT, - value_lambda=lambda x: ( - cast(float, x.data) / 1000 - if x.vehicle.details.reports_charging_power_in_watts() - else x.data - ), + value_lambda=_get_charging_power, ), RenaultSensorEntityDescription( key="plug_state", @@ -176,17 +197,9 @@ SENSOR_TYPES: tuple[RenaultSensorEntityDescription, ...] = ( data_key="plugStatus", device_class=DEVICE_CLASS_PLUG_STATE, entity_class=RenaultSensor[KamereonVehicleBatteryStatusData], - icon_lambda=lambda x: ( - "mdi:power-plug" - if x.data == PlugState.PLUGGED.value - else "mdi:power-plug-off" - ), + icon_lambda=_get_plug_state_icon, name="Plug State", - value_lambda=lambda x: ( - _get_formatted_plug_status( - cast(KamereonVehicleBatteryStatusData, x.coordinator.data) - ) - ), + value_lambda=_get_plug_state_formatted, ), RenaultSensorEntityDescription( key="battery_autonomy", @@ -227,7 +240,7 @@ SENSOR_TYPES: tuple[RenaultSensorEntityDescription, ...] = ( name="Mileage", native_unit_of_measurement=LENGTH_KILOMETERS, state_class=STATE_CLASS_TOTAL_INCREASING, - value_lambda=lambda x: round(cast(float, x.data)), + value_lambda=_get_rounded_value, ), RenaultSensorEntityDescription( key="fuel_autonomy", @@ -239,7 +252,7 @@ SENSOR_TYPES: tuple[RenaultSensorEntityDescription, ...] = ( native_unit_of_measurement=LENGTH_KILOMETERS, state_class=STATE_CLASS_MEASUREMENT, requires_fuel=True, - value_lambda=lambda x: round(cast(float, x.data)), + value_lambda=_get_rounded_value, ), RenaultSensorEntityDescription( key="fuel_quantity", @@ -251,7 +264,7 @@ SENSOR_TYPES: tuple[RenaultSensorEntityDescription, ...] = ( native_unit_of_measurement=VOLUME_LITERS, state_class=STATE_CLASS_MEASUREMENT, requires_fuel=True, - value_lambda=lambda x: round(cast(float, x.data)), + value_lambda=_get_rounded_value, ), RenaultSensorEntityDescription( key="outside_temperature", @@ -269,9 +282,7 @@ SENSOR_TYPES: tuple[RenaultSensorEntityDescription, ...] = ( data_key="chargeMode", device_class=DEVICE_CLASS_CHARGE_MODE, entity_class=RenaultSensor[KamereonVehicleChargeModeData], - icon_lambda=lambda x: ( - "mdi:calendar-clock" if x.data == "schedule_mode" else "mdi:calendar-remove" - ), + icon_lambda=_get_charge_mode_icon, name="Charge Mode", ), ) diff --git a/tests/components/renault/__init__.py b/tests/components/renault/__init__.py index 9191851c777..f77c4bcd40a 100644 --- a/tests/components/renault/__init__.py +++ b/tests/components/renault/__init__.py @@ -1,6 +1,7 @@ """Tests for the Renault integration.""" from __future__ import annotations +from types import MappingProxyType from typing import Any from unittest.mock import patch @@ -10,6 +11,7 @@ from renault_api.renault_account import RenaultAccount from homeassistant.components.renault.const import DOMAIN from homeassistant.config_entries import SOURCE_USER from homeassistant.const import ( + ATTR_ICON, ATTR_IDENTIFIERS, ATTR_MANUFACTURER, ATTR_MODEL, @@ -20,7 +22,7 @@ from homeassistant.core import HomeAssistant from homeassistant.helpers import aiohttp_client from homeassistant.helpers.device_registry import DeviceRegistry -from .const import MOCK_CONFIG, MOCK_VEHICLES +from .const import ICON_FOR_EMPTY_VALUES, MOCK_CONFIG, MOCK_VEHICLES from tests.common import MockConfigEntry, load_fixture @@ -64,6 +66,12 @@ def get_fixtures(vehicle_type: str) -> dict[str, Any]: } +def get_no_data_icon(expected_entity: MappingProxyType): + """Check attribute for icon for inactive sensors.""" + entity_id = expected_entity["entity_id"] + return ICON_FOR_EMPTY_VALUES.get(entity_id, expected_entity.get(ATTR_ICON)) + + async def setup_renault_integration_simple(hass: HomeAssistant): """Create the Renault integration.""" config_entry = get_mock_config_entry() diff --git a/tests/components/renault/const.py b/tests/components/renault/const.py index c100f85c498..e955f24018a 100644 --- a/tests/components/renault/const.py +++ b/tests/components/renault/const.py @@ -41,13 +41,21 @@ from homeassistant.const import ( VOLUME_LITERS, ) -CHECK_ATTRIBUTES = ( +FIXED_ATTRIBUTES = ( ATTR_DEVICE_CLASS, - ATTR_ICON, - ATTR_LAST_UPDATE, ATTR_STATE_CLASS, ATTR_UNIT_OF_MEASUREMENT, ) +DYNAMIC_ATTRIBUTES = ( + ATTR_ICON, + ATTR_LAST_UPDATE, +) + +ICON_FOR_EMPTY_VALUES = { + "sensor.charge_mode": "mdi:calendar-remove", + "sensor.charge_state": "mdi:flash-off", + "sensor.plug_state": "mdi:power-plug-off", +} # Mock config data to be used across multiple tests MOCK_CONFIG = { diff --git a/tests/components/renault/test_binary_sensor.py b/tests/components/renault/test_binary_sensor.py index c357d9d7a5a..a89c1da7808 100644 --- a/tests/components/renault/test_binary_sensor.py +++ b/tests/components/renault/test_binary_sensor.py @@ -6,22 +6,24 @@ from renault_api.kamereon import exceptions from homeassistant.components.binary_sensor import DOMAIN as BINARY_SENSOR_DOMAIN from homeassistant.components.renault.renault_entities import ATTR_LAST_UPDATE -from homeassistant.const import STATE_OFF, STATE_UNAVAILABLE +from homeassistant.const import ATTR_ICON, STATE_OFF, STATE_UNAVAILABLE +from homeassistant.core import HomeAssistant from homeassistant.setup import async_setup_component from . import ( check_device_registry, + get_no_data_icon, setup_renault_integration_vehicle, setup_renault_integration_vehicle_with_no_data, setup_renault_integration_vehicle_with_side_effect, ) -from .const import CHECK_ATTRIBUTES, MOCK_VEHICLES +from .const import DYNAMIC_ATTRIBUTES, FIXED_ATTRIBUTES, MOCK_VEHICLES from tests.common import mock_device_registry, mock_registry @pytest.mark.parametrize("vehicle_type", MOCK_VEHICLES.keys()) -async def test_binary_sensors(hass, vehicle_type): +async def test_binary_sensors(hass: HomeAssistant, vehicle_type: str): """Test for Renault binary sensors.""" await async_setup_component(hass, "persistent_notification", {}) entity_registry = mock_registry(hass) @@ -43,12 +45,12 @@ async def test_binary_sensors(hass, vehicle_type): assert registry_entry.unique_id == expected_entity["unique_id"] state = hass.states.get(entity_id) assert state.state == expected_entity["result"] - for attr in CHECK_ATTRIBUTES: + for attr in FIXED_ATTRIBUTES + DYNAMIC_ATTRIBUTES: assert state.attributes.get(attr) == expected_entity.get(attr) @pytest.mark.parametrize("vehicle_type", MOCK_VEHICLES.keys()) -async def test_binary_sensor_empty(hass, vehicle_type): +async def test_binary_sensor_empty(hass: HomeAssistant, vehicle_type: str): """Test for Renault binary sensors with empty data from Renault.""" await async_setup_component(hass, "persistent_notification", {}) entity_registry = mock_registry(hass) @@ -70,15 +72,15 @@ async def test_binary_sensor_empty(hass, vehicle_type): assert registry_entry.unique_id == expected_entity["unique_id"] state = hass.states.get(entity_id) assert state.state == STATE_OFF - for attr in CHECK_ATTRIBUTES: - if attr == ATTR_LAST_UPDATE: - assert state.attributes.get(attr) is None - else: - assert state.attributes.get(attr) == expected_entity.get(attr) + for attr in FIXED_ATTRIBUTES: + assert state.attributes.get(attr) == expected_entity.get(attr) + # Check dynamic attributes: + assert state.attributes.get(ATTR_ICON) == get_no_data_icon(expected_entity) + assert ATTR_LAST_UPDATE not in state.attributes @pytest.mark.parametrize("vehicle_type", MOCK_VEHICLES.keys()) -async def test_binary_sensor_errors(hass, vehicle_type): +async def test_binary_sensor_errors(hass: HomeAssistant, vehicle_type: str): """Test for Renault binary sensors with temporary failure.""" await async_setup_component(hass, "persistent_notification", {}) entity_registry = mock_registry(hass) @@ -107,11 +109,11 @@ async def test_binary_sensor_errors(hass, vehicle_type): assert registry_entry.unique_id == expected_entity["unique_id"] state = hass.states.get(entity_id) assert state.state == STATE_UNAVAILABLE - for attr in CHECK_ATTRIBUTES: - if attr == ATTR_LAST_UPDATE: - assert state.attributes.get(attr) is None - else: - assert state.attributes.get(attr) == expected_entity.get(attr) + for attr in FIXED_ATTRIBUTES: + assert state.attributes.get(attr) == expected_entity.get(attr) + # Check dynamic attributes: + assert state.attributes.get(ATTR_ICON) == get_no_data_icon(expected_entity) + assert ATTR_LAST_UPDATE not in state.attributes async def test_binary_sensor_access_denied(hass): diff --git a/tests/components/renault/test_init.py b/tests/components/renault/test_init.py index 37a67151972..3446bb1f9fa 100644 --- a/tests/components/renault/test_init.py +++ b/tests/components/renault/test_init.py @@ -6,11 +6,12 @@ from renault_api.gigya.exceptions import InvalidCredentialsException from homeassistant.components.renault.const import DOMAIN from homeassistant.config_entries import ConfigEntryState +from homeassistant.core import HomeAssistant from . import get_mock_config_entry, setup_renault_integration_simple -async def test_setup_unload_entry(hass): +async def test_setup_unload_entry(hass: HomeAssistant): """Test entry setup and unload.""" with patch("homeassistant.components.renault.PLATFORMS", []): config_entry = await setup_renault_integration_simple(hass) @@ -26,7 +27,7 @@ async def test_setup_unload_entry(hass): assert config_entry.entry_id not in hass.data[DOMAIN] -async def test_setup_entry_bad_password(hass): +async def test_setup_entry_bad_password(hass: HomeAssistant): """Test entry setup and unload.""" # Create a mock entry so we don't have to go through config flow config_entry = get_mock_config_entry() @@ -44,7 +45,7 @@ async def test_setup_entry_bad_password(hass): assert not hass.data.get(DOMAIN) -async def test_setup_entry_exception(hass): +async def test_setup_entry_exception(hass: HomeAssistant): """Test ConfigEntryNotReady when API raises an exception during entry setup.""" config_entry = get_mock_config_entry() config_entry.add_to_hass(hass) diff --git a/tests/components/renault/test_sensor.py b/tests/components/renault/test_sensor.py index 4bc4d96d75f..370721bb0dd 100644 --- a/tests/components/renault/test_sensor.py +++ b/tests/components/renault/test_sensor.py @@ -4,52 +4,26 @@ from unittest.mock import patch import pytest from renault_api.kamereon import exceptions -from homeassistant.components.renault.const import ( - DEVICE_CLASS_CHARGE_MODE, - DEVICE_CLASS_CHARGE_STATE, - DEVICE_CLASS_PLUG_STATE, -) from homeassistant.components.renault.renault_entities import ATTR_LAST_UPDATE from homeassistant.components.sensor import DOMAIN as SENSOR_DOMAIN -from homeassistant.const import ( - ATTR_DEVICE_CLASS, - ATTR_ICON, - STATE_UNAVAILABLE, - STATE_UNKNOWN, -) -from homeassistant.core import State +from homeassistant.const import ATTR_ICON, STATE_UNAVAILABLE, STATE_UNKNOWN +from homeassistant.core import HomeAssistant from homeassistant.setup import async_setup_component from . import ( check_device_registry, + get_no_data_icon, setup_renault_integration_vehicle, setup_renault_integration_vehicle_with_no_data, setup_renault_integration_vehicle_with_side_effect, ) -from .const import CHECK_ATTRIBUTES, MOCK_VEHICLES +from .const import DYNAMIC_ATTRIBUTES, FIXED_ATTRIBUTES, MOCK_VEHICLES from tests.common import mock_device_registry, mock_registry -def check_inactive_attribute(state: State, attr: str, expected_entity: dict): - """Check attribute for icon for inactive sensors.""" - if attr == ATTR_LAST_UPDATE: - assert state.attributes.get(attr) is None - elif attr == ATTR_ICON: - if expected_entity.get(ATTR_DEVICE_CLASS) == DEVICE_CLASS_CHARGE_MODE: - assert state.attributes.get(ATTR_ICON) == "mdi:calendar-remove" - elif expected_entity.get(ATTR_DEVICE_CLASS) == DEVICE_CLASS_CHARGE_STATE: - assert state.attributes.get(ATTR_ICON) == "mdi:flash-off" - elif expected_entity.get(ATTR_DEVICE_CLASS) == DEVICE_CLASS_PLUG_STATE: - assert state.attributes.get(ATTR_ICON) == "mdi:power-plug-off" - else: - assert state.attributes.get(ATTR_ICON) == expected_entity.get(ATTR_ICON) - else: - assert state.attributes.get(attr) == expected_entity.get(attr) - - @pytest.mark.parametrize("vehicle_type", MOCK_VEHICLES.keys()) -async def test_sensors(hass, vehicle_type): +async def test_sensors(hass: HomeAssistant, vehicle_type: str): """Test for Renault sensors.""" await async_setup_component(hass, "persistent_notification", {}) entity_registry = mock_registry(hass) @@ -71,12 +45,12 @@ async def test_sensors(hass, vehicle_type): assert registry_entry.unique_id == expected_entity["unique_id"] state = hass.states.get(entity_id) assert state.state == expected_entity["result"] - for attr in CHECK_ATTRIBUTES: + for attr in FIXED_ATTRIBUTES + DYNAMIC_ATTRIBUTES: assert state.attributes.get(attr) == expected_entity.get(attr) @pytest.mark.parametrize("vehicle_type", MOCK_VEHICLES.keys()) -async def test_sensor_empty(hass, vehicle_type): +async def test_sensor_empty(hass: HomeAssistant, vehicle_type: str): """Test for Renault sensors with empty data from Renault.""" await async_setup_component(hass, "persistent_notification", {}) entity_registry = mock_registry(hass) @@ -98,12 +72,15 @@ async def test_sensor_empty(hass, vehicle_type): assert registry_entry.unique_id == expected_entity["unique_id"] state = hass.states.get(entity_id) assert state.state == STATE_UNKNOWN - for attr in CHECK_ATTRIBUTES: - check_inactive_attribute(state, attr, expected_entity) + for attr in FIXED_ATTRIBUTES: + assert state.attributes.get(attr) == expected_entity.get(attr) + # Check dynamic attributes: + assert state.attributes.get(ATTR_ICON) == get_no_data_icon(expected_entity) + assert ATTR_LAST_UPDATE not in state.attributes @pytest.mark.parametrize("vehicle_type", MOCK_VEHICLES.keys()) -async def test_sensor_errors(hass, vehicle_type): +async def test_sensor_errors(hass: HomeAssistant, vehicle_type: str): """Test for Renault sensors with temporary failure.""" await async_setup_component(hass, "persistent_notification", {}) entity_registry = mock_registry(hass) @@ -132,11 +109,14 @@ async def test_sensor_errors(hass, vehicle_type): assert registry_entry.unique_id == expected_entity["unique_id"] state = hass.states.get(entity_id) assert state.state == STATE_UNAVAILABLE - for attr in CHECK_ATTRIBUTES: - check_inactive_attribute(state, attr, expected_entity) + for attr in FIXED_ATTRIBUTES: + assert state.attributes.get(attr) == expected_entity.get(attr) + # Check dynamic attributes: + assert state.attributes.get(ATTR_ICON) == get_no_data_icon(expected_entity) + assert ATTR_LAST_UPDATE not in state.attributes -async def test_sensor_access_denied(hass): +async def test_sensor_access_denied(hass: HomeAssistant): """Test for Renault sensors with access denied failure.""" await async_setup_component(hass, "persistent_notification", {}) entity_registry = mock_registry(hass) @@ -160,7 +140,7 @@ async def test_sensor_access_denied(hass): assert len(entity_registry.entities) == 0 -async def test_sensor_not_supported(hass): +async def test_sensor_not_supported(hass: HomeAssistant): """Test for Renault sensors with access denied failure.""" await async_setup_component(hass, "persistent_notification", {}) entity_registry = mock_registry(hass)