Devialet browse and play media #134168
Devialet browse and play media #134168
Conversation
fwestenberg
force-pushed
the
Devialet-browse-and-play-media
branch
from
e6bb28c to
adb1fbb
Compare
| if await self.client.async_search_allowed(): | ||
| self.entry.async_create_background_task( | ||
| hass=self.hass, | ||
| target=self.client.async_discover_upnp_device(), | ||
| name=f"{DOMAIN}_UPnP_Discovery", | ||
| ) |
There was a problem hiding this comment.
At every restart of the speaker, the UPnP port will change. This method finds the UPnP port and initiates the playback capability.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| }, | ||
| "exceptions": { | ||
| "upnp_error": { | ||
| "message": "Unable to play media. UPnP setup not ready for Devialet device." |
There was a problem hiding this comment.
What can the user do about this?
There was a problem hiding this comment.
The idea is, when you select a source (radio browser for example) and playback is not possible, you get noticed instead of waiting and nothing happens...
There was a problem hiding this comment.
But usually when UPnP is not available, browsing media is also not supported. This should (almost) never happen.
There was a problem hiding this comment.
Yes, but what does UPnP setup not ready mean for the user? What do they need to do to fix it
There was a problem hiding this comment.
There is no known solution, beside maybe restart the speaker.
| @@ -13,18 +17,19 @@ async def test_load_unload_config_entry( | |||
| hass: HomeAssistant, aioclient_mock: AiohttpClientMocker | |||
| ) -> None: | |||
| """Test the Devialet configuration entry loading and unloading.""" | |||
| entry = await setup_integration(hass, aioclient_mock) | |||
| with patch.object(DevialetApi, "upnp_available", return_value=True): | |||
There was a problem hiding this comment.
| with patch.object(DevialetApi, "upnp_available", return_value=True): | |
| with patch("homeassistant.components.devialet.DevialetApi.upnp_available", return_value=True): |
The integration would be a lot nicer if we could patch the library instead of the HTTP calls
There was a problem hiding this comment.
Why do you prefer the patch above the patch.object here?
| assert entry.state is ConfigEntryState.LOADED | ||
|
|
||
| freezer.tick(10) | ||
| async_fire_time_changed(hass) |
There was a problem hiding this comment.
| async_fire_time_changed(hass) | |
| async_fire_time_changed(hass) | |
| await hass.async_block_till_done() |
There was a problem hiding this comment.
Why do we need to skip time here in the first place? The integration already has state at this point right?
There was a problem hiding this comment.
If this part is removed, the update has not taken place yet and the attributes are not set.
There was a problem hiding this comment.
So without the tick: KeyError: 'volume_level'
| with patch.object(DevialetApi, "equalizer", new_callable=PropertyMock) as mock: | ||
| mock.return_value = None | ||
|
|
||
| with patch.object( | ||
| DevialetApi, "night_mode", new_callable=PropertyMock | ||
| ) as mock: | ||
| mock.return_value = True | ||
|
|
There was a problem hiding this comment.
Seeing this I would really recommend doing proper mocks, because this isn't ideal. Like, either use library mocks or use http mocks. Don't use both as this becomes hard to maintain
There was a problem hiding this comment.
I can improve testing in another PR. It requires a lot of time to me and this is already an existing part.
There was a problem hiding this comment.
Yes but the tests themselves can be a lot cleaner than having multiple nested with patches, you can consider patching the full object or merging the patch statements with patch(), patch():
There was a problem hiding this comment.
So I merged the patch statements at all locations. It's not perfect, but already a lot cleaner I think. Thanks for the suggestion.
| patch( | ||
| "homeassistant.components.media_source.async_browse_media", | ||
| return_value=True, | ||
| ) as mock_browse_media, |
There was a problem hiding this comment.
Why do we patch this internal?
There was a problem hiding this comment.
What would be another solution to test the async browse media function?
There was a problem hiding this comment.
I don't know, I did work on the Spotify one, but I am more wondering why we do this so I can see how we would solve this otherwise, hence its just a question rather than a "this should be changed" :)
There was a problem hiding this comment.
I tried to patch the first function after the Devialet integration's call. So that's why I've patched this one. Not sure if it's the best way to test it but it seems to work.
Breaking change
Proposed change
Add play media and browse media support for Devialet.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: