Improve the discovery process for Gree (#45449)

* Add support for async device discovery

* FIx missing dispatcher cleanup breaking integration reload

* Update homeassistant/components/gree/climate.py

Co-authored-by: Erik Montnemery <erik@montnemery.com>

* Update homeassistant/components/gree/switch.py

Co-authored-by: Erik Montnemery <erik@montnemery.com>

* Update homeassistant/components/gree/bridge.py

Co-authored-by: Erik Montnemery <erik@montnemery.com>

* Working on feedback

* Improving load/unload tests

* Update homeassistant/components/gree/__init__.py

Co-authored-by: Erik Montnemery <erik@montnemery.com>

* Working on more feedback

* Add tests covering async discovery scenarios

* Remove unnecessary shutdown

* Update homeassistant/components/gree/__init__.py

Co-authored-by: Erik Montnemery <erik@montnemery.com>

* Code refactor from reviews

Co-authored-by: Erik Montnemery <erik@montnemery.com>

tests/components/gree/common.py

@@ -1,6 +1,43 @@
 """Common helpers for gree test cases."""
+import asyncio
+import logging
 from unittest.mock import AsyncMock, Mock
 
+from greeclimate.discovery import Listener
+
+from homeassistant.components.gree.const import DISCOVERY_TIMEOUT
+
+_LOGGER = logging.getLogger(__name__)
+
+
+class FakeDiscovery:
+    """Mock class replacing Gree device discovery."""
+
+    def __init__(self, timeout: int = DISCOVERY_TIMEOUT) -> None:
+        """Initialize the class."""
+        self.mock_devices = [build_device_mock()]
+        self.timeout = timeout
+        self._listeners = []
+        self.scan_count = 0
+
+    def add_listener(self, listener: Listener) -> None:
+        """Add an event listener."""
+        self._listeners.append(listener)
+
+    async def scan(self, wait_for: int = 0):
+        """Search for devices, return mocked data."""
+        self.scan_count += 1
+        _LOGGER.info("CALLED SCAN %d TIMES", self.scan_count)
+
+        infos = [x.device_info for x in self.mock_devices]
+        for listener in self._listeners:
+            [await listener.device_found(x) for x in infos]
+
+        if wait_for:
+            await asyncio.sleep(wait_for)
+
+        return infos
+
 
 def build_device_info_mock(
     name="fake-device-1", ipAddress="1.1.1.1", mac="aabbcc112233"

tests/components/gree/conftest.py

@@ -1,36 +1,24 @@
 """Pytest module configuration."""
-from unittest.mock import AsyncMock, patch
+from unittest.mock import patch
 
 import pytest
 
-from .common import build_device_info_mock, build_device_mock
+from .common import FakeDiscovery, build_device_mock
 
 
-@pytest.fixture(name="discovery")
+@pytest.fixture(autouse=True, name="discovery")
 def discovery_fixture():
-    """Patch the discovery service."""
-    with patch(
-        "homeassistant.components.gree.bridge.Discovery.search_devices",
-        new_callable=AsyncMock,
-        return_value=[build_device_info_mock()],
-    ) as mock:
+    """Patch the discovery object."""
+    with patch("homeassistant.components.gree.bridge.Discovery") as mock:
+        mock.return_value = FakeDiscovery()
         yield mock
 
 
-@pytest.fixture(name="device")
+@pytest.fixture(autouse=True, name="device")
 def device_fixture():
-    """Path the device search and bind."""
+    """Patch the device search and bind."""
     with patch(
         "homeassistant.components.gree.bridge.Device",
         return_value=build_device_mock(),
     ) as mock:
         yield mock
-
-
-@pytest.fixture(name="setup")
-def setup_fixture():
-    """Patch the climate setup."""
-    with patch(
-        "homeassistant.components.gree.climate.async_setup_entry", return_value=True
-    ) as setup:
-        yield setup

tests/components/gree/test_climate.py

@@ -97,7 +97,7 @@ async def test_discovery_setup(hass, discovery, device):
         name="fake-device-2", ipAddress="2.2.2.2", mac="bbccdd223344"
     )
 
-    discovery.return_value = [MockDevice1.device_info, MockDevice2.device_info]
+    discovery.return_value.mock_devices = [MockDevice1, MockDevice2]
     device.side_effect = [MockDevice1, MockDevice2]
 
     await async_setup_gree(hass)
@@ -106,24 +106,127 @@ async def test_discovery_setup(hass, discovery, device):
     assert len(hass.states.async_all(DOMAIN)) == 2
 
 
-async def test_discovery_setup_connection_error(hass, discovery, device):
+async def test_discovery_setup_connection_error(hass, discovery, device, mock_now):
     """Test gree integration is setup."""
-    MockDevice1 = build_device_mock(name="fake-device-1")
+    MockDevice1 = build_device_mock(
+        name="fake-device-1", ipAddress="1.1.1.1", mac="aabbcc112233"
+    )
+    MockDevice1.bind = AsyncMock(side_effect=DeviceNotBoundError)
+    MockDevice1.update_state = AsyncMock(side_effect=DeviceNotBoundError)
+
+    discovery.return_value.mock_devices = [MockDevice1]
+    device.return_value = MockDevice1
+
+    await async_setup_gree(hass)
+    await hass.async_block_till_done()
+
+    assert len(hass.states.async_all(DOMAIN)) == 1
+    state = hass.states.get(ENTITY_ID)
+    assert state.name == "fake-device-1"
+    assert state.state == STATE_UNAVAILABLE
+
+
+async def test_discovery_after_setup(hass, discovery, device, mock_now):
+    """Test gree devices don't change after multiple discoveries."""
+    MockDevice1 = build_device_mock(
+        name="fake-device-1", ipAddress="1.1.1.1", mac="aabbcc112233"
+    )
     MockDevice1.bind = AsyncMock(side_effect=DeviceNotBoundError)
 
-    MockDevice2 = build_device_mock(name="fake-device-2")
-    MockDevice2.bind = AsyncMock(side_effect=DeviceNotBoundError)
+    MockDevice2 = build_device_mock(
+        name="fake-device-2", ipAddress="2.2.2.2", mac="bbccdd223344"
+    )
+    MockDevice2.bind = AsyncMock(side_effect=DeviceTimeoutError)
 
+    discovery.return_value.mock_devices = [MockDevice1, MockDevice2]
     device.side_effect = [MockDevice1, MockDevice2]
 
     await async_setup_gree(hass)
     await hass.async_block_till_done()
-    assert discovery.call_count == 1
 
-    assert not hass.states.async_all(DOMAIN)
+    assert discovery.return_value.scan_count == 1
+    assert len(hass.states.async_all(DOMAIN)) == 2
+
+    # rediscover the same devices shouldn't change anything
+    discovery.return_value.mock_devices = [MockDevice1, MockDevice2]
+    device.side_effect = [MockDevice1, MockDevice2]
+
+    next_update = mock_now + timedelta(minutes=6)
+    with patch("homeassistant.util.dt.utcnow", return_value=next_update):
+        async_fire_time_changed(hass, next_update)
+    await hass.async_block_till_done()
+
+    assert discovery.return_value.scan_count == 2
+    assert len(hass.states.async_all(DOMAIN)) == 2
 
 
-async def test_update_connection_failure(hass, discovery, device, mock_now):
+async def test_discovery_add_device_after_setup(hass, discovery, device, mock_now):
+    """Test gree devices can be added after initial setup."""
+    MockDevice1 = build_device_mock(
+        name="fake-device-1", ipAddress="1.1.1.1", mac="aabbcc112233"
+    )
+    MockDevice1.bind = AsyncMock(side_effect=DeviceNotBoundError)
+
+    MockDevice2 = build_device_mock(
+        name="fake-device-2", ipAddress="2.2.2.2", mac="bbccdd223344"
+    )
+    MockDevice2.bind = AsyncMock(side_effect=DeviceTimeoutError)
+
+    discovery.return_value.mock_devices = [MockDevice1]
+    device.side_effect = [MockDevice1]
+
+    await async_setup_gree(hass)
+    await hass.async_block_till_done()
+
+    assert discovery.return_value.scan_count == 1
+    assert len(hass.states.async_all(DOMAIN)) == 1
+
+    # rediscover the same devices shouldn't change anything
+    discovery.return_value.mock_devices = [MockDevice2]
+    device.side_effect = [MockDevice2]
+
+    next_update = mock_now + timedelta(minutes=6)
+    with patch("homeassistant.util.dt.utcnow", return_value=next_update):
+        async_fire_time_changed(hass, next_update)
+    await hass.async_block_till_done()
+
+    assert discovery.return_value.scan_count == 2
+    assert len(hass.states.async_all(DOMAIN)) == 2
+
+
+async def test_discovery_device_bind_after_setup(hass, discovery, device, mock_now):
+    """Test gree devices can be added after a late device bind."""
+    MockDevice1 = build_device_mock(
+        name="fake-device-1", ipAddress="1.1.1.1", mac="aabbcc112233"
+    )
+    MockDevice1.bind = AsyncMock(side_effect=DeviceNotBoundError)
+    MockDevice1.update_state = AsyncMock(side_effect=DeviceNotBoundError)
+
+    discovery.return_value.mock_devices = [MockDevice1]
+    device.return_value = MockDevice1
+
+    await async_setup_gree(hass)
+    await hass.async_block_till_done()
+
+    assert len(hass.states.async_all(DOMAIN)) == 1
+    state = hass.states.get(ENTITY_ID)
+    assert state.name == "fake-device-1"
+    assert state.state == STATE_UNAVAILABLE
+
+    # Now the device becomes available
+    MockDevice1.bind.side_effect = None
+    MockDevice1.update_state.side_effect = None
+
+    next_update = mock_now + timedelta(minutes=5)
+    with patch("homeassistant.util.dt.utcnow", return_value=next_update):
+        async_fire_time_changed(hass, next_update)
+    await hass.async_block_till_done()
+
+    state = hass.states.get(ENTITY_ID)
+    assert state.state != STATE_UNAVAILABLE
+
+
+async def test_update_connection_failure(hass, device, mock_now):
     """Testing update hvac connection failure exception."""
     device().update_state.side_effect = [
         DEFAULT_MOCK,
@@ -229,11 +332,10 @@ async def test_send_command_device_timeout(hass, discovery, device, mock_now):
     # Send failure should not raise exceptions or change device state
     assert await hass.services.async_call(
         DOMAIN,
-        SERVICE_SET_HVAC_MODE,
-        {ATTR_ENTITY_ID: ENTITY_ID, ATTR_HVAC_MODE: HVAC_MODE_AUTO},
+        SERVICE_TURN_ON,
+        {ATTR_ENTITY_ID: ENTITY_ID},
         blocking=True,
     )
-    await hass.async_block_till_done()
 
     state = hass.states.get(ENTITY_ID)
     assert state is not None
@@ -244,45 +346,6 @@ async def test_send_power_on(hass, discovery, device, mock_now):
     """Test for sending power on command to the device."""
     await async_setup_gree(hass)
 
-    assert await hass.services.async_call(
-        DOMAIN,
-        SERVICE_TURN_ON,
-        {ATTR_ENTITY_ID: ENTITY_ID},
-        blocking=True,
-    )
-
-    state = hass.states.get(ENTITY_ID)
-    assert state is not None
-    assert state.state != HVAC_MODE_OFF
-
-
-async def test_send_power_on_device_timeout(hass, discovery, device, mock_now):
-    """Test for sending power on command to the device with a device timeout."""
-    device().push_state_update.side_effect = DeviceTimeoutError
-
-    await async_setup_gree(hass)
-
-    assert await hass.services.async_call(
-        DOMAIN,
-        SERVICE_TURN_ON,
-        {ATTR_ENTITY_ID: ENTITY_ID},
-        blocking=True,
-    )
-
-    state = hass.states.get(ENTITY_ID)
-    assert state is not None
-    assert state.state != HVAC_MODE_OFF
-
-
-async def test_send_power_off(hass, discovery, device, mock_now):
-    """Test for sending power off command to the device."""
-    await async_setup_gree(hass)
-
-    next_update = mock_now + timedelta(minutes=5)
-    with patch("homeassistant.util.dt.utcnow", return_value=next_update):
-        async_fire_time_changed(hass, next_update)
-    await hass.async_block_till_done()
-
     assert await hass.services.async_call(
         DOMAIN,
         SERVICE_TURN_OFF,
@@ -301,11 +364,6 @@ async def test_send_power_off_device_timeout(hass, discovery, device, mock_now):
 
     await async_setup_gree(hass)
 
-    next_update = mock_now + timedelta(minutes=5)
-    with patch("homeassistant.util.dt.utcnow", return_value=next_update):
-        async_fire_time_changed(hass, next_update)
-    await hass.async_block_till_done()
-
     assert await hass.services.async_call(
         DOMAIN,
         SERVICE_TURN_OFF,

tests/components/gree/test_config_flow.py

@@ -1,20 +1,60 @@
 """Tests for the Gree Integration."""
+from unittest.mock import patch
+
 from homeassistant import config_entries, data_entry_flow
 from homeassistant.components.gree.const import DOMAIN as GREE_DOMAIN
 
+from .common import FakeDiscovery
 
-async def test_creating_entry_sets_up_climate(hass, discovery, device, setup):
+
+async def test_creating_entry_sets_up_climate(hass):
     """Test setting up Gree creates the climate components."""
-    result = await hass.config_entries.flow.async_init(
-        GREE_DOMAIN, context={"source": config_entries.SOURCE_USER}
-    )
+    with patch(
+        "homeassistant.components.gree.climate.async_setup_entry", return_value=True
+    ) as setup, patch(
+        "homeassistant.components.gree.bridge.Discovery", return_value=FakeDiscovery()
+    ), patch(
+        "homeassistant.components.gree.config_flow.Discovery",
+        return_value=FakeDiscovery(),
+    ):
+        result = await hass.config_entries.flow.async_init(
+            GREE_DOMAIN, context={"source": config_entries.SOURCE_USER}
+        )
 
-    # Confirmation form
-    assert result["type"] == data_entry_flow.RESULT_TYPE_FORM
+        # Confirmation form
+        assert result["type"] == data_entry_flow.RESULT_TYPE_FORM
 
-    result = await hass.config_entries.flow.async_configure(result["flow_id"], {})
-    assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY
+        result = await hass.config_entries.flow.async_configure(result["flow_id"], {})
+        assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY
 
-    await hass.async_block_till_done()
+        await hass.async_block_till_done()
 
-    assert len(setup.mock_calls) == 1
+        assert len(setup.mock_calls) == 1
+
+
+async def test_creating_entry_has_no_devices(hass):
+    """Test setting up Gree creates the climate components."""
+    with patch(
+        "homeassistant.components.gree.climate.async_setup_entry", return_value=True
+    ) as setup, patch(
+        "homeassistant.components.gree.bridge.Discovery", return_value=FakeDiscovery()
+    ) as discovery, patch(
+        "homeassistant.components.gree.config_flow.Discovery",
+        return_value=FakeDiscovery(),
+    ) as discovery2:
+        discovery.return_value.mock_devices = []
+        discovery2.return_value.mock_devices = []
+
+        result = await hass.config_entries.flow.async_init(
+            GREE_DOMAIN, context={"source": config_entries.SOURCE_USER}
+        )
+
+        # Confirmation form
+        assert result["type"] == data_entry_flow.RESULT_TYPE_FORM
+
+        result = await hass.config_entries.flow.async_configure(result["flow_id"], {})
+        assert result["type"] == data_entry_flow.RESULT_TYPE_ABORT
+
+        await hass.async_block_till_done()
+
+        assert len(setup.mock_calls) == 0

tests/components/gree/test_init.py

@@ -1,5 +1,4 @@
 """Tests for the Gree Integration."""
-
 from unittest.mock import patch
 
 from homeassistant.components.gree.const import DOMAIN as GREE_DOMAIN
@@ -9,31 +8,39 @@ from homeassistant.setup import async_setup_component
 from tests.common import MockConfigEntry
 
 
-async def test_setup_simple(hass, discovery, device):
+async def test_setup_simple(hass):
     """Test gree integration is setup."""
-    await async_setup_component(hass, GREE_DOMAIN, {})
-    await hass.async_block_till_done()
-
-    # No flows started
-    assert len(hass.config_entries.flow.async_progress()) == 0
-
-
-async def test_unload_config_entry(hass, discovery, device):
-    """Test that the async_unload_entry works."""
-    # As we have currently no configuration, we just to pass the domain here.
     entry = MockConfigEntry(domain=GREE_DOMAIN)
     entry.add_to_hass(hass)
 
     with patch(
         "homeassistant.components.gree.climate.async_setup_entry",
         return_value=True,
-    ) as climate_setup:
+    ) as climate_setup, patch(
+        "homeassistant.components.gree.switch.async_setup_entry",
+        return_value=True,
+    ) as switch_setup:
         assert await async_setup_component(hass, GREE_DOMAIN, {})
         await hass.async_block_till_done()
 
         assert len(climate_setup.mock_calls) == 1
+        assert len(switch_setup.mock_calls) == 1
         assert entry.state == ENTRY_STATE_LOADED
 
+    # No flows started
+    assert len(hass.config_entries.flow.async_progress()) == 0
+
+
+async def test_unload_config_entry(hass):
+    """Test that the async_unload_entry works."""
+    # As we have currently no configuration, we just to pass the domain here.
+    entry = MockConfigEntry(domain=GREE_DOMAIN)
+    entry.add_to_hass(hass)
+
+    assert await async_setup_component(hass, GREE_DOMAIN, {})
+    await hass.async_block_till_done()
+
     await hass.config_entries.async_unload(entry.entry_id)
+    await hass.async_block_till_done()
 
     assert entry.state == ENTRY_STATE_NOT_LOADED

tests/components/gree/test_switch.py

@@ -26,7 +26,7 @@ async def async_setup_gree(hass):
     await hass.async_block_till_done()
 
 
-async def test_send_panel_light_on(hass, discovery, device):
+async def test_send_panel_light_on(hass):
     """Test for sending power on command to the device."""
     await async_setup_gree(hass)
 
@@ -42,7 +42,7 @@ async def test_send_panel_light_on(hass, discovery, device):
     assert state.state == STATE_ON
 
 
-async def test_send_panel_light_on_device_timeout(hass, discovery, device):
+async def test_send_panel_light_on_device_timeout(hass, device):
     """Test for sending power on command to the device with a device timeout."""
     device().push_state_update.side_effect = DeviceTimeoutError
 
@@ -60,7 +60,7 @@ async def test_send_panel_light_on_device_timeout(hass, discovery, device):
     assert state.state == STATE_ON
 
 
-async def test_send_panel_light_off(hass, discovery, device):
+async def test_send_panel_light_off(hass):
     """Test for sending power on command to the device."""
     await async_setup_gree(hass)
 
@@ -76,7 +76,7 @@ async def test_send_panel_light_off(hass, discovery, device):
     assert state.state == STATE_OFF
 
 
-async def test_send_panel_light_toggle(hass, discovery, device):
+async def test_send_panel_light_toggle(hass):
     """Test for sending power on command to the device."""
     await async_setup_gree(hass)
 
@@ -117,7 +117,7 @@ async def test_send_panel_light_toggle(hass, discovery, device):
     assert state.state == STATE_ON
 
 
-async def test_panel_light_name(hass, discovery, device):
+async def test_panel_light_name(hass):
     """Test for name property."""
     await async_setup_gree(hass)
     state = hass.states.get(ENTITY_ID)