From 073951177b31f90be7232b03df0fd4db77cb3089 Mon Sep 17 00:00:00 2001 From: Garrett <7310260+G-Two@users.noreply.github.com> Date: Sun, 23 Oct 2022 19:54:22 -0400 Subject: [PATCH] Code quality update for Subaru sensors (#79482) * Use distance device class for sensors * Change sensor name casing and unique_id * Migrate sensor entity unique_id * Match title-cased unique_id when migrating * Remove unneeded regex to find '_' delimited id suffix * Incorporate PR review comments * Add check to prevent extra odometer entity migration --- homeassistant/components/subaru/sensor.py | 75 ++++++++++++++++---- tests/components/subaru/conftest.py | 54 +++++++------- tests/components/subaru/test_init.py | 48 ++++++++----- tests/components/subaru/test_sensor.py | 85 +++++++++++++++++++++++ 4 files changed, 205 insertions(+), 57 deletions(-) diff --git a/homeassistant/components/subaru/sensor.py b/homeassistant/components/subaru/sensor.py index bb467fc77de..cae5a7b14a4 100644 --- a/homeassistant/components/subaru/sensor.py +++ b/homeassistant/components/subaru/sensor.py @@ -23,7 +23,8 @@ from homeassistant.const import ( VOLUME_GALLONS, VOLUME_LITERS, ) -from homeassistant.core import HomeAssistant +from homeassistant.core import HomeAssistant, callback +from homeassistant.helpers import entity_registry as er from homeassistant.helpers.entity_platform import AddEntitiesCallback from homeassistant.helpers.update_coordinator import ( CoordinatorEntity, @@ -51,6 +52,7 @@ from .const import ( _LOGGER = logging.getLogger(__name__) + # Fuel consumption units FUEL_CONSUMPTION_LITERS_PER_HUNDRED_KILOMETERS = "L/100km" FUEL_CONSUMPTION_MILES_PER_GALLON = "mi/gal" @@ -62,6 +64,7 @@ KM_PER_MI = DistanceConverter.convert(1, LENGTH_MILES, LENGTH_KILOMETERS) SAFETY_SENSORS = [ SensorEntityDescription( key=sc.ODOMETER, + device_class=SensorDeviceClass.DISTANCE, icon="mdi:road-variant", name="Odometer", native_unit_of_measurement=LENGTH_KILOMETERS, @@ -74,12 +77,13 @@ API_GEN_2_SENSORS = [ SensorEntityDescription( key=sc.AVG_FUEL_CONSUMPTION, icon="mdi:leaf", - name="Avg Fuel Consumption", + name="Avg fuel consumption", native_unit_of_measurement=FUEL_CONSUMPTION_LITERS_PER_HUNDRED_KILOMETERS, state_class=SensorStateClass.MEASUREMENT, ), SensorEntityDescription( key=sc.DIST_TO_EMPTY, + device_class=SensorDeviceClass.DISTANCE, icon="mdi:gas-station", name="Range", native_unit_of_measurement=LENGTH_KILOMETERS, @@ -88,42 +92,42 @@ API_GEN_2_SENSORS = [ SensorEntityDescription( key=sc.TIRE_PRESSURE_FL, device_class=SensorDeviceClass.PRESSURE, - name="Tire Pressure FL", + name="Tire pressure FL", native_unit_of_measurement=PRESSURE_HPA, state_class=SensorStateClass.MEASUREMENT, ), SensorEntityDescription( key=sc.TIRE_PRESSURE_FR, device_class=SensorDeviceClass.PRESSURE, - name="Tire Pressure FR", + name="Tire pressure FR", native_unit_of_measurement=PRESSURE_HPA, state_class=SensorStateClass.MEASUREMENT, ), SensorEntityDescription( key=sc.TIRE_PRESSURE_RL, device_class=SensorDeviceClass.PRESSURE, - name="Tire Pressure RL", + name="Tire pressure RL", native_unit_of_measurement=PRESSURE_HPA, state_class=SensorStateClass.MEASUREMENT, ), SensorEntityDescription( key=sc.TIRE_PRESSURE_RR, device_class=SensorDeviceClass.PRESSURE, - name="Tire Pressure RR", + name="Tire pressure RR", native_unit_of_measurement=PRESSURE_HPA, state_class=SensorStateClass.MEASUREMENT, ), SensorEntityDescription( key=sc.EXTERNAL_TEMP, device_class=SensorDeviceClass.TEMPERATURE, - name="External Temp", + name="External temp", native_unit_of_measurement=TEMP_CELSIUS, state_class=SensorStateClass.MEASUREMENT, ), SensorEntityDescription( key=sc.BATTERY_VOLTAGE, device_class=SensorDeviceClass.VOLTAGE, - name="12V Battery Voltage", + name="12V battery voltage", native_unit_of_measurement=ELECTRIC_POTENTIAL_VOLT, state_class=SensorStateClass.MEASUREMENT, ), @@ -133,22 +137,23 @@ API_GEN_2_SENSORS = [ EV_SENSORS = [ SensorEntityDescription( key=sc.EV_DISTANCE_TO_EMPTY, + device_class=SensorDeviceClass.DISTANCE, icon="mdi:ev-station", - name="EV Range", + name="EV range", native_unit_of_measurement=LENGTH_MILES, state_class=SensorStateClass.MEASUREMENT, ), SensorEntityDescription( key=sc.EV_STATE_OF_CHARGE_PERCENT, device_class=SensorDeviceClass.BATTERY, - name="EV Battery Level", + name="EV battery level", native_unit_of_measurement=PERCENTAGE, state_class=SensorStateClass.MEASUREMENT, ), SensorEntityDescription( key=sc.EV_TIME_TO_FULLY_CHARGED_UTC, device_class=SensorDeviceClass.TIMESTAMP, - name="EV Time to Full Charge", + name="EV time to full charge", state_class=SensorStateClass.MEASUREMENT, ), ] @@ -164,6 +169,7 @@ async def async_setup_entry( coordinator = entry[ENTRY_COORDINATOR] vehicle_info = entry[ENTRY_VEHICLES] entities = [] + await _async_migrate_entries(hass, config_entry) for info in vehicle_info.values(): entities.extend(create_vehicle_sensors(info, coordinator)) async_add_entities(entities) @@ -211,7 +217,7 @@ class SubaruSensor( self.vin = vehicle_info[VEHICLE_VIN] self.entity_description = description self._attr_device_info = get_device_info(vehicle_info) - self._attr_unique_id = f"{self.vin}_{description.name}" + self._attr_unique_id = f"{self.vin}_{description.key}" @property def native_value(self) -> None | int | float: @@ -273,3 +279,48 @@ class SubaruSensor( if last_update_success and self.vin not in self.coordinator.data: return False return last_update_success + + +async def _async_migrate_entries( + hass: HomeAssistant, config_entry: ConfigEntry +) -> None: + """Migrate sensor entries from HA<=2022.10 to use preferred unique_id.""" + entity_registry = er.async_get(hass) + + all_sensors = [] + all_sensors.extend(EV_SENSORS) + all_sensors.extend(API_GEN_2_SENSORS) + all_sensors.extend(SAFETY_SENSORS) + + # Old unique_id is (previously title-cased) sensor name (e.g. "VIN_Avg Fuel Consumption") + replacements = {str(s.name).upper(): s.key for s in all_sensors} + + @callback + def update_unique_id(entry: er.RegistryEntry) -> dict[str, Any] | None: + id_split = entry.unique_id.split("_") + key = id_split[1].upper() if len(id_split) == 2 else None + + if key not in replacements or id_split[1] == replacements[key]: + return None + + new_unique_id = entry.unique_id.replace(id_split[1], replacements[key]) + _LOGGER.debug( + "Migrating entity '%s' unique_id from '%s' to '%s'", + entry.entity_id, + entry.unique_id, + new_unique_id, + ) + if existing_entity_id := entity_registry.async_get_entity_id( + entry.domain, entry.platform, new_unique_id + ): + _LOGGER.debug( + "Cannot migrate to unique_id '%s', already exists for '%s'", + new_unique_id, + existing_entity_id, + ) + return None + return { + "new_unique_id": new_unique_id, + } + + await er.async_migrate_entries(hass, config_entry.entry_id, update_unique_id) diff --git a/tests/components/subaru/conftest.py b/tests/components/subaru/conftest.py index 492ec06c1e8..20d70a7d496 100644 --- a/tests/components/subaru/conftest.py +++ b/tests/components/subaru/conftest.py @@ -5,6 +5,7 @@ from unittest.mock import patch import pytest from subarulink.const import COUNTRY_USA +from homeassistant import config_entries from homeassistant.components.homeassistant import DOMAIN as HA_DOMAIN from homeassistant.components.subaru.const import ( CONF_COUNTRY, @@ -71,6 +72,15 @@ TEST_OPTIONS = { CONF_UPDATE_ENABLED: True, } +TEST_CONFIG_ENTRY = { + "entry_id": "1", + "domain": DOMAIN, + "title": TEST_CONFIG[CONF_USERNAME], + "data": TEST_CONFIG, + "options": TEST_OPTIONS, + "source": config_entries.SOURCE_USER, +} + TEST_DEVICE_NAME = "test_vehicle_2" TEST_ENTITY_ID = f"sensor.{TEST_DEVICE_NAME}_odometer" @@ -81,26 +91,16 @@ def advance_time_to_next_fetch(hass): async_fire_time_changed(hass, future) -async def setup_subaru_integration( +async def setup_subaru_config_entry( hass, - vehicle_list=None, - vehicle_data=None, - vehicle_status=None, + config_entry, + vehicle_list=[TEST_VIN_2_EV], + vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV], + vehicle_status=VEHICLE_STATUS_EV, connect_effect=None, fetch_effect=None, ): - """Create Subaru entry.""" - assert await async_setup_component(hass, HA_DOMAIN, {}) - assert await async_setup_component(hass, DOMAIN, {}) - - config_entry = MockConfigEntry( - domain=DOMAIN, - data=TEST_CONFIG, - options=TEST_OPTIONS, - entry_id=1, - ) - config_entry.add_to_hass(hass) - + """Run async_setup with API mocks in place.""" with patch( MOCK_API_CONNECT, return_value=connect_effect is None, @@ -134,20 +134,22 @@ async def setup_subaru_integration( await hass.config_entries.async_setup(config_entry.entry_id) await hass.async_block_till_done() + +@pytest.fixture +async def subaru_config_entry(hass): + """Create a Subaru config entry prior to setup.""" + await async_setup_component(hass, HA_DOMAIN, {}) + config_entry = MockConfigEntry(**TEST_CONFIG_ENTRY) + config_entry.add_to_hass(hass) return config_entry @pytest.fixture -async def ev_entry(hass): +async def ev_entry(hass, subaru_config_entry): """Create a Subaru entry representing an EV vehicle with full STARLINK subscription.""" - entry = await setup_subaru_integration( - hass, - vehicle_list=[TEST_VIN_2_EV], - vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV], - vehicle_status=VEHICLE_STATUS_EV, - ) + await setup_subaru_config_entry(hass, subaru_config_entry) assert DOMAIN in hass.config_entries.async_domains() assert len(hass.config_entries.async_entries(DOMAIN)) == 1 - assert hass.config_entries.async_get_entry(entry.entry_id) - assert entry.state is ConfigEntryState.LOADED - return entry + assert hass.config_entries.async_get_entry(subaru_config_entry.entry_id) + assert subaru_config_entry.state is ConfigEntryState.LOADED + return subaru_config_entry diff --git a/tests/components/subaru/test_init.py b/tests/components/subaru/test_init.py index cd87ed40315..46a8b2e103b 100644 --- a/tests/components/subaru/test_init.py +++ b/tests/components/subaru/test_init.py @@ -24,7 +24,7 @@ from .conftest import ( MOCK_API_FETCH, MOCK_API_UPDATE, TEST_ENTITY_ID, - setup_subaru_integration, + setup_subaru_config_entry, ) @@ -42,61 +42,70 @@ async def test_setup_ev(hass, ev_entry): assert check_entry.state is ConfigEntryState.LOADED -async def test_setup_g2(hass): +async def test_setup_g2(hass, subaru_config_entry): """Test setup with a G2 vehcile .""" - entry = await setup_subaru_integration( + await setup_subaru_config_entry( hass, + subaru_config_entry, vehicle_list=[TEST_VIN_3_G2], vehicle_data=VEHICLE_DATA[TEST_VIN_3_G2], vehicle_status=VEHICLE_STATUS_G2, ) - check_entry = hass.config_entries.async_get_entry(entry.entry_id) + check_entry = hass.config_entries.async_get_entry(subaru_config_entry.entry_id) assert check_entry assert check_entry.state is ConfigEntryState.LOADED -async def test_setup_g1(hass): +async def test_setup_g1(hass, subaru_config_entry): """Test setup with a G1 vehicle.""" - entry = await setup_subaru_integration( - hass, vehicle_list=[TEST_VIN_1_G1], vehicle_data=VEHICLE_DATA[TEST_VIN_1_G1] + await setup_subaru_config_entry( + hass, + subaru_config_entry, + vehicle_list=[TEST_VIN_1_G1], + vehicle_data=VEHICLE_DATA[TEST_VIN_1_G1], ) - check_entry = hass.config_entries.async_get_entry(entry.entry_id) + check_entry = hass.config_entries.async_get_entry(subaru_config_entry.entry_id) assert check_entry assert check_entry.state is ConfigEntryState.LOADED -async def test_unsuccessful_connect(hass): +async def test_unsuccessful_connect(hass, subaru_config_entry): """Test unsuccessful connect due to connectivity.""" - entry = await setup_subaru_integration( + await setup_subaru_config_entry( hass, + subaru_config_entry, connect_effect=SubaruException("Service Unavailable"), vehicle_list=[TEST_VIN_2_EV], vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV], vehicle_status=VEHICLE_STATUS_EV, ) - check_entry = hass.config_entries.async_get_entry(entry.entry_id) + check_entry = hass.config_entries.async_get_entry(subaru_config_entry.entry_id) assert check_entry assert check_entry.state is ConfigEntryState.SETUP_RETRY -async def test_invalid_credentials(hass): +async def test_invalid_credentials(hass, subaru_config_entry): """Test invalid credentials.""" - entry = await setup_subaru_integration( + await setup_subaru_config_entry( hass, + subaru_config_entry, connect_effect=InvalidCredentials("Invalid Credentials"), vehicle_list=[TEST_VIN_2_EV], vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV], vehicle_status=VEHICLE_STATUS_EV, ) - check_entry = hass.config_entries.async_get_entry(entry.entry_id) + check_entry = hass.config_entries.async_get_entry(subaru_config_entry.entry_id) assert check_entry assert check_entry.state is ConfigEntryState.SETUP_ERROR -async def test_update_skip_unsubscribed(hass): +async def test_update_skip_unsubscribed(hass, subaru_config_entry): """Test update function skips vehicles without subscription.""" - await setup_subaru_integration( - hass, vehicle_list=[TEST_VIN_1_G1], vehicle_data=VEHICLE_DATA[TEST_VIN_1_G1] + await setup_subaru_config_entry( + hass, + subaru_config_entry, + vehicle_list=[TEST_VIN_1_G1], + vehicle_data=VEHICLE_DATA[TEST_VIN_1_G1], ) with patch(MOCK_API_FETCH) as mock_fetch: @@ -126,10 +135,11 @@ async def test_update_disabled(hass, ev_entry): mock_update.assert_not_called() -async def test_fetch_failed(hass): +async def test_fetch_failed(hass, subaru_config_entry): """Tests when fetch fails.""" - await setup_subaru_integration( + await setup_subaru_config_entry( hass, + subaru_config_entry, vehicle_list=[TEST_VIN_2_EV], vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV], vehicle_status=VEHICLE_STATUS_EV, diff --git a/tests/components/subaru/test_sensor.py b/tests/components/subaru/test_sensor.py index aec9e6ede7a..caec43d36e8 100644 --- a/tests/components/subaru/test_sensor.py +++ b/tests/components/subaru/test_sensor.py @@ -1,11 +1,16 @@ """Test Subaru sensors.""" from unittest.mock import patch +import pytest + +from homeassistant.components.sensor import DOMAIN as SENSOR_DOMAIN from homeassistant.components.subaru.sensor import ( API_GEN_2_SENSORS, + DOMAIN as SUBARU_DOMAIN, EV_SENSORS, SAFETY_SENSORS, ) +from homeassistant.helpers import entity_registry as er from homeassistant.util import slugify from homeassistant.util.unit_system import US_CUSTOMARY_SYSTEM @@ -13,6 +18,7 @@ from .api_responses import ( EXPECTED_STATE_EV_IMPERIAL, EXPECTED_STATE_EV_METRIC, EXPECTED_STATE_EV_UNAVAILABLE, + TEST_VIN_2_EV, VEHICLE_STATUS_EV, ) from .conftest import ( @@ -20,6 +26,7 @@ from .conftest import ( MOCK_API_GET_DATA, TEST_DEVICE_NAME, advance_time_to_next_fetch, + setup_subaru_config_entry, ) @@ -50,6 +57,84 @@ async def test_sensors_missing_vin_data(hass, ev_entry): _assert_data(hass, EXPECTED_STATE_EV_UNAVAILABLE) +@pytest.mark.parametrize( + "entitydata,old_unique_id,new_unique_id", + [ + ( + { + "domain": SENSOR_DOMAIN, + "platform": SUBARU_DOMAIN, + "unique_id": f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].name}", + }, + f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].name}", + f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].key}", + ), + ], +) +async def test_sensor_migrate_unique_ids( + hass, entitydata, old_unique_id, new_unique_id, subaru_config_entry +) -> None: + """Test successful migration of entity unique_ids.""" + entity_registry = er.async_get(hass) + entity: er.RegistryEntry = entity_registry.async_get_or_create( + **entitydata, + config_entry=subaru_config_entry, + ) + assert entity.unique_id == old_unique_id + + await setup_subaru_config_entry(hass, subaru_config_entry) + + entity_migrated = entity_registry.async_get(entity.entity_id) + assert entity_migrated + assert entity_migrated.unique_id == new_unique_id + + +@pytest.mark.parametrize( + "entitydata,old_unique_id,new_unique_id", + [ + ( + { + "domain": SENSOR_DOMAIN, + "platform": SUBARU_DOMAIN, + "unique_id": f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].name}", + }, + f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].name}", + f"{TEST_VIN_2_EV}_{API_GEN_2_SENSORS[0].key}", + ) + ], +) +async def test_sensor_migrate_unique_ids_duplicate( + hass, entitydata, old_unique_id, new_unique_id, subaru_config_entry +) -> None: + """Test unsuccessful migration of entity unique_ids due to duplicate.""" + entity_registry = er.async_get(hass) + entity: er.RegistryEntry = entity_registry.async_get_or_create( + **entitydata, + config_entry=subaru_config_entry, + ) + assert entity.unique_id == old_unique_id + + # create existing entry with new_unique_id that conflicts with migrate + existing_entity = entity_registry.async_get_or_create( + SENSOR_DOMAIN, + SUBARU_DOMAIN, + unique_id=new_unique_id, + config_entry=subaru_config_entry, + ) + + await setup_subaru_config_entry(hass, subaru_config_entry) + + entity_migrated = entity_registry.async_get(entity.entity_id) + assert entity_migrated + assert entity_migrated.unique_id == old_unique_id + + entity_not_changed = entity_registry.async_get(existing_entity.entity_id) + assert entity_not_changed + assert entity_not_changed.unique_id == new_unique_id + + assert entity_migrated != entity_not_changed + + def _assert_data(hass, expected_state): sensor_list = EV_SENSORS sensor_list.extend(API_GEN_2_SENSORS)