Refactor services.py: register once in async_setup, fix multi-account resolution - #419
Draft
rhammen wants to merge 31 commits into
Draft
Refactor services.py: register once in async_setup, fix multi-account resolution#419rhammen wants to merge 31 commits into
rhammen wants to merge 31 commits into
Conversation
Design for replacing PR custom-components#394 (coordinator/entity tests) with a pytest-homeassistant-custom-component based approach: real hass + MockConfigEntry, patch at the Zaptec client boundary, assert on public state. Includes an OS-guarded Windows compat shim so the harness runs in native-Windows py314 and on Linux CI. Establishes reusable infra for the later custom-components#395 replacement. Bug custom-components#410 kept test-only (xfail) pending maintainer input on availability semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds success-poll, poll-failure, and charging-interval-switch tests driven through the real HA setup path (setup_integration), covering ZaptecUpdateCoordinator.last_update_success and set_update_interval.
The `hass` fixture is now exercised by real behavior tests, so the temporary tests/test_harness_smoke.py is no longer needed. Coverage check on coordinator.py/entity.py (the two migrated modules) showed real gaps once measured against just the migrated test files: coordinator.py's trigger_poll()/_trigger_poll() sequence (cancellation, child-coordinator triggering, the no-op-without-zaptec_object path) and the charging-interval-requires-Charger validation were entirely untested, and entity.py had a few uncovered branches in _get_zaptec_value(), _log_zaptec_attribute, and _log_unavailable(). Added targeted behavior tests for both, bringing coordinator.py to 100% and entity.py to 98% (line/branch, matching the pre-migration targets). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
is not a bug) Replace the strict-xfail test_entity_becomes_unavailable_when_key_missing, which asserted incorrect intended behavior, with two passing tests that document the real mechanism: CoordinatorEntity.available is driven solely by coordinator.last_update_success, so a single missing backing key leaves the entity available (it just retains its prior value), while a failed coordinator poll does mark the entity unavailable.
requirements_test.txt hard-pinned homeassistant==2026.4.3 and pytest-homeassistant-custom-component==0.13.324, both of which require Python >=3.14 — so CI's 3.13 matrix leg failed at "Install requirements". HA is already pinned in requirements.txt (with a 3.13 sed-revert to 2026.2.3 in validate.yaml), and pytest-hacc pins an exact homeassistant itself, so dropping the duplicate HA line and leaving pytest-hacc unpinned lets pip resolve the release matching whichever HA the active Python leg installs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…3.13 pytest-homeassistant-custom-component pins an exact homeassistant version, so it must match the HA each CI Python leg installs (requirements.txt pins HA; validate.yaml sed-reverts it to 2026.2.3 on the 3.13 leg). Leaving pytest-hacc unpinned made pip backtrack to an ancient 0.2.1 release (dragging in pytest 6.2.2, which crashes Python 3.13's assertion rewriter). Select the matching release per Python version via environment markers: 0.13.324 (HA 2026.4.3) on py>=3.14, 0.13.316 (HA 2026.2.3) on py<3.14. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ns HA closure) Installing requirements.txt (dev-container pins) alongside pytest-hacc's HA dependency closure caused irreconcilable conflicts (e.g. pydantic 2.13.1 vs pytest-hacc's 2.12.2). pytest-hacc is designed to own the HA + test dependency set, so the test job now installs only requirements_test.txt: pytest-hacc brings HA + the pytest stack, and requirements_test.txt adds just the integration's non-HA manifest deps (azure-servicebus, aiolimiter). HA version per Python leg comes from the pytest-hacc marker, so the 3.13 sed-revert is no longer needed. requirements.txt (dev container) is left untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Revert the validate.yaml decoupling: the devcontainer (scripts/setup) also installs requirements.txt + requirements_test.txt together, so decoupling only CI would leave the devcontainer broken by the same conflict. Instead, relax requirements.txt's pydantic from ==2.13.1 to the manifest range (>=2.11.7,<2.14). pytest-hacc pins pydantic to HA's version (2.12.2), which the range allows, so both files now install together in CI and the devcontainer. A dry run confirmed pydantic was the only dependency conflict. Remaining CI failures are the pre-existing live-network tests (test_zconst.py / test_redact.py) which pytest-hacc's socket blocking rejects; those are addressed separately by removing the zaptec_constants live call (supersedes custom-components#398). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…components#257) Pivot the migration design/plan away from the committed native-Windows shim to Linux-native infra: on Linux pytest-hacc autoloads, so the harness is scoped to HA-integration tests via two pytest invocations (pytest tests --ignore=tests/zaptec + pytest tests/zaptec -p no:homeassistant), combining coverage with --cov-append. Keeps the tests/zaptec/* API-client tests (future standalone PyPI lib, custom-components#257) as plain pytest with their live constants call intact. Plan is a delta over the implemented branch and is meant to be executed in the devcontainer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
On Linux (CI + devcontainer) pytest-hacc autoloads via its pytest11 entry point, so the win32-guarded shim (fcntl/resource/socketpair stubs + explicit pytest_plugins load) and the global -p no:homeassistant are no longer needed. This also removes the root cause of the harness's session-wide load, which is what made scoping tests/zaptec/* away from it hard (see Option C in the migration spec). Verified (native Windows, py314): tests/zaptec -p no:homeassistant runs as before (80 passed, 1 skipped, 22 pre-existing DNS errors, no SocketBlockedError); pytest tests --ignore=tests/zaptec now fails fast on ModuleNotFoundError: fcntl, confirming the harness autoload takes effect and that half now requires Linux. ruff format/check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…verage Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…m-components#257) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
….__getitem__ _backed_get's mock .get() did a bare dict lookup, diverging from the real ZaptecBase (which normalizes camelCase API keys to snake_case symmetrically on both read and write). Harmless today since all seed data is hand-authored snake_case, but would have silently broken a future fixture seeded from a raw diagnostics dump (e.g. custom-components#395) without the normalization. to_under is idempotent on already-normalized keys, so this has no effect on current tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n check The interval-switch test only asserted charger_coord.update_interval < idle_interval, which verifies ordering but not the actual values selected. Add equality assertions against ZAPTEC_POLL_INTERVAL_IDLE/CHARGING (matching the pattern in HA core's own coordinator tests, e.g. tests/components/jvc_projector/test_coordinator.py's assert coordinator.update_interval == INTERVAL_SLOW/FAST), which catches a coordinator wiring bug the relation alone would miss. Kept the relation assertion too: it catches a const.py regression (charging >= idle) that the equality checks alone would miss, since the coordinator would still be 'correctly' wired to whatever (wrong) constants are defined. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mat branch Audited every test docstring in test_entity.py for whether a maintainer could tell WHY each private-attribute/method access is needed without re-deriving it themselves. Added the missing 'why' to six spots: - _entity_from_coordinator: why it reaches into _listeners at all (no public API for 'entities subscribed to this coordinator'), why hasattr(_log_value) is the discriminator against the coordinator's own listener, and why key_not_in_skip_list exists (gates one specific _log_unavailable log line). - test_log_value_*: _log_value has no public-state effect, so there's no hass.states equivalent to test through. - test_get_zaptec_value_returns_default_when_key_missing: distinguishes the MISSING-sentinel default (triggers unavailability) from sensor.py's one explicit-default call site (opts out, for a genuinely optional key). - test_get_zaptec_value_raises_when_intermediate_value_not_mapping: no shipped entity currently uses a dotted key, so this guards the helper's documented contract ahead of any real caller. - test_log_zaptec_attribute_*: clarifies which of the four formatting branches are live in production (str, Iterable) vs. unused-but-documented (None) vs. genuinely dead (the scalar fallback) — and adds a 4th assertion covering that previously-untested fallback branch (confirmed missing via direct query against a coverage.py SQLite db from a prior run). - test_log_unavailable_*: explains why it pokes _attr_available/_prev_available directly instead of driving both transitions through a real coordinator refresh — ties directly to the custom-components#410 finding that those attributes are decoupled from the entity's actual HA-reported availability. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The entity_id.startswith('mock') filter only works because conftest.py's
mock_zaptec seeds 'Mock Charger'/'Mock Home' and HA slugifies entity names
into entity_id's object_id half. That dependency lived silently in a
different file; note it here so a future rename doesn't produce a confusing
'expected at least one zaptec entity' failure with no pointer to the cause.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ctor tests Audited the remaining tests in test_coordinator.py for the same 'why' gap: - test_charging_update_interval_requires_charger_object: why a bare MagicMock() manager suffices (accessed via manager.zaptec, auto-vivifies, before the guard fires) and why setup_integration isn't used (deliberately bypassed to hit the constructor guard in isolation). - test_trigger_poll_is_noop_without_zaptec_object: why head_coordinator specifically (the one coordinator built with zaptec_object=None, unlike every device coordinator). - test_trigger_poll_triggers_child_charger_coordinators: why asyncio.sleep is patched globally instead of a delays-list constant (installations use a different constant than the sibling cancel/reschedule test patches), and why the child coordinator's trigger_poll is mocked rather than left real (isolates parent-calls-child from the child's own mechanics). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Audited conftest.py for the same 'why' gap as the other test files: - make_charger: model is hardcoded to ZaptecBase's base default format, not Charger.model's real device-ID-prefix lookup override -- a known simplification, same category as _backed_get's already-documented ones. - mock_zaptec: the __getitem__/__iter__/__contains__/__len__ wiring isn't arbitrary scaffolding -- Zaptec is itself Mapping[str, ZaptecBase] in production, and real code indexes into it directly. - mock_zaptec: redact.dumps.return_value = '' is load-bearing, not incidental -- __init__.py's startup debug-dump path concatenates its result into a string, which would TypeError on an unconfigured MagicMock. - setup_integration: notes the unittest.mock 'patch where it's used, not where it's defined' rule behind the patch target, since __init__.py holds its own local Zaptec reference via 'from .zaptec import Zaptec'. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Moved to a dedicated fork-only branch per maintainer preference not to carry AI-generated planning docs in the upstream repo.
… review sveinse noted this code may move to other repos where the referenced issue numbers become meaningless; drop them from comments/docstrings while keeping the technical explanation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sveinse asked for setup specific to the future-standalone API client (custom-components#414 review) to live under tests/zaptec/, separate from the HA integration test setup. zaptec_username/zaptec_password stay in the root conftest since tests/test_diagnostics.py (outside tests/zaptec, not yet converted to the new pattern) still needs them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
steinmn noted the per-Python pytest-hacc pins speak for themselves (custom-components#414 review). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… resolution Fixes custom-components#253 - moves service registration from async_setup_entry (once per config entry) to async_setup (once at HA startup), per HA's action-setup quality-scale rule. async_unload_services is removed - nothing else called it, and services now live for HA's lifetime, not any one config entry's. Also fixes a real bug this refactor uncovered: iter_objects used to search only the one ZaptecManager captured in a closure at registration time. Because registration was guarded by hass.services.has_service, a user with two Zaptec accounts could never reach the second account's chargers via zaptec.* services - only whichever entry set up first ever got wired in. iter_objects now resolves service_call.hass and searches every currently loaded config entry's manager. Rebuilt on top of the pytest-hacc harness migration (PR custom-components#414): tests use the real hass/MockConfigEntry harness throughout, including a full test_services.py rewrite exercising real HA device/entity registries and real service dispatch through async_setup_services.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
async_setup_servicesregistration fromasync_setup_entry(once per config entry) to a newasync_setup(once at HA startup), per HA's action-setup quality-scale rule.async_unload_servicesis removed - nothing else called it, and services now live for HA's lifetime, not any one config entry's.iter_objectsand all eightservice_handle_*coroutines out of the closure nested insideasync_setup_servicesto module level inservices.py.iter_objectsused to search only the oneZaptecManagercaptured in a closure at registration time. Because registration was guarded byhass.services.has_service, a user with two Zaptec accounts (two config entries) could never reach the second account's chargers viazaptec.*services - only whichever entry set up first ever got wired in.iter_objectsnow resolvesservice_call.hassand searches every currently loaded config entry's manager viahass.config_entries.async_entries(DOMAIN)+entry.runtime_data.Dependency note
This branch is built on top of #414 (pytest-homeassistant-custom-component harness migration). Please merge #414 first - until then this PR's diff will include its changes too.
Supersedes #402, which predates #414's harness migration and needs the same rebuild #414's own description calls for.
Test plan
ruff format --diff/ruff check- cleanpytest tests --ignore=tests/zaptec --cov=./custom_components/zaptec --cov-branch- all passingpytest tests/zaptec -p no:homeassistant --cov-append- all passingservices.pyat 100% statement coverage (99% branch - two branches are dead code giveniter_objectsis only ever called withmustbe=Charger/Installation)runtime_data(not currently loaded) being skipped rather than crashing, plus all pre-existing service-handler anditer_objectserror-path coverage, rewritten against the realhass/MockConfigEntryharnessAI policy compliance
I personally reviewed this code together with Claude to the best of my knowledge, including an independent subagent code review of the branch. Based on that process, I believe this complies with the AI policy to the best of my ability.
🤖 Generated with Claude Code