From 2e36a79357e09c3cdede1085c8ce5308f89a42f2 Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Tue, 24 May 2022 21:37:37 +0200 Subject: [PATCH] Changes after late upnp review (#72241) * Changes after review of #70008, part 1 * Changes after review from #70008 * Revert to UpnpDataUpdateCoordinator._async_update_data --- homeassistant/components/upnp/__init__.py | 27 ++- homeassistant/components/upnp/config_flow.py | 2 +- homeassistant/components/upnp/device.py | 51 ++--- tests/components/upnp/conftest.py | 196 +++---------------- tests/components/upnp/test_binary_sensor.py | 34 ++-- tests/components/upnp/test_sensor.py | 141 ++++++------- 6 files changed, 132 insertions(+), 319 deletions(-) diff --git a/homeassistant/components/upnp/__init__.py b/homeassistant/components/upnp/__init__.py index b571a2b447f..07560f7413f 100644 --- a/homeassistant/components/upnp/__init__.py +++ b/homeassistant/components/upnp/__init__.py @@ -39,7 +39,7 @@ from .const import ( DOMAIN, LOGGER, ) -from .device import Device, async_get_mac_address_from_host +from .device import Device, async_create_device, async_get_mac_address_from_host NOTIFICATION_ID = "upnp_notification" NOTIFICATION_TITLE = "UPnP/IGD Setup" @@ -113,8 +113,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: try: await asyncio.wait_for(device_discovered_event.wait(), timeout=10) except asyncio.TimeoutError as err: - LOGGER.debug("Device not discovered: %s", usn) - raise ConfigEntryNotReady from err + raise ConfigEntryNotReady(f"Device not discovered: {usn}") from err finally: cancel_discovered_callback() @@ -123,12 +122,11 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: assert discovery_info.ssdp_location is not None location = discovery_info.ssdp_location try: - device = await Device.async_create_device(hass, location) + device = await async_create_device(hass, location) except UpnpConnectionError as err: - LOGGER.debug( - "Error connecting to device at location: %s, err: %s", location, err - ) - raise ConfigEntryNotReady from err + raise ConfigEntryNotReady( + f"Error connecting to device at location: {location}, err: {err}" + ) from err # Track the original UDN such that existing sensors do not change their unique_id. if CONFIG_ENTRY_ORIGINAL_UDN not in entry.data: @@ -255,21 +253,15 @@ class UpnpDataUpdateCoordinator(DataUpdateCoordinator): LOGGER, name=device.name, update_interval=update_interval, - update_method=self._async_fetch_data, ) - async def _async_fetch_data(self) -> Mapping[str, Any]: + async def _async_update_data(self) -> Mapping[str, Any]: """Update data.""" try: update_values = await asyncio.gather( self.device.async_get_traffic_data(), self.device.async_get_status(), ) - - return { - **update_values[0], - **update_values[1], - } except UpnpCommunicationError as exception: LOGGER.debug( "Caught exception when updating device: %s, exception: %s", @@ -280,6 +272,11 @@ class UpnpDataUpdateCoordinator(DataUpdateCoordinator): f"Unable to communicate with IGD at: {self.device.device_url}" ) from exception + return { + **update_values[0], + **update_values[1], + } + class UpnpEntity(CoordinatorEntity[UpnpDataUpdateCoordinator]): """Base class for UPnP/IGD entities.""" diff --git a/homeassistant/components/upnp/config_flow.py b/homeassistant/components/upnp/config_flow.py index b7208e6e6e2..b54098b6566 100644 --- a/homeassistant/components/upnp/config_flow.py +++ b/homeassistant/components/upnp/config_flow.py @@ -54,7 +54,7 @@ async def _async_wait_for_discoveries(hass: HomeAssistant) -> bool: async def device_discovered(info: SsdpServiceInfo, change: SsdpChange) -> None: if change != SsdpChange.BYEBYE: - LOGGER.info( + LOGGER.debug( "Device discovered: %s, at: %s", info.ssdp_usn, info.ssdp_location, diff --git a/homeassistant/components/upnp/device.py b/homeassistant/components/upnp/device.py index 0e7c7902bd9..334d870939f 100644 --- a/homeassistant/components/upnp/device.py +++ b/homeassistant/components/upnp/device.py @@ -9,7 +9,6 @@ from typing import Any from urllib.parse import urlparse from async_upnp_client.aiohttp import AiohttpSessionRequester -from async_upnp_client.client import UpnpDevice from async_upnp_client.client_factory import UpnpFactory from async_upnp_client.exceptions import UpnpError from async_upnp_client.profiles.igd import IgdDevice, StatusInfo @@ -47,15 +46,19 @@ async def async_get_mac_address_from_host(hass: HomeAssistant, host: str) -> str return mac_address -async def async_create_upnp_device( - hass: HomeAssistant, ssdp_location: str -) -> UpnpDevice: - """Create UPnP device.""" +async def async_create_device(hass: HomeAssistant, ssdp_location: str) -> Device: + """Create UPnP/IGD device.""" session = async_get_clientsession(hass) requester = AiohttpSessionRequester(session, with_sleep=True, timeout=20) factory = UpnpFactory(requester, disable_state_variable_validation=True) - return await factory.async_create_device(ssdp_location) + upnp_device = await factory.async_create_device(ssdp_location) + + # Create profile wrapper. + igd_device = IgdDevice(upnp_device, None) + device = Device(hass, igd_device) + + return device class Device: @@ -66,40 +69,8 @@ class Device: self.hass = hass self._igd_device = igd_device self.coordinator: DataUpdateCoordinator | None = None - self._mac_address: str | None = None - - @classmethod - async def async_create_device( - cls, hass: HomeAssistant, ssdp_location: str - ) -> Device: - """Create UPnP/IGD device.""" - upnp_device = await async_create_upnp_device(hass, ssdp_location) - - # Create profile wrapper. - igd_device = IgdDevice(upnp_device, None) - device = cls(hass, igd_device) - - return device - - @property - def mac_address(self) -> str | None: - """Get the mac address.""" - return self._mac_address - - @mac_address.setter - def mac_address(self, mac_address: str) -> None: - """Set the mac address.""" - self._mac_address = mac_address - - @property - def original_udn(self) -> str | None: - """Get the mac address.""" - return self._original_udn - - @original_udn.setter - def original_udn(self, original_udn: str) -> None: - """Set the original UDN.""" - self._original_udn = original_udn + self.mac_address: str | None = None + self.original_udn: str | None = None @property def udn(self) -> str: diff --git a/tests/components/upnp/conftest.py b/tests/components/upnp/conftest.py index dd22db878cf..687518bb46d 100644 --- a/tests/components/upnp/conftest.py +++ b/tests/components/upnp/conftest.py @@ -1,33 +1,23 @@ """Configuration for SSDP tests.""" from __future__ import annotations -from collections.abc import Sequence -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, create_autospec, patch from urllib.parse import urlparse from async_upnp_client.client import UpnpDevice -from async_upnp_client.event_handler import UpnpEventHandler -from async_upnp_client.profiles.igd import StatusInfo +from async_upnp_client.profiles.igd import IgdDevice, StatusInfo import pytest from homeassistant.components import ssdp from homeassistant.components.upnp.const import ( - BYTES_RECEIVED, - BYTES_SENT, CONFIG_ENTRY_LOCATION, CONFIG_ENTRY_MAC_ADDRESS, CONFIG_ENTRY_ORIGINAL_UDN, CONFIG_ENTRY_ST, CONFIG_ENTRY_UDN, DOMAIN, - PACKETS_RECEIVED, - PACKETS_SENT, - ROUTER_IP, - ROUTER_UPTIME, - WAN_STATUS, ) from homeassistant.core import HomeAssistant -from homeassistant.util import dt from tests.common import MockConfigEntry @@ -59,160 +49,37 @@ TEST_DISCOVERY = ssdp.SsdpServiceInfo( ) -class MockUpnpDevice: - """Mock async_upnp_client UpnpDevice.""" - - def __init__(self, location: str) -> None: - """Initialize.""" - self.device_url = location - - @property - def manufacturer(self) -> str: - """Get manufacturer.""" - return TEST_DISCOVERY.upnp[ssdp.ATTR_UPNP_MANUFACTURER] - - @property - def name(self) -> str: - """Get name.""" - return TEST_DISCOVERY.upnp[ssdp.ATTR_UPNP_FRIENDLY_NAME] - - @property - def model_name(self) -> str: - """Get the model name.""" - return TEST_DISCOVERY.upnp[ssdp.ATTR_UPNP_MODEL_NAME] - - @property - def device_type(self) -> str: - """Get the device type.""" - return TEST_DISCOVERY.upnp[ssdp.ATTR_UPNP_DEVICE_TYPE] - - @property - def udn(self) -> str: - """Get the UDN.""" - return TEST_DISCOVERY.upnp[ssdp.ATTR_UPNP_UDN] - - @property - def usn(self) -> str: - """Get the USN.""" - return f"{self.udn}::{self.device_type}" - - @property - def unique_id(self) -> str: - """Get the unique id.""" - return self.usn - - def reinit(self, new_upnp_device: UpnpDevice) -> None: - """Reinitialize.""" - self.device_url = new_upnp_device.device_url - - -class MockIgdDevice: - """Mock async_upnp_client IgdDevice.""" - - def __init__(self, device: MockUpnpDevice, event_handler: UpnpEventHandler) -> None: - """Initialize mock device.""" - self.device = device - self.profile_device = device - - self._timestamp = dt.utcnow() - self.traffic_times_polled = 0 - self.status_times_polled = 0 - - self.traffic_data = { - BYTES_RECEIVED: 0, - BYTES_SENT: 0, - PACKETS_RECEIVED: 0, - PACKETS_SENT: 0, - } - self.status_data = { - WAN_STATUS: "Connected", - ROUTER_UPTIME: 10, - ROUTER_IP: "8.9.10.11", - } - - @property - def name(self) -> str: - """Get the name of the device.""" - return self.profile_device.name - - @property - def manufacturer(self) -> str: - """Get the manufacturer of this device.""" - return self.profile_device.manufacturer - - @property - def model_name(self) -> str: - """Get the model name of this device.""" - return self.profile_device.model_name - - @property - def udn(self) -> str: - """Get the UDN of the device.""" - return self.profile_device.udn - - @property - def device_type(self) -> str: - """Get the device type of this device.""" - return self.profile_device.device_type - - async def async_get_total_bytes_received(self) -> int | None: - """Get total bytes received.""" - self.traffic_times_polled += 1 - return self.traffic_data[BYTES_RECEIVED] - - async def async_get_total_bytes_sent(self) -> int | None: - """Get total bytes sent.""" - return self.traffic_data[BYTES_SENT] - - async def async_get_total_packets_received(self) -> int | None: - """Get total packets received.""" - return self.traffic_data[PACKETS_RECEIVED] - - async def async_get_total_packets_sent(self) -> int | None: - """Get total packets sent.""" - return self.traffic_data[PACKETS_SENT] - - async def async_get_external_ip_address( - self, services: Sequence[str] | None = None - ) -> str | None: - """ - Get the external IP address. - - :param services List of service names to try to get action from, defaults to [WANIPC,WANPPP] - """ - return self.status_data[ROUTER_IP] - - async def async_get_status_info( - self, services: Sequence[str] | None = None - ) -> StatusInfo | None: - """ - Get status info. - - :param services List of service names to try to get action from, defaults to [WANIPC,WANPPP] - """ - self.status_times_polled += 1 - return StatusInfo( - self.status_data[WAN_STATUS], "", self.status_data[ROUTER_UPTIME] - ) - - @pytest.fixture(autouse=True) -def mock_upnp_device(): - """Mock homeassistant.components.upnp.Device.""" +def mock_igd_device() -> IgdDevice: + """Mock async_upnp_client device.""" + mock_upnp_device = create_autospec(UpnpDevice, instance=True) + mock_upnp_device.device_url = TEST_DISCOVERY.ssdp_location - async def mock_async_create_upnp_device( - hass: HomeAssistant, location: str - ) -> UpnpDevice: - """Create UPnP device.""" - return MockUpnpDevice(location) + mock_igd_device = create_autospec(IgdDevice) + mock_igd_device.name = TEST_DISCOVERY.upnp[ssdp.ATTR_UPNP_FRIENDLY_NAME] + mock_igd_device.manufacturer = TEST_DISCOVERY.upnp[ssdp.ATTR_UPNP_MANUFACTURER] + mock_igd_device.model_name = TEST_DISCOVERY.upnp[ssdp.ATTR_UPNP_MODEL_NAME] + mock_igd_device.udn = TEST_DISCOVERY.ssdp_udn + mock_igd_device.device = mock_upnp_device + + mock_igd_device.async_get_total_bytes_received.return_value = 0 + mock_igd_device.async_get_total_bytes_sent.return_value = 0 + mock_igd_device.async_get_total_packets_received.return_value = 0 + mock_igd_device.async_get_total_packets_sent.return_value = 0 + mock_igd_device.async_get_status_info.return_value = StatusInfo( + "Connected", + "", + 10, + ) + mock_igd_device.async_get_external_ip_address.return_value = "8.9.10.11" with patch( - "homeassistant.components.upnp.device.async_create_upnp_device", - side_effect=mock_async_create_upnp_device, - ) as mock_async_create_upnp_device, patch( - "homeassistant.components.upnp.device.IgdDevice", new=MockIgdDevice - ) as mock_igd_device: - yield mock_async_create_upnp_device, mock_igd_device + "homeassistant.components.upnp.device.UpnpFactory.async_create_device" + ), patch( + "homeassistant.components.upnp.device.IgdDevice.__new__", + return_value=mock_igd_device, + ): + yield mock_igd_device @pytest.fixture @@ -297,11 +164,11 @@ async def ssdp_no_discovery(): @pytest.fixture -async def config_entry( +async def mock_config_entry( hass: HomeAssistant, mock_get_source_ip, ssdp_instant_discovery, - mock_upnp_device, + mock_igd_device: IgdDevice, mock_mac_address_from_host, ): """Create an initialized integration.""" @@ -316,6 +183,7 @@ async def config_entry( CONFIG_ENTRY_MAC_ADDRESS: TEST_MAC_ADDRESS, }, ) + entry.igd_device = mock_igd_device # Load config_entry. entry.add_to_hass(hass) diff --git a/tests/components/upnp/test_binary_sensor.py b/tests/components/upnp/test_binary_sensor.py index 22264792420..24e5cdce47c 100644 --- a/tests/components/upnp/test_binary_sensor.py +++ b/tests/components/upnp/test_binary_sensor.py @@ -2,36 +2,34 @@ from datetime import timedelta -from homeassistant.components.upnp.const import ( - DOMAIN, - ROUTER_IP, - ROUTER_UPTIME, - WAN_STATUS, -) +from async_upnp_client.profiles.igd import StatusInfo + +from homeassistant.components.upnp.const import DEFAULT_SCAN_INTERVAL from homeassistant.core import HomeAssistant import homeassistant.util.dt as dt_util -from .conftest import MockIgdDevice - from tests.common import MockConfigEntry, async_fire_time_changed -async def test_upnp_binary_sensors(hass: HomeAssistant, config_entry: MockConfigEntry): +async def test_upnp_binary_sensors( + hass: HomeAssistant, mock_config_entry: MockConfigEntry +): """Test normal sensors.""" # First poll. wan_status_state = hass.states.get("binary_sensor.mock_name_wan_status") assert wan_status_state.state == "on" # Second poll. - mock_device: MockIgdDevice = hass.data[DOMAIN][ - config_entry.entry_id - ].device._igd_device - mock_device.status_data = { - WAN_STATUS: "Disconnected", - ROUTER_UPTIME: 100, - ROUTER_IP: "", - } - async_fire_time_changed(hass, dt_util.utcnow() + timedelta(seconds=31)) + mock_igd_device = mock_config_entry.igd_device + mock_igd_device.async_get_status_info.return_value = StatusInfo( + "Disconnected", + "", + 40, + ) + + async_fire_time_changed( + hass, dt_util.utcnow() + timedelta(seconds=DEFAULT_SCAN_INTERVAL) + ) await hass.async_block_till_done() wan_status_state = hass.states.get("binary_sensor.mock_name_wan_status") diff --git a/tests/components/upnp/test_sensor.py b/tests/components/upnp/test_sensor.py index 6c00f63a479..2abd357ac31 100644 --- a/tests/components/upnp/test_sensor.py +++ b/tests/components/upnp/test_sensor.py @@ -3,114 +3,93 @@ from datetime import timedelta from unittest.mock import patch +from async_upnp_client.profiles.igd import StatusInfo import pytest -from homeassistant.components.upnp import UpnpDataUpdateCoordinator -from homeassistant.components.upnp.const import ( - BYTES_RECEIVED, - BYTES_SENT, - DEFAULT_SCAN_INTERVAL, - DOMAIN, - PACKETS_RECEIVED, - PACKETS_SENT, - ROUTER_IP, - ROUTER_UPTIME, - WAN_STATUS, -) +from homeassistant.components.upnp.const import DEFAULT_SCAN_INTERVAL from homeassistant.core import HomeAssistant import homeassistant.util.dt as dt_util -from .conftest import MockIgdDevice - from tests.common import MockConfigEntry, async_fire_time_changed -async def test_upnp_sensors(hass: HomeAssistant, config_entry: MockConfigEntry): +async def test_upnp_sensors(hass: HomeAssistant, mock_config_entry: MockConfigEntry): """Test normal sensors.""" # First poll. - b_received_state = hass.states.get("sensor.mock_name_b_received") - b_sent_state = hass.states.get("sensor.mock_name_b_sent") - packets_received_state = hass.states.get("sensor.mock_name_packets_received") - packets_sent_state = hass.states.get("sensor.mock_name_packets_sent") - external_ip_state = hass.states.get("sensor.mock_name_external_ip") - wan_status_state = hass.states.get("sensor.mock_name_wan_status") - assert b_received_state.state == "0" - assert b_sent_state.state == "0" - assert packets_received_state.state == "0" - assert packets_sent_state.state == "0" - assert external_ip_state.state == "8.9.10.11" - assert wan_status_state.state == "Connected" + assert hass.states.get("sensor.mock_name_b_received").state == "0" + assert hass.states.get("sensor.mock_name_b_sent").state == "0" + assert hass.states.get("sensor.mock_name_packets_received").state == "0" + assert hass.states.get("sensor.mock_name_packets_sent").state == "0" + assert hass.states.get("sensor.mock_name_external_ip").state == "8.9.10.11" + assert hass.states.get("sensor.mock_name_wan_status").state == "Connected" # Second poll. - mock_device: MockIgdDevice = hass.data[DOMAIN][ - config_entry.entry_id - ].device._igd_device - mock_device.traffic_data = { - BYTES_RECEIVED: 10240, - BYTES_SENT: 20480, - PACKETS_RECEIVED: 30, - PACKETS_SENT: 40, - } - mock_device.status_data = { - WAN_STATUS: "Disconnected", - ROUTER_UPTIME: 100, - ROUTER_IP: "", - } + mock_igd_device = mock_config_entry.igd_device + mock_igd_device.async_get_total_bytes_received.return_value = 10240 + mock_igd_device.async_get_total_bytes_sent.return_value = 20480 + mock_igd_device.async_get_total_packets_received.return_value = 30 + mock_igd_device.async_get_total_packets_sent.return_value = 40 + mock_igd_device.async_get_status_info.return_value = StatusInfo( + "Disconnected", + "", + 40, + ) + mock_igd_device.async_get_external_ip_address.return_value = "" + now = dt_util.utcnow() async_fire_time_changed(hass, now + timedelta(seconds=DEFAULT_SCAN_INTERVAL)) await hass.async_block_till_done() - b_received_state = hass.states.get("sensor.mock_name_b_received") - b_sent_state = hass.states.get("sensor.mock_name_b_sent") - packets_received_state = hass.states.get("sensor.mock_name_packets_received") - packets_sent_state = hass.states.get("sensor.mock_name_packets_sent") - external_ip_state = hass.states.get("sensor.mock_name_external_ip") - wan_status_state = hass.states.get("sensor.mock_name_wan_status") - assert b_received_state.state == "10240" - assert b_sent_state.state == "20480" - assert packets_received_state.state == "30" - assert packets_sent_state.state == "40" - assert external_ip_state.state == "" - assert wan_status_state.state == "Disconnected" + assert hass.states.get("sensor.mock_name_b_received").state == "10240" + assert hass.states.get("sensor.mock_name_b_sent").state == "20480" + assert hass.states.get("sensor.mock_name_packets_received").state == "30" + assert hass.states.get("sensor.mock_name_packets_sent").state == "40" + assert hass.states.get("sensor.mock_name_external_ip").state == "" + assert hass.states.get("sensor.mock_name_wan_status").state == "Disconnected" -async def test_derived_upnp_sensors(hass: HomeAssistant, config_entry: MockConfigEntry): +async def test_derived_upnp_sensors( + hass: HomeAssistant, mock_config_entry: MockConfigEntry +): """Test derived sensors.""" - coordinator: UpnpDataUpdateCoordinator = hass.data[DOMAIN][config_entry.entry_id] - # First poll. - kib_s_received_state = hass.states.get("sensor.mock_name_kib_s_received") - kib_s_sent_state = hass.states.get("sensor.mock_name_kib_s_sent") - packets_s_received_state = hass.states.get("sensor.mock_name_packets_s_received") - packets_s_sent_state = hass.states.get("sensor.mock_name_packets_s_sent") - assert kib_s_received_state.state == "unknown" - assert kib_s_sent_state.state == "unknown" - assert packets_s_received_state.state == "unknown" - assert packets_s_sent_state.state == "unknown" + assert hass.states.get("sensor.mock_name_kib_s_received").state == "unknown" + assert hass.states.get("sensor.mock_name_kib_s_sent").state == "unknown" + assert hass.states.get("sensor.mock_name_packets_s_received").state == "unknown" + assert hass.states.get("sensor.mock_name_packets_s_sent").state == "unknown" # Second poll. + mock_igd_device = mock_config_entry.igd_device + mock_igd_device.async_get_total_bytes_received.return_value = int( + 10240 * DEFAULT_SCAN_INTERVAL + ) + mock_igd_device.async_get_total_bytes_sent.return_value = int( + 20480 * DEFAULT_SCAN_INTERVAL + ) + mock_igd_device.async_get_total_packets_received.return_value = int( + 30 * DEFAULT_SCAN_INTERVAL + ) + mock_igd_device.async_get_total_packets_sent.return_value = int( + 40 * DEFAULT_SCAN_INTERVAL + ) + now = dt_util.utcnow() with patch( "homeassistant.components.upnp.device.utcnow", return_value=now + timedelta(seconds=DEFAULT_SCAN_INTERVAL), ): - mock_device: MockIgdDevice = coordinator.device._igd_device - mock_device.traffic_data = { - BYTES_RECEIVED: int(10240 * DEFAULT_SCAN_INTERVAL), - BYTES_SENT: int(20480 * DEFAULT_SCAN_INTERVAL), - PACKETS_RECEIVED: int(30 * DEFAULT_SCAN_INTERVAL), - PACKETS_SENT: int(40 * DEFAULT_SCAN_INTERVAL), - } async_fire_time_changed(hass, now + timedelta(seconds=DEFAULT_SCAN_INTERVAL)) await hass.async_block_till_done() - kib_s_received_state = hass.states.get("sensor.mock_name_kib_s_received") - kib_s_sent_state = hass.states.get("sensor.mock_name_kib_s_sent") - packets_s_received_state = hass.states.get( - "sensor.mock_name_packets_s_received" - ) - packets_s_sent_state = hass.states.get("sensor.mock_name_packets_s_sent") - assert float(kib_s_received_state.state) == pytest.approx(10.0, rel=0.1) - assert float(kib_s_sent_state.state) == pytest.approx(20.0, rel=0.1) - assert float(packets_s_received_state.state) == pytest.approx(30.0, rel=0.1) - assert float(packets_s_sent_state.state) == pytest.approx(40.0, rel=0.1) + assert float( + hass.states.get("sensor.mock_name_kib_s_received").state + ) == pytest.approx(10.0, rel=0.1) + assert float( + hass.states.get("sensor.mock_name_kib_s_sent").state + ) == pytest.approx(20.0, rel=0.1) + assert float( + hass.states.get("sensor.mock_name_packets_s_received").state + ) == pytest.approx(30.0, rel=0.1) + assert float( + hass.states.get("sensor.mock_name_packets_s_sent").state + ) == pytest.approx(40.0, rel=0.1)