Songpal code and test improvement

[shenxn]

Breaking change

To call songpal/set_sound_setting on all songpal devices, the entity_id now need to be set to all instead of left unset.

Proposed change

Following reviews by @MartinHjelmare in #34714, here are some code improvements including the test import and service registration.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

[MartinHjelmare]

Thanks! Looks good!

The test failure is unrelated.

[shenxn]

I'm still working on adding some tests for the service calls. Should be done today.

[MartinHjelmare]

Looks good!

[shenxn]

@MartinHjelmare I think the integration is now good for "gold" integration quality. Can you double check if I missed anything? Also, I'm not pretty sure what the "Uses aiohttp and allows passing in websession" means in the "platinum" quality scale. Do you have any idea where I should look at?

[MartinHjelmare]

Please copy the bullet list of the integration quality scale here and make a comment for each criteria. All may not apply etc.

https://developers.home-assistant.io/docs/integration_quality_scale_index

[rytilahti]

I did just a very brief read-through and to me this looks good (I'm not knowledgeable enough to judge the test improvements, but more testing the better), thanks for the improvements!

About the removal of polling; this will/may break it for some users, as not all devices support the websocket interface. Are you sure we want to do that just to gain "gold status"? Alas, I cannot recall the details, but I think I added support for xhrpost (rytilahti/python-songpal@4b96e30) after getting some feedback just after the initial inclusion to homeassistant. It could have also been that the "xhrpost-only" devices were only sony bravia televisions which are unsupported anyway, so maybe this doesn't affect any real users..

[rytilahti]

Also, I'm not pretty sure what the "Uses aiohttp and allows passing in websession" means in the "platinum" quality scale.

This means that the backend library should be modified to allow passing aiohttp.ClientSession object to be used by the backend, or that's my understanding at least.

[shenxn]

@rytilahti The poll is removed from __init__ simply because it is never used. The SongpalDevice was instantiated using SongpalDevice(name, endpoint) so poll is always False. If we do need to support polling for some devices, maybe we can add that feature in another PR.

[rytilahti]

Ohhhkay, no need to support polling then at all, all the better! I suppose (or hope) that the polling information got read from the config file at some point in the past, though..

[MartinHjelmare]

If the library uses aiohttp it should accept an (optional) client session, to meet the criteria, yes.

[shenxn]

Silver 🥈

• Connection/configuration is handled via a component Not sure about what this means.
• Set an appropriate SCAN_INTERVAL (if a polling integration) Not polling.
• Raise PlatformNotReady if unable to connect during platform setup (if appropriate)
• Handles expiration of auth credentials. Refresh if possible or print correct error and fail setup. If based on a config entry, should trigger a new config entry flow to re-authorize. No authentication.
• Handles internet unavailable. Log a warning once when unavailable, log once when reconnected. Not pretty sure about this one. But since we connect to the device directly, I think we don't need to handle this separately.
• Handles device/service unavailable. Log a warning once when unavailable, log once when reconnected. Handled and there is test for that.
• Set available property to False if appropriate Unavailable when disconnected. There is test for that.
• Entities have unique ID (if available) MAC address is used as unique ID.

Gold 🥇

• Configurable via config entries
  • Don't allow configuring already configured device/service (example: no 2 entries for same hub) Checked using endpoint.
  • Tests for config flow
  • Discoverable (if available SSDP discoverable
  • Set unique ID in config flow (if available)
• Entities have device info (if available)
• Tests for fetching data from the integration and controlling it
• Has a code owner
• Entities only subscribe to updates inside async_added_to_hass and unsubscribe inside async_will_remove_from_hass
• Entities have correct device classes where appropriate
• Supports entities being disabled and leverages Entity.entity_registry_enabled_default to disable less popular entities Only one entity so default enabled.
• If the device/service API can remove entities, the integration should make sure to clean up the entity and device registry. If I understand this correctly, the device/service API cannot remove entities for this integration.

[MartinHjelmare]

Box number one means that the config should be centralized with either a config schema in the base integration module and/or by using config entries. So you can check that one too.

[MartinHjelmare]

The gold level is approved. 🎉

[shenxn]

@MartinHjelmare @rytilahti Thanks a lot for the review. 👍