Songpal code and test improvement

.coveragerc

@@ -679,8 +679,6 @@ omit =
     homeassistant/components/somfy/*
     homeassistant/components/somfy_mylink/*
     homeassistant/components/sonarr/sensor.py
-    homeassistant/components/songpal/__init__.py
-    homeassistant/components/songpal/media_player.py
     homeassistant/components/sonos/*
     homeassistant/components/sony_projector/switch.py
     homeassistant/components/spc/*

homeassistant/components/songpal/manifest.json

@@ -10,5 +10,6 @@
       "st": "urn:schemas-sony-com:service:ScalarWebAPI:1",
       "manufacturer": "Sony Corporation"
     }
-  ]
+  ],
+  "quality_scale": "gold"
 }

homeassistant/components/songpal/media_player.py

@@ -3,6 +3,7 @@
 from collections import OrderedDict
 import logging
 
+import async_timeout
 from songpal import (
     ConnectChange,
     ContentChange,
@@ -23,15 +24,13 @@
     SUPPORT_VOLUME_STEP,
 )
 from homeassistant.config_entries import ConfigEntry
-from homeassistant.const import (
-    ATTR_ENTITY_ID,
-    CONF_NAME,
-    EVENT_HOMEASSISTANT_STOP,
-    STATE_OFF,
-    STATE_ON,
-)
+from homeassistant.const import CONF_NAME, EVENT_HOMEASSISTANT_STOP, STATE_OFF, STATE_ON
 from homeassistant.exceptions import PlatformNotReady
-from homeassistant.helpers import config_validation as cv, device_registry as dr
+from homeassistant.helpers import (
+    config_validation as cv,
+    device_registry as dr,
+    entity_platform,
+)
 from homeassistant.helpers.typing import HomeAssistantType
 
 from .const import CONF_ENDPOINT, DOMAIN, SET_SOUND_SETTING
@@ -50,13 +49,7 @@
     | SUPPORT_TURN_OFF
 )
 
-SET_SOUND_SCHEMA = vol.Schema(
-    {
-        vol.Optional(ATTR_ENTITY_ID): cv.entity_id,
-        vol.Required(PARAM_NAME): cv.string,
-        vol.Required(PARAM_VALUE): cv.string,
-    }
-)
+INITIAL_RETRY_DELAY = 10
 
 
 async def async_setup_platform(
@@ -72,58 +65,38 @@ async def async_setup_entry(
     hass: HomeAssistantType, config_entry: ConfigEntry, async_add_entities
 ) -> None:
     """Set up songpal media player."""
-    if DOMAIN not in hass.data:
-        hass.data[DOMAIN] = {}
-
     name = config_entry.data[CONF_NAME]
     endpoint = config_entry.data[CONF_ENDPOINT]
 
-    if endpoint in hass.data[DOMAIN]:
-        _LOGGER.debug("The endpoint exists already, skipping setup.")
-        return
-
-    device = SongpalDevice(name, endpoint)
+    device = Device(endpoint)
     try:
-        await device.initialize()
-    except SongpalException as ex:
-        _LOGGER.error("Unable to get methods from songpal: %s", ex)
+        async with async_timeout.timeout(
+            10
+        ):  # set timeout to avoid blocking the setup process
+            await device.get_supported_methods()
+    except (SongpalException, asyncio.TimeoutError) as ex:
+        _LOGGER.warning("[%s(%s)] Unable to connect.", name, endpoint)
+        _LOGGER.debug("Unable to get methods from songpal: %s", ex)
         raise PlatformNotReady
 
-    hass.data[DOMAIN][endpoint] = device
-
-    async_add_entities([device], True)
-
-    async def async_service_handler(service):
-        """Service handler."""
-        entity_id = service.data.get("entity_id", None)
-        params = {
-            key: value for key, value in service.data.items() if key != ATTR_ENTITY_ID
-        }
-
-        for device in hass.data[DOMAIN].values():
-            if device.entity_id == entity_id or entity_id is None:
-                _LOGGER.debug(
-                    "Calling %s (entity: %s) with params %s", service, entity_id, params
-                )
+    songpal_entity = SongpalEntity(name, device)
+    async_add_entities([songpal_entity], True)
 
-                await device.async_set_sound_setting(
-                    params[PARAM_NAME], params[PARAM_VALUE]
-                )
-
-    hass.services.async_register(
-        DOMAIN, SET_SOUND_SETTING, async_service_handler, schema=SET_SOUND_SCHEMA
+    platform = entity_platform.current_platform.get()
+    platform.async_register_entity_service(
+        SET_SOUND_SETTING,
+        {vol.Required(PARAM_NAME): cv.string, vol.Required(PARAM_VALUE): cv.string},
+        "async_set_sound_setting",
     )
 
 
-class SongpalDevice(MediaPlayerEntity):
+class SongpalEntity(MediaPlayerEntity):
     """Class representing a Songpal device."""
 
-    def __init__(self, name, endpoint, poll=False):
+    def __init__(self, name, device):
         """Init."""
         self._name = name
-        self._endpoint = endpoint
-        self._poll = poll
-        self.dev = Device(self._endpoint)
+        self._dev = device
         self._sysinfo = None
         self._model = None
 
@@ -143,19 +116,15 @@ def __init__(self, name, endpoint, poll=False):
     @property
     def should_poll(self):
         """Return True if the device should be polled."""
-        return self._poll
+        return False
 
-    async def initialize(self):
-        """Initialize the device."""
-        await self.dev.get_supported_methods()
-        self._sysinfo = await self.dev.get_system_info()
-        interface_info = await self.dev.get_interface_information()
-        self._model = interface_info.modelName
+    async def async_added_to_hass(self):
+        """Run when entity is added to hass."""
+        await self.async_activate_websocket()
 
     async def async_will_remove_from_hass(self):
         """Run when entity will be removed from hass."""
-        self.hass.data[DOMAIN].pop(self._endpoint)
-        await self.dev.stop_listen_notifications()
+        await self._dev.stop_listen_notifications()
 
     async def async_activate_websocket(self):
         """Activate websocket for listening if wanted."""
@@ -182,40 +151,48 @@ async def _power_changed(power: PowerChange):
             self.async_write_ha_state()
 
         async def _try_reconnect(connect: ConnectChange):
-            _LOGGER.error(
-                "Got disconnected with %s, trying to reconnect.", connect.exception
+            _LOGGER.warning(
+                "[%s(%s)] Got disconnected, trying to reconnect.",
+                self.name,
+                self._dev.endpoint,
             )
+            _LOGGER.debug("Disconnected: %s", connect.exception)
             self._available = False
-            self.dev.clear_notification_callbacks()
             self.async_write_ha_state()
 
             # Try to reconnect forever, a successful reconnect will initialize
             # the websocket connection again.
-            delay = 10
+            delay = INITIAL_RETRY_DELAY
             while not self._available:
                 _LOGGER.debug("Trying to reconnect in %s seconds", delay)
                 await asyncio.sleep(delay)
-                # We need to inform HA about the state in case we are coming
-                # back from a disconnected state.
-                await self.async_update_ha_state(force_refresh=True)
-                delay = min(2 * delay, 300)
-
-            _LOGGER.info("Reconnected to %s", self.name)
 
-        self.dev.on_notification(VolumeChange, _volume_changed)
-        self.dev.on_notification(ContentChange, _source_changed)
-        self.dev.on_notification(PowerChange, _power_changed)
-        self.dev.on_notification(ConnectChange, _try_reconnect)
+                try:
+                    await self._dev.get_supported_methods()
+                except SongpalException as ex:
+                    _LOGGER.debug("Failed to reconnect: %s", ex)
+                    delay = min(2 * delay, 300)
+                else:
+                    # We need to inform HA about the state in case we are coming
+                    # back from a disconnected state.
+                    await self.async_update_ha_state(force_refresh=True)
+
+            self.hass.loop.create_task(self._dev.listen_notifications())
+            _LOGGER.warning(
+                "[%s(%s)] Connection reestablished.", self.name, self._dev.endpoint
+            )
 
-        async def listen_events():
-            await self.dev.listen_notifications()
+        self._dev.on_notification(VolumeChange, _volume_changed)
+        self._dev.on_notification(ContentChange, _source_changed)
+        self._dev.on_notification(PowerChange, _power_changed)
+        self._dev.on_notification(ConnectChange, _try_reconnect)
 
         async def handle_stop(event):
-            await self.dev.stop_listen_notifications()
+            await self._dev.stop_listen_notifications()
 
         self.hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, handle_stop)
 
-        self.hass.loop.create_task(listen_events())
+        self.hass.loop.create_task(self._dev.listen_notifications())
 
     @property
     def name(self):
@@ -246,12 +223,20 @@ def available(self):
 
     async def async_set_sound_setting(self, name, value):
         """Change a setting on the device."""
-        await self.dev.set_sound_settings(name, value)
+        _LOGGER.debug("Calling set_sound_setting with %s: %s", name, value)
+        await self._dev.set_sound_settings(name, value)
 
     async def async_update(self):
         """Fetch updates from the device."""
         try:
-            volumes = await self.dev.get_volume_information()
+            if self._sysinfo is None:
+                self._sysinfo = await self._dev.get_system_info()
+
+            if self._model is None:
+                interface_info = await self._dev.get_interface_information()
+                self._model = interface_info.modelName
+
+            volumes = await self._dev.get_volume_information()
             if not volumes:
                 _LOGGER.error("Got no volume controls, bailing out")
                 self._available = False
@@ -269,11 +254,11 @@ async def async_update(self):
             self._volume_control = volume
             self._is_muted = self._volume_control.is_muted
 
-            status = await self.dev.get_power()
+            status = await self._dev.get_power()
             self._state = status.status
             _LOGGER.debug("Got state: %s", status)
 
-            inputs = await self.dev.get_inputs()
+            inputs = await self._dev.get_inputs()
             _LOGGER.debug("Got ins: %s", inputs)
 
             self._sources = OrderedDict()
@@ -286,9 +271,6 @@ async def async_update(self):
 
             self._available = True
 
-            # activate notifications if wanted
-            if not self._poll:
-                await self.hass.async_create_task(self.async_activate_websocket())
         except SongpalException as ex:
             _LOGGER.error("Unable to update: %s", ex)
             self._available = False
@@ -342,11 +324,11 @@ async def async_volume_down(self):
 
     async def async_turn_on(self):
         """Turn the device on."""
-        return await self.dev.set_power(True)
+        return await self._dev.set_power(True)
 
     async def async_turn_off(self):
         """Turn the device off."""
-        return await self.dev.set_power(False)
+        return await self._dev.set_power(False)
 
     async def async_mute_volume(self, mute):
         """Mute or unmute the device."""

tests/components/songpal/__init__.py

@@ -1 +1,104 @@
 """Test the songpal integration."""
+from songpal import SongpalException
+
+from homeassistant.components.songpal.const import CONF_ENDPOINT
+from homeassistant.const import CONF_NAME
+
+from tests.async_mock import AsyncMock, MagicMock, patch
+
+FRIENDLY_NAME = "name"
+ENTITY_ID = f"media_player.{FRIENDLY_NAME}"
+HOST = "0.0.0.0"
+ENDPOINT = f"http://{HOST}:10000/sony"
+MODEL = "model"
+MAC = "mac"
+SW_VERSION = "sw_ver"
+
+CONF_DATA = {
+    CONF_NAME: FRIENDLY_NAME,
+    CONF_ENDPOINT: ENDPOINT,
+}
+
+
+def _create_mocked_device(throw_exception=False):
+    mocked_device = MagicMock()
+
+    type(mocked_device).get_supported_methods = AsyncMock(
+        side_effect=SongpalException("Unable to do POST request: ")
+        if throw_exception
+        else None
+    )
+
+    interface_info = MagicMock()
+    interface_info.modelName = MODEL
+    type(mocked_device).get_interface_information = AsyncMock(
+        return_value=interface_info
+    )
+
+    sys_info = MagicMock()
+    sys_info.macAddr = MAC
+    sys_info.version = SW_VERSION
+    type(mocked_device).get_system_info = AsyncMock(return_value=sys_info)
+
+    volume1 = MagicMock()
+    volume1.maxVolume = 100
+    volume1.minVolume = 0
+    volume1.volume = 50
+    volume1.is_muted = False
+    volume1.set_volume = AsyncMock()
+    volume1.set_mute = AsyncMock()
+    volume2 = MagicMock()
+    volume2.maxVolume = 100
+    volume2.minVolume = 0
+    volume2.volume = 20
+    volume2.is_muted = True
+    mocked_device.volume1 = volume1
+    type(mocked_device).get_volume_information = AsyncMock(
+        return_value=[volume1, volume2]
+    )
+
+    power = MagicMock()
+    power.status = True
+    type(mocked_device).get_power = AsyncMock(return_value=power)
+
+    input1 = MagicMock()
+    input1.title = "title1"
+    input1.uri = "uri1"
+    input1.active = False
+    input1.activate = AsyncMock()
+    mocked_device.input1 = input1
+    input2 = MagicMock()
+    input2.title = "title2"
+    input2.uri = "uri2"
+    input2.active = True
+    type(mocked_device).get_inputs = AsyncMock(return_value=[input1, input2])
+
+    type(mocked_device).set_power = AsyncMock()
+    type(mocked_device).set_sound_settings = AsyncMock()
+    type(mocked_device).listen_notifications = AsyncMock()
+    type(mocked_device).stop_listen_notifications = AsyncMock()
+
+    notification_callbacks = {}
+    mocked_device.notification_callbacks = notification_callbacks
+
+    def _on_notification(name, callback):
+        notification_callbacks[name] = callback
+
+    type(mocked_device).on_notification = MagicMock(side_effect=_on_notification)
+    type(mocked_device).clear_notification_callbacks = MagicMock()
+
+    return mocked_device
+
+
+def _patch_config_flow_device(mocked_device):
+    return patch(
+        "homeassistant.components.songpal.config_flow.Device",
+        return_value=mocked_device,
+    )
+
+
+def _patch_media_player_device(mocked_device):
+    return patch(
+        "homeassistant.components.songpal.media_player.Device",
+        return_value=mocked_device,
+    )