Address late smarttub review (#46703)

* _config -> config

* remove unused string

* remove entity tests

* replace unit tests with integration tests using the core

* refactor polling to use asyncio.gather

* remove redundant component init

* remove gather in favor of simple loop

* use async_fire_time_changed instead of async_update_entity

* use hass.config_entries.async_setup instead of calling smarttub.async_setup_entry directly

* replace stray smarttub.async_setup_entry call

* async_unload_entry -> hass.config_entries.async_unload

* remove broken test

homeassistant/components/smarttub/__init__.py

@@ -10,7 +10,7 @@ _LOGGER = logging.getLogger(__name__)
 PLATFORMS = ["climate"]
 
 
-async def async_setup(hass, _config):
+async def async_setup(hass, config):
     """Set up smarttub component."""
 
     hass.data.setdefault(DOMAIN, {})

homeassistant/components/smarttub/controller.py

@@ -79,12 +79,15 @@ class SmartTubController:
         try:
             async with async_timeout.timeout(POLLING_TIMEOUT):
                 for spa in self.spas:
-                    data[spa.id] = {"status": await spa.get_status()}
+                    data[spa.id] = await self._get_spa_data(spa)
         except APIError as err:
             raise UpdateFailed(err) from err
 
         return data
 
+    async def _get_spa_data(self, spa):
+        return {"status": await spa.get_status()}
+
     async def async_register_devices(self, entry):
         """Register devices with the device registry for all spas."""
         device_registry = await dr.async_get_registry(self._hass)

homeassistant/components/smarttub/strings.json

@@ -11,8 +11,7 @@
       }
     },
     "error": {
-      "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]",
+      "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]"
-      "unknown": "[%key:common::config_flow::error::unknown%]"
     },
     "abort": {
       "already_configured": "[%key:common::config_flow::abort::already_configured_device%]",

homeassistant/components/smarttub/translations/en.json

@@ -5,8 +5,7 @@
             "reauth_successful": "Re-authentication was successful"
         },
         "error": {
-            "invalid_auth": "Invalid authentication",
+            "invalid_auth": "Invalid authentication"
-            "unknown": "Unexpected error"
         },
         "step": {
             "user": {

tests/components/smarttub/conftest.py

@@ -6,8 +6,8 @@ import pytest
 import smarttub
 
 from homeassistant.components.smarttub.const import DOMAIN
-from homeassistant.components.smarttub.controller import SmartTubController
 from homeassistant.const import CONF_EMAIL, CONF_PASSWORD
+from homeassistant.setup import async_setup_component
 
 from tests.common import MockConfigEntry
 
@@ -28,6 +28,12 @@ def config_entry(config_data):
     )
 
 
+@pytest.fixture
+async def setup_component(hass):
+    """Set up the component."""
+    assert await async_setup_component(hass, DOMAIN, {}) is True
+
+
 @pytest.fixture(name="spa")
 def mock_spa():
     """Mock a SmartTub.Spa."""
@@ -65,22 +71,3 @@ def mock_api(account, spa):
         api_mock = api_class_mock.return_value
         api_mock.get_account.return_value = account
         yield api_mock
-
-
-@pytest.fixture
-async def controller(smarttub_api, hass, config_entry):
-    """Instantiate controller for testing."""
-
-    controller = SmartTubController(hass)
-    assert len(controller.spas) == 0
-    assert await controller.async_setup_entry(config_entry)
-
-    assert len(controller.spas) > 0
-
-    return controller
-
-
-@pytest.fixture
-async def coordinator(controller):
-    """Provide convenient access to the coordinator via the controller."""
-    return controller.coordinator

tests/components/smarttub/test_climate.py

@@ -1,5 +1,9 @@
 """Test the SmartTub climate platform."""
 
+from datetime import timedelta
+
+import smarttub
+
 from homeassistant.components.climate.const import (
     ATTR_CURRENT_TEMPERATURE,
     ATTR_HVAC_ACTION,
@@ -15,15 +19,22 @@ from homeassistant.components.climate.const import (
     SERVICE_SET_TEMPERATURE,
     SUPPORT_TARGET_TEMPERATURE,
 )
-from homeassistant.components.smarttub.const import DEFAULT_MAX_TEMP, DEFAULT_MIN_TEMP
+from homeassistant.components.smarttub.const import (
+    DEFAULT_MAX_TEMP,
+    DEFAULT_MIN_TEMP,
+    SCAN_INTERVAL,
+)
 from homeassistant.const import (
     ATTR_ENTITY_ID,
     ATTR_SUPPORTED_FEATURES,
     ATTR_TEMPERATURE,
 )
+from homeassistant.util import dt
+
+from tests.common import async_fire_time_changed
 
 
-async def test_thermostat(coordinator, spa, hass, config_entry):
+async def test_thermostat_update(spa, hass, config_entry, smarttub_api):
     """Test the thermostat entity."""
 
     spa.get_status.return_value = {
@@ -44,7 +55,7 @@ async def test_thermostat(coordinator, spa, hass, config_entry):
     assert state.attributes[ATTR_HVAC_ACTION] == CURRENT_HVAC_HEAT
 
     spa.get_status.return_value["heater"] = "OFF"
-    await hass.helpers.entity_component.async_update_entity(entity_id)
+    await trigger_update(hass)
     state = hass.states.get(entity_id)
 
     assert state.attributes[ATTR_HVAC_ACTION] == CURRENT_HVAC_IDLE
@@ -72,3 +83,14 @@ async def test_thermostat(coordinator, spa, hass, config_entry):
         blocking=True,
     )
     # does nothing
+
+    spa.get_status.side_effect = smarttub.APIError
+    await trigger_update(hass)
+    # should not fail
+
+
+async def trigger_update(hass):
+    """Trigger a polling update by moving time forward."""
+    new_time = dt.utcnow() + timedelta(seconds=SCAN_INTERVAL + 1)
+    async_fire_time_changed(hass, new_time)
+    await hass.async_block_till_done()

tests/components/smarttub/test_controller.py

@@ -1,37 +0,0 @@
-"""Test the SmartTub controller."""
-
-import pytest
-import smarttub
-
-from homeassistant.components.smarttub.controller import SmartTubController
-from homeassistant.helpers.update_coordinator import UpdateFailed
-
-
-async def test_invalid_credentials(hass, controller, smarttub_api, config_entry):
-    """Check that we return False if the configured credentials are invalid.
-
-    This should mean that the user changed their SmartTub password.
-    """
-
-    smarttub_api.login.side_effect = smarttub.LoginFailed
-    controller = SmartTubController(hass)
-    ret = await controller.async_setup_entry(config_entry)
-    assert ret is False
-
-
-async def test_update(controller, spa):
-    """Test data updates from API."""
-    data = await controller.async_update_data()
-    assert data[spa.id] == {"status": spa.get_status.return_value}
-
-    spa.get_status.side_effect = smarttub.APIError
-    with pytest.raises(UpdateFailed):
-        data = await controller.async_update_data()
-
-
-async def test_login(controller, smarttub_api, account):
-    """Test SmartTubController.login."""
-    smarttub_api.get_account.return_value.id = "account-id1"
-    account = await controller.login("test-email1", "test-password1")
-    smarttub_api.login.assert_called()
-    assert account == account

tests/components/smarttub/test_entity.py

@@ -1,18 +0,0 @@
-"""Test SmartTubEntity."""
-
-from homeassistant.components.smarttub.entity import SmartTubEntity
-
-
-async def test_entity(coordinator, spa):
-    """Test SmartTubEntity."""
-
-    entity = SmartTubEntity(coordinator, spa, "entity1")
-
-    assert entity.device_info
-    assert entity.name
-
-    coordinator.data[spa.id] = {}
-    assert entity.get_spa_status("foo") is None
-    coordinator.data[spa.id]["status"] = {"foo": "foo1", "bar": {"baz": "barbaz1"}}
-    assert entity.get_spa_status("foo") == "foo1"
-    assert entity.get_spa_status("bar.baz") == "barbaz1"

tests/components/smarttub/test_init.py

@@ -1,48 +1,50 @@
 """Test smarttub setup process."""
 
 import asyncio
-from unittest.mock import patch
 
-import pytest
 from smarttub import LoginFailed
 
 from homeassistant.components import smarttub
-from homeassistant.exceptions import ConfigEntryNotReady
+from homeassistant.config_entries import (
+    ENTRY_STATE_SETUP_ERROR,
+    ENTRY_STATE_SETUP_RETRY,
+)
 from homeassistant.setup import async_setup_component
 
 
-async def test_setup_with_no_config(hass):
+async def test_setup_with_no_config(setup_component, hass, smarttub_api):
     """Test that we do not discover anything."""
-    assert await async_setup_component(hass, smarttub.DOMAIN, {}) is True
 
     # No flows started
     assert len(hass.config_entries.flow.async_progress()) == 0
 
-    assert smarttub.const.SMARTTUB_CONTROLLER not in hass.data[smarttub.DOMAIN]
+    smarttub_api.login.assert_not_called()
 
 
-async def test_setup_entry_not_ready(hass, config_entry, smarttub_api):
+async def test_setup_entry_not_ready(setup_component, hass, config_entry, smarttub_api):
     """Test setup when the entry is not ready."""
-    assert await async_setup_component(hass, smarttub.DOMAIN, {}) is True
     smarttub_api.login.side_effect = asyncio.TimeoutError
 
-    with pytest.raises(ConfigEntryNotReady):
+    config_entry.add_to_hass(hass)
-        await smarttub.async_setup_entry(hass, config_entry)
+    await hass.config_entries.async_setup(config_entry.entry_id)
+    assert config_entry.state == ENTRY_STATE_SETUP_RETRY
 
 
-async def test_setup_auth_failed(hass, config_entry, smarttub_api):
+async def test_setup_auth_failed(setup_component, hass, config_entry, smarttub_api):
     """Test setup when the credentials are invalid."""
-    assert await async_setup_component(hass, smarttub.DOMAIN, {}) is True
     smarttub_api.login.side_effect = LoginFailed
 
-    assert await smarttub.async_setup_entry(hass, config_entry) is False
+    config_entry.add_to_hass(hass)
+    await hass.config_entries.async_setup(config_entry.entry_id)
+    assert config_entry.state == ENTRY_STATE_SETUP_ERROR
 
 
-async def test_config_passed_to_config_entry(hass, config_entry, config_data):
+async def test_config_passed_to_config_entry(
+    hass, config_entry, config_data, smarttub_api
+):
     """Test that configured options are loaded via config entry."""
     config_entry.add_to_hass(hass)
-    ret = await async_setup_component(hass, smarttub.DOMAIN, config_data)
+    assert await async_setup_component(hass, smarttub.DOMAIN, config_data)
-    assert ret is True
 
 
 async def test_unload_entry(hass, config_entry, smarttub_api):
@@ -51,10 +53,4 @@ async def test_unload_entry(hass, config_entry, smarttub_api):
 
     assert await async_setup_component(hass, smarttub.DOMAIN, {}) is True
 
-    assert await smarttub.async_unload_entry(hass, config_entry)
+    assert await hass.config_entries.async_unload(config_entry.entry_id)
-
-    # test failure of platform unload
-    assert await async_setup_component(hass, smarttub.DOMAIN, {}) is True
-    with patch.object(hass.config_entries, "async_forward_entry_unload") as mock:
-        mock.return_value = False
-        assert await smarttub.async_unload_entry(hass, config_entry) is False