-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
Add tests for join and unjoin service calls in Sonos #145602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hey there @jjlawren, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, @PeteRager!
tests/components/sonos/conftest.py
Outdated
soco: MockSoCo, | ||
service: SonosMockService, | ||
variables: dict[str, str], | ||
uui_ds: str | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is an uui_d?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uui_ds is an optional XML element that may be present in a Sonos zone topology event. Sonos chose the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better name to pick? Because I can see many people checking this and thinking its a typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An option is to update it to use the full name that Sonos sends in the event which is
zone_player_uui_ds_in_group
That's probably the best so that searching for this attribute will also return this mock function parameter.
with caplog.at_level(logging.WARNING): | ||
caplog.clear() | ||
await hass.services.async_call( | ||
MP_DOMAIN, | ||
SERVICE_JOIN, | ||
{ | ||
"entity_id": "media_player.living_room", | ||
"group_members": ["media_player.bedroom"], | ||
}, | ||
blocking=False, | ||
) | ||
await join_complete_event.wait() | ||
# Fire the ZGS event to update the speaker grouping as the join method is waiting | ||
# for the speakers to be regrouped. | ||
group_speakers(soco_living_room, soco_bedroom) | ||
await hass.async_block_till_done(wait_background_tasks=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the joining failed I think we should raise a HomeAssistantError
instead so we can tell the user that without them having to look at the logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that is a good change.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
Improve test coverage for media_player.join and add test coverage for media_player.unjoin in the Sonos Integration.
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: