Subaru integration code quality changes (#48193)

* Apply changes from code review

* Update sensor tests

* Fix pylint error

* Apply suggestions from code review

Co-authored-by: Brandon Rothweiler <brandonrothweiler@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

Co-authored-by: Brandon Rothweiler <brandonrothweiler@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

homeassistant/components/subaru/config_flow.py

@@ -28,8 +28,11 @@ class SubaruConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
 
     VERSION = 1
     CONNECTION_CLASS = config_entries.CONN_CLASS_CLOUD_POLL
-    config_data = {CONF_PIN: None}
+
-    controller = None
+    def __init__(self):
+        """Initialize config flow."""
+        self.config_data = {CONF_PIN: None}
+        self.controller = None
 
     async def async_step_user(self, user_input=None):
         """Handle the start of the config flow."""
@@ -105,9 +108,8 @@ class SubaruConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
             device_name=device_name,
             country=data[CONF_COUNTRY],
         )
-        _LOGGER.debug("Using subarulink %s", self.controller.version)
         _LOGGER.debug(
-            "Setting up first time connection to Subuaru API; This may take up to 20 seconds"
+            "Setting up first time connection to Subaru API.  This may take up to 20 seconds"
         )
         if await self.controller.connect():
             _LOGGER.debug("Successfully authenticated and authorized with Subaru API")

homeassistant/components/subaru/const.py

@@ -36,12 +36,7 @@ PLATFORMS = [
 
 ICONS = {
     "Avg Fuel Consumption": "mdi:leaf",
-    "EV Time to Full Charge": "mdi:car-electric",
     "EV Range": "mdi:ev-station",
     "Odometer": "mdi:road-variant",
     "Range": "mdi:gas-station",
-    "Tire Pressure FL": "mdi:gauge",
-    "Tire Pressure FR": "mdi:gauge",
-    "Tire Pressure RL": "mdi:gauge",
-    "Tire Pressure RR": "mdi:gauge",
 }

homeassistant/components/subaru/entity.py

@@ -1,7 +1,7 @@
 """Base class for all Subaru Entities."""
 from homeassistant.helpers.update_coordinator import CoordinatorEntity
 
-from .const import DOMAIN, ICONS, MANUFACTURER, VEHICLE_NAME, VEHICLE_VIN
+from .const import DOMAIN, MANUFACTURER, VEHICLE_NAME, VEHICLE_VIN
 
 
 class SubaruEntity(CoordinatorEntity):
@@ -24,11 +24,6 @@ class SubaruEntity(CoordinatorEntity):
         """Return a unique ID."""
         return f"{self.vin}_{self.entity_type}"
 
-    @property
-    def icon(self):
-        """Return the icon of the sensor."""
-        return ICONS.get(self.entity_type)
-
     @property
     def device_info(self):
         """Return the device_info of the device."""

homeassistant/components/subaru/sensor.py

@@ -4,7 +4,9 @@ import subarulink.const as sc
 from homeassistant.components.sensor import DEVICE_CLASSES, SensorEntity
 from homeassistant.const import (
     DEVICE_CLASS_BATTERY,
+    DEVICE_CLASS_PRESSURE,
     DEVICE_CLASS_TEMPERATURE,
+    DEVICE_CLASS_TIMESTAMP,
     DEVICE_CLASS_VOLTAGE,
     LENGTH_KILOMETERS,
     LENGTH_MILES,
@@ -30,6 +32,7 @@ from .const import (
     DOMAIN,
     ENTRY_COORDINATOR,
     ENTRY_VEHICLES,
+    ICONS,
     VEHICLE_API_GEN,
     VEHICLE_HAS_EV,
     VEHICLE_HAS_SAFETY_SERVICE,
@@ -76,25 +79,25 @@ API_GEN_2_SENSORS = [
     },
     {
         SENSOR_TYPE: "Tire Pressure FL",
-        SENSOR_CLASS: None,
+        SENSOR_CLASS: DEVICE_CLASS_PRESSURE,
         SENSOR_FIELD: sc.TIRE_PRESSURE_FL,
         SENSOR_UNITS: PRESSURE_HPA,
     },
     {
         SENSOR_TYPE: "Tire Pressure FR",
-        SENSOR_CLASS: None,
+        SENSOR_CLASS: DEVICE_CLASS_PRESSURE,
         SENSOR_FIELD: sc.TIRE_PRESSURE_FR,
         SENSOR_UNITS: PRESSURE_HPA,
     },
     {
         SENSOR_TYPE: "Tire Pressure RL",
-        SENSOR_CLASS: None,
+        SENSOR_CLASS: DEVICE_CLASS_PRESSURE,
         SENSOR_FIELD: sc.TIRE_PRESSURE_RL,
         SENSOR_UNITS: PRESSURE_HPA,
     },
     {
         SENSOR_TYPE: "Tire Pressure RR",
-        SENSOR_CLASS: None,
+        SENSOR_CLASS: DEVICE_CLASS_PRESSURE,
         SENSOR_FIELD: sc.TIRE_PRESSURE_RR,
         SENSOR_UNITS: PRESSURE_HPA,
     },
@@ -128,7 +131,7 @@ EV_SENSORS = [
     },
     {
         SENSOR_TYPE: "EV Time to Full Charge",
-        SENSOR_CLASS: None,
+        SENSOR_CLASS: DEVICE_CLASS_TIMESTAMP,
         SENSOR_FIELD: sc.EV_TIME_TO_FULLY_CHARGED,
         SENSOR_UNITS: TIME_MINUTES,
     },
@@ -140,7 +143,7 @@ async def async_setup_entry(hass, config_entry, async_add_entities):
     coordinator = hass.data[DOMAIN][config_entry.entry_id][ENTRY_COORDINATOR]
     vehicle_info = hass.data[DOMAIN][config_entry.entry_id][ENTRY_VEHICLES]
     entities = []
-    for vin in vehicle_info.keys():
+    for vin in vehicle_info:
         entities.extend(create_vehicle_sensors(vehicle_info[vin], coordinator))
     async_add_entities(entities, True)
 
@@ -190,7 +193,14 @@ class SubaruSensor(SubaruEntity, SensorEntity):
         """Return the class of this device, from component DEVICE_CLASSES."""
         if self.sensor_class in DEVICE_CLASSES:
             return self.sensor_class
-        return super().device_class
+        return None
+
+    @property
+    def icon(self):
+        """Return the icon of the sensor."""
+        if not self.device_class:
+            return ICONS.get(self.entity_type)
+        return None
 
     @property
     def state(self):

homeassistant/components/subaru/strings.json

@@ -22,8 +22,7 @@
       "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]",
       "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]",
       "incorrect_pin": "Incorrect PIN",
-      "bad_pin_format": "PIN should be 4 digits",
+      "bad_pin_format": "PIN should be 4 digits"
-      "unknown": "[%key:common::config_flow::error::unknown%]"
     },
     "abort": {
       "already_configured": "[%key:common::config_flow::abort::already_configured_account%]",

tests/components/subaru/conftest.py

@@ -1,4 +1,5 @@
 """Common functions needed to setup tests for Subaru component."""
+from datetime import timedelta
 from unittest.mock import patch
 
 import pytest
@@ -9,6 +10,7 @@ from homeassistant.components.subaru.const import (
     CONF_COUNTRY,
     CONF_UPDATE_ENABLED,
     DOMAIN,
+    FETCH_INTERVAL,
     VEHICLE_API_GEN,
     VEHICLE_HAS_EV,
     VEHICLE_HAS_REMOTE_SERVICE,
@@ -19,10 +21,11 @@ from homeassistant.components.subaru.const import (
 from homeassistant.config_entries import ENTRY_STATE_LOADED
 from homeassistant.const import CONF_DEVICE_ID, CONF_PASSWORD, CONF_PIN, CONF_USERNAME
 from homeassistant.setup import async_setup_component
+import homeassistant.util.dt as dt_util
 
 from .api_responses import TEST_VIN_2_EV, VEHICLE_DATA, VEHICLE_STATUS_EV
 
-from tests.common import MockConfigEntry
+from tests.common import MockConfigEntry, async_fire_time_changed
 
 MOCK_API = "homeassistant.components.subaru.SubaruAPI."
 MOCK_API_CONNECT = f"{MOCK_API}connect"
@@ -36,7 +39,7 @@ MOCK_API_GET_EV_STATUS = f"{MOCK_API}get_ev_status"
 MOCK_API_GET_RES_STATUS = f"{MOCK_API}get_res_status"
 MOCK_API_GET_REMOTE_STATUS = f"{MOCK_API}get_remote_status"
 MOCK_API_GET_SAFETY_STATUS = f"{MOCK_API}get_safety_status"
-MOCK_API_GET_GET_DATA = f"{MOCK_API}get_data"
+MOCK_API_GET_DATA = f"{MOCK_API}get_data"
 MOCK_API_UPDATE = f"{MOCK_API}update"
 MOCK_API_FETCH = f"{MOCK_API}fetch"
 
@@ -67,6 +70,12 @@ TEST_OPTIONS = {
 TEST_ENTITY_ID = "sensor.test_vehicle_2_odometer"
 
 
+def advance_time_to_next_fetch(hass):
+    """Fast forward time to next fetch."""
+    future = dt_util.utcnow() + timedelta(seconds=FETCH_INTERVAL + 30)
+    async_fire_time_changed(hass, future)
+
+
 async def setup_subaru_integration(
     hass,
     vehicle_list=None,
@@ -110,7 +119,7 @@ async def setup_subaru_integration(
         MOCK_API_GET_SAFETY_STATUS,
         return_value=vehicle_data[VEHICLE_HAS_SAFETY_SERVICE],
     ), patch(
-        MOCK_API_GET_GET_DATA,
+        MOCK_API_GET_DATA,
         return_value=vehicle_status,
     ), patch(
         MOCK_API_UPDATE,

tests/components/subaru/test_config_flow.py

@@ -11,6 +11,7 @@ from homeassistant import config_entries
 from homeassistant.components.subaru import config_flow
 from homeassistant.components.subaru.const import CONF_UPDATE_ENABLED, DOMAIN
 from homeassistant.const import CONF_DEVICE_ID, CONF_PIN
+from homeassistant.setup import async_setup_component
 
 from .conftest import (
     MOCK_API_CONNECT,
@@ -26,19 +27,17 @@ from .conftest import (
 
 from tests.common import MockConfigEntry
 
+ASYNC_SETUP = "homeassistant.components.subaru.async_setup"
+ASYNC_SETUP_ENTRY = "homeassistant.components.subaru.async_setup_entry"
+
 
 async def test_user_form_init(user_form):
     """Test the initial user form for first step of the config flow."""
-    expected = {
+    assert user_form["description_placeholders"] is None
-        "data_schema": mock.ANY,
+    assert user_form["errors"] is None
-        "description_placeholders": None,
+    assert user_form["handler"] == DOMAIN
-        "errors": None,
+    assert user_form["step_id"] == "user"
-        "flow_id": mock.ANY,
+    assert user_form["type"] == "form"
-        "handler": DOMAIN,
-        "step_id": "user",
-        "type": "form",
-    }
-    assert expected == user_form
 
 
 async def test_user_form_repeat_identifier(hass, user_form):
@@ -96,13 +95,19 @@ async def test_user_form_pin_not_required(hass, user_form):
     with patch(MOCK_API_CONNECT, return_value=True,) as mock_connect, patch(
         MOCK_API_IS_PIN_REQUIRED,
         return_value=False,
-    ) as mock_is_pin_required:
+    ) as mock_is_pin_required, patch(
+        ASYNC_SETUP, return_value=True
+    ) as mock_setup, patch(
+        ASYNC_SETUP_ENTRY, return_value=True
+    ) as mock_setup_entry:
         result = await hass.config_entries.flow.async_configure(
             user_form["flow_id"],
             TEST_CREDS,
         )
-    assert len(mock_connect.mock_calls) == 2
+    assert len(mock_connect.mock_calls) == 1
     assert len(mock_is_pin_required.mock_calls) == 1
+    assert len(mock_setup.mock_calls) == 1
+    assert len(mock_setup_entry.mock_calls) == 1
 
     expected = {
         "title": TEST_USERNAME,
@@ -117,7 +122,7 @@ async def test_user_form_pin_not_required(hass, user_form):
     }
     expected["data"][CONF_PIN] = None
     result["data"][CONF_DEVICE_ID] = TEST_DEVICE_ID
-    assert expected == result
+    assert result == expected
 
 
 async def test_pin_form_init(pin_form):
@@ -131,7 +136,7 @@ async def test_pin_form_init(pin_form):
         "step_id": "pin",
         "type": "form",
     }
-    assert expected == pin_form
+    assert pin_form == expected
 
 
 async def test_pin_form_bad_pin_format(hass, pin_form):
@@ -154,13 +159,19 @@ async def test_pin_form_success(hass, pin_form):
     with patch(MOCK_API_TEST_PIN, return_value=True,) as mock_test_pin, patch(
         MOCK_API_UPDATE_SAVED_PIN,
         return_value=True,
-    ) as mock_update_saved_pin:
+    ) as mock_update_saved_pin, patch(
+        ASYNC_SETUP, return_value=True
+    ) as mock_setup, patch(
+        ASYNC_SETUP_ENTRY, return_value=True
+    ) as mock_setup_entry:
         result = await hass.config_entries.flow.async_configure(
             pin_form["flow_id"], user_input={CONF_PIN: TEST_PIN}
         )
 
     assert len(mock_test_pin.mock_calls) == 1
     assert len(mock_update_saved_pin.mock_calls) == 1
+    assert len(mock_setup.mock_calls) == 1
+    assert len(mock_setup_entry.mock_calls) == 1
     expected = {
         "title": TEST_USERNAME,
         "description": None,
@@ -196,16 +207,10 @@ async def test_pin_form_incorrect_pin(hass, pin_form):
 
 async def test_option_flow_form(options_form):
     """Test config flow options form."""
-    expected = {
+    assert options_form["description_placeholders"] is None
-        "data_schema": mock.ANY,
+    assert options_form["errors"] is None
-        "description_placeholders": None,
+    assert options_form["step_id"] == "init"
-        "errors": None,
+    assert options_form["type"] == "form"
-        "flow_id": mock.ANY,
-        "handler": mock.ANY,
-        "step_id": "init",
-        "type": "form",
-    }
-    assert expected == options_form
 
 
 async def test_option_flow(hass, options_form):
@@ -247,4 +252,5 @@ async def options_form(hass):
     """Return options form for Subaru config flow."""
     entry = MockConfigEntry(domain=DOMAIN, data={}, options=None)
     entry.add_to_hass(hass)
+    await async_setup_component(hass, DOMAIN, {})
     return await hass.config_entries.options.async_init(entry.entry_id)

tests/components/subaru/test_sensor.py

@@ -1,4 +1,6 @@
 """Test Subaru sensors."""
+from unittest.mock import patch
+
 from homeassistant.components.subaru.const import VEHICLE_NAME
 from homeassistant.components.subaru.sensor import (
     API_GEN_2_SENSORS,
@@ -19,20 +21,25 @@ from .api_responses import (
     VEHICLE_STATUS_EV,
 )
 
-from tests.components.subaru.conftest import setup_subaru_integration
+from tests.components.subaru.conftest import (
+    MOCK_API_FETCH,
+    MOCK_API_GET_DATA,
+    advance_time_to_next_fetch,
+)
 
 VEHICLE_NAME = VEHICLE_DATA[TEST_VIN_2_EV][VEHICLE_NAME]
 
 
-async def test_sensors_ev_imperial(hass):
+async def test_sensors_ev_imperial(hass, ev_entry):
     """Test sensors supporting imperial units."""
     hass.config.units = IMPERIAL_SYSTEM
-    await setup_subaru_integration(
+
-        hass,
+    with patch(MOCK_API_FETCH), patch(
-        vehicle_list=[TEST_VIN_2_EV],
+        MOCK_API_GET_DATA, return_value=VEHICLE_STATUS_EV
-        vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV],
+    ):
-        vehicle_status=VEHICLE_STATUS_EV,
+        advance_time_to_next_fetch(hass)
-    )
+        await hass.async_block_till_done()
+
     _assert_data(hass, EXPECTED_STATE_EV_IMPERIAL)
 
 
@@ -41,14 +48,12 @@ async def test_sensors_ev_metric(hass, ev_entry):
     _assert_data(hass, EXPECTED_STATE_EV_METRIC)
 
 
-async def test_sensors_missing_vin_data(hass):
+async def test_sensors_missing_vin_data(hass, ev_entry):
     """Test for missing VIN dataset."""
-    await setup_subaru_integration(
+    with patch(MOCK_API_FETCH), patch(MOCK_API_GET_DATA, return_value=None):
-        hass,
+        advance_time_to_next_fetch(hass)
-        vehicle_list=[TEST_VIN_2_EV],
+        await hass.async_block_till_done()
-        vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV],
+
-        vehicle_status=None,
-    )
     _assert_data(hass, EXPECTED_STATE_EV_UNAVAILABLE)