Enhance Rinnai integration with reauthentication handling and configuration improvements #134
Conversation
…mprove docstring for async_update_data method
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive testing infrastructure and makes several improvements to the Rinnai Control-R Home Assistant integration. The changes include test files with mocked dependencies, a GitHub Actions CI/CD workflow, and various code quality improvements to the integration itself.
- Adds three test files with unit tests, platform import tests, and end-to-end integration tests
- Introduces GitHub Actions workflow for automated testing, validation, and release management
- Refactors device coordinator to return data properly and adds null-safety guards to all properties
- Fixes services.yaml configuration and updates manifest/HACS metadata
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_setup_entry.py | Adds unit tests for async_setup_entry and reauth flows with mocked API |
| tests/test_platform_imports.py | Adds test to verify platform modules can be imported |
| tests/test_e2e_config_entry.py | Adds end-to-end test that sets up platforms and verifies entity creation |
| pytest.ini | Configures pytest for async tests and suppresses deprecation warnings |
| hacs.json | Removes redundant metadata (now in manifest.json) |
| custom_components/rinnaicontrolr-ha/translations/en.json | Adds reauth issue translations |
| custom_components/rinnaicontrolr-ha/strings.json | Adds reauth issue strings |
| custom_components/rinnaicontrolr-ha/services.yaml | Fixes invalid default value and converts options to strings |
| custom_components/rinnaicontrolr-ha/manifest.json | Reorganizes fields, adds issue_tracker, bumps version to 1.5.9 |
| custom_components/rinnaicontrolr-ha/device.py | Refactors _async_update_data to return dict, adds null guards to all properties |
| custom_components/rinnaicontrolr-ha/config_flow.py | Removes unnecessary OptionsFlow init method, adds comment |
| custom_components/rinnaicontrolr-ha/init.py | Updates import path, adds issue registry for reauth, converts options to dict |
| .github/workflows/test-with-homeassistant.yaml | Adds comprehensive CI/CD pipeline with validation, testing, and release automation |
Comments suppressed due to low confidence (1)
custom_components/rinnaicontrolr-ha/init.py:14
- Import of 'WATER_HEATER_DOMAIN' is not used.
from homeassistant.components.water_heater.const import DOMAIN as WATER_HEATER_DOMAIN
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def _async_update_data(self) -> None: | ||
| """Update data via library""" | ||
| async def _async_update_data(self) -> dict[str, Any]: | ||
| """Fetch data from device via API and return the fetched device information as a dict[str, Any].""" |
There was a problem hiding this comment.
[nitpick] The docstring is redundant as it restates what's already clear from the return type annotation. Consider describing what the returned dictionary contains (e.g., 'Returns device information with keys: data.getDevice containing device_name, model, firmware, shadow, info, etc.').
| """Fetch data from device via API and return the fetched device information as a dict[str, Any].""" | |
| """ | |
| Fetch data from device via API. | |
| Returns: | |
| dict[str, Any]: Dictionary containing device information with keys such as: | |
| - device_name: Name of the device | |
| - model: Device model | |
| - firmware: Firmware version | |
| - shadow: Device shadow state | |
| - info: Additional device info | |
| ... (other keys may be present depending on API response) | |
| """ |
| fi | ||
| # Check stdout/stderr for critical errors | ||
| if grep -iE "ERROR.*rinnai.*Traceback|CRITICAL.*rinnai" ha_output.log; then |
There was a problem hiding this comment.
The regex pattern ERROR.*rinnai.*Traceback requires 'rinnai' to appear between 'ERROR' and 'Traceback', which may not match multi-line tracebacks where 'ERROR' and 'Traceback' are on different lines or 'rinnai' appears after 'Traceback'. Consider using a more robust pattern like 'ERROR.*rinnai|Traceback.*rinnai|CRITICAL.*rinnai' or checking for these patterns separately.
| if grep -iE "ERROR.*rinnai.*Traceback|CRITICAL.*rinnai" ha_output.log; then | |
| if grep -iE "ERROR.*rinnai|Traceback.*rinnai|CRITICAL.*rinnai" ha_output.log; then |
| from homeassistant.exceptions import ConfigEntryNotReady, ConfigEntryAuthFailed | ||
| from homeassistant.helpers.aiohttp_client import async_get_clientsession | ||
| from homeassistant.components.water_heater import DOMAIN as WATER_HEATER_DOMAIN | ||
| from homeassistant.components.water_heater.const import DOMAIN as WATER_HEATER_DOMAIN |
There was a problem hiding this comment.
The import has been changed from homeassistant.components.water_heater to homeassistant.components.water_heater.const, but WATER_HEATER_DOMAIN appears to be unused in this file. Consider verifying if this import is still necessary or if it can be removed.
| from homeassistant.components.water_heater.const import DOMAIN as WATER_HEATER_DOMAIN |
| def __init__(self, config_entry: config_entries.ConfigEntry): | ||
| """Initialize options flow.""" | ||
| self.config_entry = config_entry | ||
| # No __init__ needed; base class handles config_entry |
There was a problem hiding this comment.
This comment is misleading. The base class config_entries.OptionsFlow does provide a config_entry attribute, but it's set by the framework when the flow is created, not in __init__. A more accurate comment would be: '# config_entry is provided by the base class and set by the framework'.
| # No __init__ needed; base class handles config_entry | |
| # config_entry is provided by the base class and set by the framework |
| import sys | ||
| import types | ||
| import pathlib | ||
| from typing import Optional |
There was a problem hiding this comment.
Import of 'Optional' is not used.
| from typing import Optional |
| import types | ||
| import pathlib | ||
| from typing import Optional | ||
| from importlib.machinery import SourceFileLoader |
There was a problem hiding this comment.
Import of 'SourceFileLoader' is not used.
| from importlib.machinery import SourceFileLoader |
This update addresses multiple issues within the Rinnai integration, including handling expired refresh tokens and notifying users to reauthenticate. The options flow has been refactored to remove deprecated configurations, ensuring compatibility with future Home Assistant versions. Additionally, the integration now gracefully handles missing configuration options, preventing crashes during data updates. The services.yaml file has been corrected to ensure proper formatting of options. Versioning and CI workflows have also been updated for improved stability and testing.
Fixes #120, #129, #128, #127, #85