Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Home Assistant Integration Quality Scale #141

Open
24 of 33 tasks
cgarwood opened this issue Mar 9, 2024 · 9 comments
Open
24 of 33 tasks

Home Assistant Integration Quality Scale #141

cgarwood opened this issue Mar 9, 2024 · 9 comments

Comments

@cgarwood
Copy link
Collaborator

cgarwood commented Mar 9, 2024 •
edited by catsmanac

Putting this here so as to not lose it in HA's issue tracker...

Silver 馃
This integration is able to cope when things go wrong. It will not print any exceptions nor will it fill the log with retry attempts.

  • Satisfying all No score level requirements.
  • Connection/configuration is handled via a component.
  • Set an appropriate SCAN_INTERVAL (if a polling integration)
  • Raise PlatformNotReady if unable to connect during platform setup (if appropriate)
    EDIT 20240312: see discussion reply
  • 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. (docs)
  • Handles internet unavailable. Log a warning once when unavailable, log once when reconnected.
  • Handles device/service unavailable. Log a warning once when unavailable, log once when reconnected.
    EDIT 20240312: see discussion reply
  • Operations like service calls and entity methods (e.g. Set HVAC Mode) have proper exception handling. Raise ServiceValidationError on invalid user input and raise HomeAssistantError for other failures such as a problem communicating with a device. Read more about raising exceptions.
    EDIT 20240312: see discussion reply
  • Set available property to False if appropriate (docs)
    EDIT 20240328: Set to met, see discussion below
    EDIT 20240312: see discussion reply
  • Entities have unique ID (if available) (docs)

Gold 馃
This is a solid integration that is able to survive poor conditions and can be configured via the user interface.

  • Satisfying all Silver level requirements.
  • Configurable via config entries.
  • Don't allow configuring already configured device/service (example: no 2 entries for same hub)
  • Discoverable (if available)
  • Set unique ID in config flow (if available)
  • Raise ConfigEntryNotReady if unable to connect during entry setup (if appropriate)
    EDIT: 20240328: Set to met, removed discussion by mistake
    EDIT 20240313 see discussion reply
  • Entities have device info (if available) (docs)
  • Tests
    EDIT 20240313 See discussion reply
  • Full test coverage for the config flow
  • Above average test coverage for all integration modules
    EDIT 20240313 See discussion reply
  • Tests for fetching data from the integration and controlling it (docs)
    EDIT 20240313 See discussion reply
  • Has a code owner (docs)
  • Entities only subscribe to updates inside async_added_to_hass and unsubscribe inside async_will_remove_from_hass (docs)
    EDIT 20230313 See discussion reply
  • Entities have correct device classes where appropriate (docs)
  • Supports entities being disabled and leverages Entity.entity_registry_enabled_default to disable less popular entities (docs)
  • If the device/service API can remove entities, the integration should make sure to clean up the entity and device registry.
    EDIT 20240313 See discussion reply
  • When communicating with a device or service, the integration implements the diagnostics platform which redacts sensitive information.

Platinum 馃弳
Best of the best. The integration is completely async, meaning it's super fast. Integrations that reach platinum level will require approval by the code owner for each PR.

  • Satisfying all Gold level requirements.
  • Set appropriate PARALLEL_UPDATES constant (docs)
    EDIT 20240313 See discussion reply
  • Support config entry unloading (called when config entry is removed)
    EDIT 20240313 See discussion topic
  • Integration + dependency are async (docs)
  • Uses aiohttp or httpx and allows passing in websession (if making HTTP requests)
  • Handles expired credentials (if appropriate)
@catsmanac
Copy link
Contributor

catsmanac commented Mar 12, 2024 •
edited

  • Raise PlatformNotReady if unable to connect during platform setup (if appropriate).

Analysis

Platforms are created using await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) after first initializing the Envoy object in __init__:async_setup_entry. Connection failure is caught before setting up the platforms

Resolution

Not needed, think this marks this item as met!?

EDIT: 20240328: Setting to met, will retry if envoy doesn't connect.

Back to top

@catsmanac
Copy link
Contributor

catsmanac commented Mar 12, 2024 •
edited

  • Handles device/service unavailable. Log a warning once when unavailable, log once when reconnected.
  • Set available property to False if appropriate (docs)

Analysis

Not sure if these pass. Started HA running and switched off my simulated Envoy. On next scan Pyenphase starts logging every 2-4 seconds:

2024-03-12 10:02:54.943 DEBUG (MainThread) [pyenphase.envoy] Requesting https://192.168.3.104/ivp/meters with timeout Timeout(connect=10.0, read=60.0, write=10.0, pool=10.0)
2024-03-12 10:02:57.498 DEBUG (MainThread) [pyenphase.envoy] Requesting https://192.168.3.104/ivp/meters with timeout Timeout(connect=10.0, read=60.0, write=10.0, pool=10.0)
2024-03-12 10:02:59.914 DEBUG (MainThread) [pyenphase.envoy] Requesting https://192.168.3.104/ivp/meters with timeout Timeout(connect=10.0, read=60.0, write=10.0, pool=10.0)
2024-03-12 10:03:02.599 DEBUG (MainThread) [pyenphase.envoy] Requesting https://192.168.3.104/ivp/meters with timeout Timeout(connect=10.0, read=60.0, write=10.0, pool=10.0)
2024-03-12 10:03:07.436 DEBUG (MainThread) [pyenphase.envoy] Requesting https://192.168.3.104/ivp/meters with timeout Timeout(connect=10.0, read=60.0, write=10.0, pool=10.0)

The entities however stay on the state they had when failure started

afbeelding

afbeelding

When inserting some debug breakpoints it's getting back to

afbeelding

without higher level breakpoints firing. Only when restarting the envoy sim the breakpoint at the request in meters fires when data returns..

So it loops in retry and never seems to return

EDIT: tried with a simple pyenphase client and same happens, update call never returns and tries forever.

Resolution

EDIT 20240328: Pyenphase 1.20.1 has this fixed. Merged in home-assistant/core#114218. Setting this to matched.

Back to top

@catsmanac
Copy link
Contributor

catsmanac commented Mar 12, 2024 •
edited

  • Operations like service calls and entity methods (e.g. Set HVAC Mode) have proper exception handling. Raise ServiceValidationError on invalid user input and raise HomeAssistantError for other failures such as a problem communicating with a device. Read more about raising exceptions.

Analysis

Scanning through the code this seems to be the current list for these:

  • Number
    • envoy.set_reserve_soc
      • Raises ValueError or EnvoyFeatureNotAvailable checking values
    • envoy.update_dry_contact
      • Raises ValueError or EnvoyFeatureNotAvailable checking values

For Number these are activated by async_set_native_value methods. These have no specific error handling

  • Select
    • envoy.set_storage_mode
      • raises TypeError for mode parameter
    • envoy.update_dry_contact
      • Raises ValueError or EnvoyFeatureNotAvailable checking values

For Select these are activated by async_select_option methods. These have no specific error handling

  • Switch
    • envoy.go_on_grid
      • Raises ValueError or EnvoyFeatureNotAvailable checking values
    • envoy.go_off_grid
    • Raises ValueError or EnvoyFeatureNotAvailable checking values
    • envoy.close_dry_contact
      • Raises EnvoyFeatureNotAvailable checking values
    • envoy.open_dry_contact
      • Raises EnvoyFeatureNotAvailable checking values
    • envoy.enable_charge_from_grid
      • Raises ValueError or EnvoyFeatureNotAvailable checking values
    • envoy.disable_charge_from_grid
      • Raises ValueError or EnvoyFeatureNotAvailable checking values

For Switch these are activated by async_turn_on or async_turn_off method. These have no specific exception handling

These all use _request which:

  • raises EnvoyAuthenticationRequired
  • raises EnvoyAuthenticationRequired for HTTPStatus.UNAUTHORIZED, HTTPStatus.FORBIDDEN
  • Does not raise on other errors, which typically results in JSON stackdump (orjson.JSONDecodeError: unexpected character: line 1 column 1 (char 0))

Looks like ServiceValidationError or HomeAssistantError is not implemented by mapping pyenphase status to Home Assistant status?

Resolution

tbd

Back to top

@catsmanac
Copy link
Contributor

catsmanac commented Mar 13, 2024 •
edited

  • Tests
  • Tests for fetching data from the integration and controlling it (docs)

Analysis

Combining two items as it seems they are the same.

Only test_sensors, test_diagnostics currently. (Test_config_flow available as well but that fulfills other requirement).

Resolution

Add tests for:

  • binary-sensor
  • number
  • select
  • switch

I have these ready in same format as test_sensor, but can use some assistance in adding tests for the switches that can be activated. These tests are snapshot test and may need addition of functional tests as well as described in the docs links of the item.

Refs: https://developers.home-assistant.io/docs/development_testing/#writing-tests-for-integrations

EDIT 20240328: PR for pyenphase: #146

Name                                   Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------------------
src/pyenphase/auth.py                    113      3     52      0    98%   21, 190, 198
src/pyenphase/envoy.py                   256      0     98      1    99%   355->359
src/pyenphase/updaters/production.py      88      0     48      1    99%   124->143
----------------------------------------------------------------------------------
TOTAL                                   1238      3    391      2    99%

25 files skipped due to complete coverage.

PR for HA enphase_envoy: home-assistant/core#114390

homeassistant/components/enphase_envoy/config_flow.py                                   96      1    99%   82
homeassistant/components/enphase_envoy/const.py                                          5      0   100%
homeassistant/components/enphase_envoy/diagnostics.py                                   40      0   100%

Once the PR are merged these 2 item can pass.

Back to top

@catsmanac
Copy link
Contributor

catsmanac commented Mar 13, 2024 •
edited

  • Above average test coverage for all integration modules

Analysis

For this one we first need the tests added

Resolution

COV with 2 new pr:

Name                                   Stmts   Miss Branch BrPart  Cover   Missing
----------------------------------------------------------------------------------
src/pyenphase/auth.py                    113      3     52      0    98%   21, 190, 198
src/pyenphase/envoy.py                   256      0     98      1    99%   355->359
src/pyenphase/updaters/production.py      88      0     48      1    99%   124->143
----------------------------------------------------------------------------------
TOTAL                                   1238      3    391      2    99%

PR for HA enphase_envoy: home-assistant/core#114390

homeassistant/components/enphase_envoy/config_flow.py                                   96      1    99%   82
homeassistant/components/enphase_envoy/const.py                                          5      0   100%
homeassistant/components/enphase_envoy/diagnostics.py                                   40      0   100%

Back to top

@catsmanac
Copy link
Contributor

  • Entities only subscribe to updates inside async_added_to_hass and unsubscribe inside async_will_remove_from_hass (docs)

Analysis

? Are we using this, do we even need it ?

Resolution

Back to top

@catsmanac
Copy link
Contributor

  • If the device/service API can remove entities, the integration should make sure to clean up the entity and device registry.

Analysis

Don't think a have API doing this, do we? If not this is met as it doesn't apply

Resolution

No action needed, met by absence of feature.

Back to top

@catsmanac
Copy link
Contributor

  • Set appropriate PARALLEL_UPDATES constant (docs)

Analysis

Add paralellel_updates to platforms if appropriate

PARALLEL_UPDATES = 0

If the value is 0, the integration is itself responsible for limiting the number of parallel requests if necessary.

Seems this is applicable for

  • number
  • select
  • switch

Neither have it currently. We define _async_update_data which results in a default of 0. Is there need to limit this? I.e. only process button pushes sequentially?

Resolution

Add to 3 platform files. When using 0 still behaves as currently.

Back to top

@catsmanac
Copy link
Contributor

  • Support config entry unloading (called when config entry is removed)

Analysis

Think this one is met. There's __init__:async_unload_entry and __init__:async_remove_config_entry_device

Resolution

Not needed, think this one passes

Back to top

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@cgarwood @catsmanac