fix: remove stale devices for deselected chargers after reconfigure (fixes #272) - #424
Open
rhammen wants to merge 31 commits into
Open
fix: remove stale devices for deselected chargers after reconfigure (fixes #272)#424rhammen 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>
…ustom-components#272) A charger deselected via reconfigure kept its device and entities forever: the cleanup pass only removed devices with no entities or deprecated Circuit devices, never checking whether a device's zaptec id was still tracked. Untracked-device removal is gated on manual-select mode plus confirmation that every selected charger was present in the API response this session, so a transient/partial response can't be mistaken for a deselection. That presence check compares against the chargers, not every zaptec object: only chargers reach the keep-set, so a selected id belonging to a non-charger counted as present while dropping out of tracked_devices, authorizing removal of every device in the entry. Also fixes first_time_setup dropping a charger's installation on truthiness: ZaptecBase is a Mapping, so an attribute-less installation is falsy, and an untracked installation would now have its device deleted. The via_device link in create_entities_from_zaptec had the same truthiness bug. Removal breaks out of the identifier loop as well: async_remove_device pops without a default, so a device matching on two identifiers would raise KeyError. The loop now also skips identifiers from other domains, which are never in tracked_devices and would otherwise authorize removal. Ported from custom-components#399 onto custom-components#414's pytest-homeassistant-custom-component test conventions (real hass, real device/entity registries). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 tasks
rhammen
marked this pull request as ready for review
August 25, 2026 22:09
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_entryonly removed devices with no entities or deprecated Circuit devices, never checking whether a device's zaptec id was still tracked._cleanup_devicesand takes that as an explicitcheck_untrackedflag.tracked_devices- returning an empty tracked set alongside "all present", which authorizes removal of every device in the entry.first_time_setupandcreate_entities_from_zaptecboth tested an installation for truthiness.ZaptecBaseis aMapping, so an attribute-less installation is falsy: the first dropped it fromtracked_devices(and untracked now means deleted), the second dropped the charger'svia_devicelink.async_remove_devicepops without a default, so a device matching on two identifiers raisedKeyError.tracked_devices, so an identifier from another integration would have authorized removal of the device.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 #399, which predates #414's harness migration and was written against the test architecture it replaces.
Test plan
ruff format --diff/ruff check- cleanpytest tests --ignore=tests/zaptec --cov=./custom_components/zaptec --cov-branch- 44 passed, 1 skipped;__init__.pyat 91%,manager.pyat 92%,coordinator.pyandentity.pyat 100%. Run 15x to confirm stability, since platform setup order is not deterministicpytest tests/zaptec -p no:homeassistant --cov-append- passing (green in CI; thezaptec_constantsfixture needs outbound network, which my local container lacks)first_time_setupselection (including a selected id that is not a charger, and an installation kept despite being falsy), each_cleanup_devicesbranch, a device matching on two identifiers, an identifier from another integration, a charger with and without an installation (via_deviceset or absent), and end-to-end setup asserting a deselected charger's device is removed while a partial API response leaves it aloneAI policy compliance
I personally reviewed this code together with Claude to the best of my knowledge, going through each changed file in turn. Based on that process, I believe this complies with the AI policy to the best of my ability.
🤖 Generated with Claude Code