From a019f076c036e1405d7a0d605c7c182bea4015da Mon Sep 17 00:00:00 2001 From: Garrett <7310260+G-Two@users.noreply.github.com> Date: Thu, 25 Mar 2021 23:24:37 -0400 Subject: [PATCH] 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 Co-authored-by: Martin Hjelmare Co-authored-by: Brandon Rothweiler Co-authored-by: Martin Hjelmare --- .../components/subaru/config_flow.py | 10 ++-- homeassistant/components/subaru/const.py | 5 -- homeassistant/components/subaru/entity.py | 7 +-- homeassistant/components/subaru/sensor.py | 24 +++++--- homeassistant/components/subaru/strings.json | 3 +- tests/components/subaru/conftest.py | 15 ++++- tests/components/subaru/test_config_flow.py | 56 ++++++++++--------- tests/components/subaru/test_sensor.py | 35 +++++++----- 8 files changed, 88 insertions(+), 67 deletions(-) diff --git a/homeassistant/components/subaru/config_flow.py b/homeassistant/components/subaru/config_flow.py index a3586b32974..772134c66b1 100644 --- a/homeassistant/components/subaru/config_flow.py +++ b/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") diff --git a/homeassistant/components/subaru/const.py b/homeassistant/components/subaru/const.py index fa6fe984e61..cada29edd3a 100644 --- a/homeassistant/components/subaru/const.py +++ b/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", } diff --git a/homeassistant/components/subaru/entity.py b/homeassistant/components/subaru/entity.py index 4fdeca4e484..559feeea303 100644 --- a/homeassistant/components/subaru/entity.py +++ b/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.""" diff --git a/homeassistant/components/subaru/sensor.py b/homeassistant/components/subaru/sensor.py index b8362202a3d..41dd8a6604f 100644 --- a/homeassistant/components/subaru/sensor.py +++ b/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): diff --git a/homeassistant/components/subaru/strings.json b/homeassistant/components/subaru/strings.json index 064245e0732..ea9df082f3a 100644 --- a/homeassistant/components/subaru/strings.json +++ b/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", - "unknown": "[%key:common::config_flow::error::unknown%]" + "bad_pin_format": "PIN should be 4 digits" }, "abort": { "already_configured": "[%key:common::config_flow::abort::already_configured_account%]", diff --git a/tests/components/subaru/conftest.py b/tests/components/subaru/conftest.py index 8216ca2d2c2..1b8d1439e68 100644 --- a/tests/components/subaru/conftest.py +++ b/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, diff --git a/tests/components/subaru/test_config_flow.py b/tests/components/subaru/test_config_flow.py index 676b876652b..0218c11003c 100644 --- a/tests/components/subaru/test_config_flow.py +++ b/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 = { - "data_schema": mock.ANY, - "description_placeholders": None, - "errors": None, - "flow_id": mock.ANY, - "handler": DOMAIN, - "step_id": "user", - "type": "form", - } - assert expected == user_form + assert user_form["description_placeholders"] is None + assert user_form["errors"] is None + assert user_form["handler"] == DOMAIN + assert user_form["step_id"] == "user" + assert user_form["type"] == "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 = { - "data_schema": mock.ANY, - "description_placeholders": None, - "errors": None, - "flow_id": mock.ANY, - "handler": mock.ANY, - "step_id": "init", - "type": "form", - } - assert expected == options_form + assert options_form["description_placeholders"] is None + assert options_form["errors"] is None + assert options_form["step_id"] == "init" + assert options_form["type"] == "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) diff --git a/tests/components/subaru/test_sensor.py b/tests/components/subaru/test_sensor.py index 4344c147f22..f2a66e7e5e9 100644 --- a/tests/components/subaru/test_sensor.py +++ b/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, - vehicle_list=[TEST_VIN_2_EV], - vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV], - vehicle_status=VEHICLE_STATUS_EV, - ) + + with patch(MOCK_API_FETCH), patch( + MOCK_API_GET_DATA, return_value=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( - hass, - vehicle_list=[TEST_VIN_2_EV], - vehicle_data=VEHICLE_DATA[TEST_VIN_2_EV], - vehicle_status=None, - ) + with patch(MOCK_API_FETCH), patch(MOCK_API_GET_DATA, return_value=None): + advance_time_to_next_fetch(hass) + await hass.async_block_till_done() + _assert_data(hass, EXPECTED_STATE_EV_UNAVAILABLE)