Add new Hotspring Integration - #177992
Conversation
There was a problem hiding this comment.
When adding new integrations, limit included platforms to a single platform. While we appreciate the effort, reviewing larger than necessary PRs slows down the review process. Please reduce this PR to a single platform. See the review process for more details.
There was a problem hiding this comment.
Pull request overview
Adds a local-polling Hot Spring spa integration with configuration, diagnostics, and water-heater control.
Changes:
- Adds config-entry setup, polling, and reconfiguration.
- Exposes spa temperature through a water-heater entity.
- Adds diagnostics, tests, dependency metadata, and generated registration.
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
.strict-typing |
Enables strict typing. |
homeassistant/components/hotspring/__init__.py |
Sets up and unloads the integration. |
homeassistant/components/hotspring/config_flow.py |
Implements setup and reconfiguration flows. |
homeassistant/components/hotspring/const.py |
Defines integration constants. |
homeassistant/components/hotspring/coordinator.py |
Polls the spa API. |
homeassistant/components/hotspring/diagnostics.py |
Exposes diagnostic data. |
homeassistant/components/hotspring/entity.py |
Defines shared entity metadata. |
homeassistant/components/hotspring/icons.json |
Adds icon metadata. |
homeassistant/components/hotspring/manifest.json |
Declares integration metadata and dependency. |
homeassistant/components/hotspring/strings.json |
Adds configuration-flow strings. |
homeassistant/components/hotspring/water_heater.py |
Implements temperature control. |
homeassistant/generated/config_flows.py |
Registers the config flow. |
homeassistant/generated/integrations.json |
Registers generated integration metadata. |
requirements_all.txt |
Adds python-hotspring. |
tests/components/hotspring/__init__.py |
Adds platform setup helper. |
tests/components/hotspring/conftest.py |
Adds fixtures and API mocks. |
tests/components/hotspring/test_config_flow.py |
Tests setup and reconfiguration. |
tests/components/hotspring/test_diagnostics.py |
Tests diagnostics. |
tests/components/hotspring/test_init.py |
Tests entry lifecycle and setup failure. |
tests/components/hotspring/test_water_heater.py |
Tests state and temperature control. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (4)
tests/components/hotspring/test_diagnostics.py:21
- Commit the generated Syrupy snapshot for this assertion. There is no
tests/components/hotspring/snapshots/test_diagnostics.ambr, so this test reports a missing snapshot and fails in normal CI runs.
assert (
await get_diagnostics_for_config_entry(hass, hass_client, init_integration)
== snapshot
tests/components/hotspring/test_water_heater.py:34
- Commit the generated Syrupy snapshots for this test. No Hot Spring snapshot files are present, so the state and entity-registry snapshot assertions fail as missing snapshots in normal CI runs.
assert state == snapshot
homeassistant/components/hotspring/config_flow.py:34
- Handle
HotSpringErrorin this validation path as well. The sameupdate()call is treated as raising bothHotSpringConnectionErrorandHotSpringErrorby the coordinator, so a known API/response failure currently escapes the config flow instead of returning a form error.
except HotSpringConnectionError:
errors["base"] = "cannot_connect"
homeassistant/components/hotspring/diagnostics.py:11
- Redact the spa's
ssidfield from diagnostics. The currentSpaInfopayload includes the user's Wi-Fi network name, so diagnostics exports expose network-identifying data; other integrations redact this field (for example,homeassistant/components/aosmith/diagnostics.py:24).
TO_REDACT = {"unique_id", "mac_address"}
- Handle HotSpringError in config flow - Redact hostname and ssid from diagnostics payload - Add exception translations for UpdateFailed and HomeAssistantError - Add approved Syrupy test snapshots
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (3)
homeassistant/components/hotspring/quality_scale.yaml:56
- Mark diagnostics as
todorather thanexempt. Planning diagnostics for a follow-up means the rule applies but is not implemented; established quality-scale metadata represents this astodo(for example,homeassistant/components/energyid/quality_scale.yaml:64-66), while exemptions are reserved for integrations with no diagnosable data.
diagnostics:
status: exempt
comment: Diagnostics will be added in a follow-up PR.
tests/components/hotspring/test_water_heater.py:60
- Use an in-range temperature in this successful service test. The default test unit system is Celsius and this entity advertises a 40 °C maximum, so
103is converted to 217.4 °F—well beyond the device's 104 °F maximum—and the permissive mock masks that this is not a valid successful device call.
ATTR_TEMPERATURE: 103,
homeassistant/components/hotspring/coordinator.py:48
- Add a setup/update test for this
HotSpringErrorbranch. The setup test only injectsHotSpringConnectionError, so theinvalid_responsetranslation path is currently unverified even though it has distinct behavior and dedicated handling.
except HotSpringError as error:
mac_address is optional since it's derived from a value of the API. However this value is guaranteed to be present currently.
- Handle HotSpringError in config flow - Redact hostname and ssid from diagnostics payload - Add exception translations for UpdateFailed and HomeAssistantError - Add approved Syrupy test snapshots
…emperature values
…ater heater components
mac_address is optional since it's derived from a value of the API. However this value is guaranteed to be present currently.
2be1611 to
708cc47
Compare
erwindouna
left a comment
There was a problem hiding this comment.
Making solid progress, @Moustachauve! Some follow up and new points. :)
| errors["base"] = "cannot_connect" | ||
| else: | ||
| await self.async_set_unique_id( | ||
| spa.info.mac_address or spa.info.root_topic |
There was a problem hiding this comment.
What is and how unique is the value of root_topic?
| self._abort_if_unique_id_configured( | ||
| updates={CONF_HOST: user_input[CONF_HOST]} | ||
| ) |
There was a problem hiding this comment.
There's no need for a value, if we've setup the Mac address above. Plus it's a bit odd that we now let it check on the CONF_HOST. Best to omit it and just leave it without a parameter.
| errors=errors, | ||
| ) | ||
|
|
||
| async def _async_get_spa(self, host: str) -> Spa: |
There was a problem hiding this comment.
Is this only use case? If you're re-adding the reconfigure flow I'd suggest to rename it to validate_input to standardize like other integrations.
| info = self.coordinator.data.info | ||
| identifier = info.mac_address or info.root_topic | ||
| self._attr_unique_id = f"{identifier}_{key}" | ||
| connections = set() |
There was a problem hiding this comment.
What is this used for? Dynamic devices?
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="cannot_connect", | ||
| translation_placeholders={"error": str(error)}, |
There was a problem hiding this comment.
Idem feedback on the translations of errors.
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="cannot_connect", | ||
| translation_placeholders={"error": str(error)}, |
There was a problem hiding this comment.
Idem feedback on the translations of errors.
|
|
||
| @property | ||
| @override | ||
| def current_operation(self) -> str | None: |
There was a problem hiding this comment.
How can the type be alternative None, if you always return a valid state constant?
|
|
||
| @pytest.fixture | ||
| def mock_onboarding() -> Generator[MagicMock]: | ||
| """Mock that Home Assistant is currently onboarding.""" |
There was a problem hiding this comment.
Well, you're putting it on False. Why is this Mock needed to begin with? :)
Proposed change
This PR introduces a new Hotspring integration to control hot tubs that are equipped with the official Connected Spa Kit from Hotspring.
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: