Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions homeassistant/components/media_player/cast.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,6 @@
vol.All(cv.ensure_list, [cv.string]),
})

CONNECTION_RETRY = 3
CONNECTION_RETRY_WAIT = 2
CONNECTION_TIMEOUT = 10


@attr.s(slots=True, frozen=True)
class ChromecastInfo:
Expand Down Expand Up @@ -216,9 +212,12 @@ async def async_setup_entry(hass, config_entry, async_add_entities):
if not isinstance(config, list):
config = [config]

await asyncio.wait([
# no pending task
done, _ = await asyncio.wait([
_async_setup_platform(hass, cfg, async_add_entities, None)
for cfg in config])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't pending always empty here? From what I understand from the docs, pending contains a set of tasks that are not done when the timeout occurs. But there is no timeout configured here...

Also, it would be good to cancel all tasks in pending.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed pending

if any([task.exception() for task in done]):
raise PlatformNotReady

@OttoWinter OttoWinter Sep 23, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the setup for two chromecast succeeded but failed for a third chromecast? Then I think this code would raise a PlatformNotReady and re-schedule setup. But each time the setup occurs, this would end up re-connecting all three chromecasts, not just the one that failed.

If the devices have no UUID (don't know if this can be the case with config entries), this could end up creating new entities for the two connectable chromecasts every 10 seconds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will get the message such as
Discovered previous chromecast ...



async def _async_setup_platform(hass: HomeAssistantType, config: ConfigType,
Expand Down Expand Up @@ -250,8 +249,8 @@ def async_cast_discovered(discover: ChromecastInfo) -> None:
if cast_device is not None:
async_add_entities([cast_device])

async_dispatcher_connect(hass, SIGNAL_CAST_DISCOVERED,
async_cast_discovered)
remove_handler = async_dispatcher_connect(
hass, SIGNAL_CAST_DISCOVERED, async_cast_discovered)
# Re-play the callback for all past chromecasts, store the objects in
# a list to avoid concurrent modification resulting in exception.
for chromecast in list(hass.data[KNOWN_CHROMECAST_INFO_KEY]):
Expand All @@ -265,8 +264,11 @@ def async_cast_discovered(discover: ChromecastInfo) -> None:
info = await hass.async_add_job(_fill_out_missing_chromecast_info,
info)
if info.friendly_name is None:
# HTTP dial failed, so we won't be able to connect.
_LOGGER.debug("Cannot retrieve detail information for chromecast"
" %s, the device may not be online", info)
remove_handler()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, removing the handler is a good idea here 👍

raise PlatformNotReady

hass.async_add_job(_discover_chromecast, hass, info)


Expand Down Expand Up @@ -379,7 +381,7 @@ async def async_set_cast_info(self, cast_info):
pychromecast._get_chromecast_from_host, (
cast_info.host, cast_info.port, cast_info.uuid,
cast_info.model_name, cast_info.friendly_name
), CONNECTION_RETRY, CONNECTION_RETRY_WAIT, CONNECTION_TIMEOUT)
))
self._chromecast = chromecast
self._status_listener = CastStatusListener(self, chromecast)
# Initialise connection status as connected because we can only
Expand Down
10 changes: 6 additions & 4 deletions tests/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -609,16 +609,18 @@ def mock_open_f(fname, **_):
return patch.object(yaml, 'open', mock_open_f, create=True)


def mock_coro(return_value=None):
"""Return a coro that returns a value."""
return mock_coro_func(return_value)()
def mock_coro(return_value=None, exception=None):
"""Return a coro that returns a value or raise an exception."""
return mock_coro_func(return_value, exception)()


def mock_coro_func(return_value=None):
def mock_coro_func(return_value=None, exception=None):
"""Return a method to create a coro function that returns a value."""
@asyncio.coroutine
def coro(*args, **kwargs):
"""Fake coroutine."""
if exception:
raise exception
return return_value

return coro
Expand Down
20 changes: 20 additions & 0 deletions tests/components/media_player/test_cast.py
Original file line number Diff line number Diff line change
Expand Up @@ -415,3 +415,23 @@ async def test_entry_setup_list_config(hass: HomeAssistantType):
assert len(mock_setup.mock_calls) == 2
assert mock_setup.mock_calls[0][1][1] == {'host': 'bla'}
assert mock_setup.mock_calls[1][1][1] == {'host': 'blu'}


async def test_entry_setup_platform_not_ready(hass: HomeAssistantType):
"""Test failed setting up entry will raise PlatformNotReady."""
await async_setup_component(hass, 'cast', {
'cast': {
'media_player': {
'host': 'bla'
}
}
})

with patch(
'homeassistant.components.media_player.cast._async_setup_platform',
return_value=mock_coro(exception=PlatformNotReady)) as mock_setup:
with pytest.raises(PlatformNotReady):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the code, this could be just Exception right?

@awarecan awarecan Sep 23, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: I see it now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed test to throw Exception

await cast.async_setup_entry(hass, MockConfigEntry(), None)

assert len(mock_setup.mock_calls) == 1
assert mock_setup.mock_calls[0][1][1] == {'host': 'bla'}