Skip to content

Commit ab152e9

Browse files
Patch76claude
andcommitted
refactor: address PR homeassistant-ai#1398 KP13 review (9 items)
- entity_not_verified detection via warnings-substring scan (homeassistant-ai#1297-popped flag was dead — VERDICT=VALIDATED could leak silently) - sample-count invariant: len(samples) + not_verified == N_SAMPLES - rename "p99" -> "worst" (at N=10 the percentile collapses to max; the threshold-check name now reflects what it measures) - per-test try/finally cleanup: cleanup_tracker is a logging-only no-op, so 10 automations were leaking into the next worker run - caplog.at_level context manager (set_level mutated worker-wide level and leaked DEBUG-on-rest_client into every later test) - 6 comment subtractions per Boy-Scout Boy-Scout fold per homeassistant-ai#1389 measurement (RETUNE NEEDED p50>=100ms): - _POLL_CADENCE: (0.1, 1.0, 4.9) -> (0.025, 1.0, 4.975) — first-poll pulled to a 5x cushion above the measured ~4ms HA-Core entity-registration latency - unit-test pins updated to the new cadence values - test_proxy_http_request_headers_pass_through: add _wait_for_state helper + guard for state="started" (HAOS E2E fail: Node-RED was in "startup" when the strict status_code assertion ran) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 65e6d89 commit ab152e9

5 files changed

Lines changed: 229 additions & 155 deletions

File tree

src/ha_mcp/client/rest_client.py

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -926,9 +926,12 @@ async def upsert_automation_config(
926926
) from e
927927
raise
928928

929-
# 3-attempt × 6s upper-bound budget; first poll 0.1s catches the
930-
# typical sub-1s entity-publish window.
931-
_POLL_CADENCE: tuple[float, ...] = (0.1, 1.0, 4.9)
929+
# 3-attempt × 6s upper-bound budget; first poll 0.025s is a 5×
930+
# cushion above the ~4ms HA-Core entity-registration latency
931+
# measured by ``test_poll_cadence_measurement.py`` (#1389 — p50
932+
# 104.1-104.8 ms on the prior 0.1s first-poll, all from the sleep
933+
# itself with ~4 ms of real registration work).
934+
_POLL_CADENCE: tuple[float, ...] = (0.025, 1.0, 4.975)
932935

933936
async def _poll_for_automation_entity(self, unique_id: str) -> str | None:
934937
"""Poll HA state to find the entity_id assigned to a newly created automation."""

tests/src/e2e/conftest.py

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -89,22 +89,18 @@
8989
_READINESS_TIMINGS: list[dict[str, Any]] = []
9090
_ALL_READINESS_TIMINGS: list[dict[str, Any]] = []
9191

92-
# Same shape as ``_READINESS_TIMINGS``, parallel channel for the #1389
93-
# ``_POLL_CADENCE`` measurement. Lives at this conftest level (not deeper)
94-
# because pytest-xdist's master only loads conftests up to where tests
95-
# are collected; deeper subdir conftests don't get their hooks invoked
96-
# on the master, which is where ``pytest_terminal_summary`` writes to
97-
# the visible terminal output. See ``record_poll_cadence_measurement``
98-
# call site in ``workflows/automation/test_poll_cadence_measurement.py``.
92+
# Parallel channel for the #1389 ``_POLL_CADENCE`` measurement —
93+
# mirrors the xdist round-trip used by ``_READINESS_TIMINGS`` above.
9994
_POLL_CADENCE_MEASUREMENTS: list[dict[str, Any]] = []
10095
_ALL_POLL_CADENCE_MEASUREMENTS: list[dict[str, Any]] = []
10196

10297

10398
def record_poll_cadence_measurement(measurement: dict[str, Any]) -> None:
10499
"""Record a single #1389 measurement run from a test method.
105100
106-
Expected keys: ``n``, ``attempts``, ``p50``, ``p90``, ``p99``, ``min``,
107-
``max``, ``not_verified``, ``verdict``, ``samples``.
101+
See the call site in
102+
``workflows/automation/test_poll_cadence_measurement.py`` for the
103+
measurement dict shape — that file owns the schema.
108104
"""
109105
_POLL_CADENCE_MEASUREMENTS.append(measurement)
110106

@@ -264,7 +260,8 @@ def pytest_terminal_summary(terminalreporter, exitstatus, config):
264260
for m in poll_measurements:
265261
terminalreporter.write_line(
266262
f"[POLL_CADENCE_1389] N={m['n']}/{m.get('attempts', m['n'])} "
267-
f"p50={m['p50']:.1f}ms p90={m['p90']:.1f}ms p99={m['p99']:.1f}ms "
263+
f"p50={m['p50']:.1f}ms p90={m['p90']:.1f}ms "
264+
f"worst={m['worst']:.1f}ms "
268265
f"min={m['min']:.1f}ms max={m['max']:.1f}ms "
269266
f"not_verified={m['not_verified']} VERDICT={m['verdict']}"
270267
)

tests/src/e2e/haos_only/test_manage_addon_modes.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,8 @@
4545

4646
from __future__ import annotations
4747

48+
import asyncio
49+
import time
4850
from typing import Any
4951

5052
import pytest
@@ -61,6 +63,9 @@
6163
MATTER_NAME = "Matter Server"
6264
APPDAEMON_NAME = "AppDaemon"
6365

66+
_STATE_POLL_TIMEOUT = 30.0
67+
_STATE_POLL_INTERVAL = 0.5
68+
6469

6570
async def _resolve_slug(mcp_client: Any, display_name: str) -> str:
6671
"""Map an addon display name to its Supervisor slug at runtime.
@@ -88,6 +93,37 @@ async def _resolve_slug(mcp_client: Any, display_name: str) -> str:
8893
)
8994

9095

96+
async def _wait_for_state(
97+
mcp_client: Any,
98+
slug: str,
99+
expected: str,
100+
*,
101+
timeout: float = _STATE_POLL_TIMEOUT,
102+
) -> str:
103+
"""Poll ``ha_get_addon(slug=...)`` until ``state`` matches ``expected``.
104+
105+
Mirrors the helper in ``test_addon_lifecycle.py``. Same private-copy
106+
rationale as ``_resolve_slug``: pytest treats sibling test files as
107+
independent collection units, so cross-file imports of test-internal
108+
helpers needlessly couple modules. If a third test file needs this,
109+
move both into ``utilities/``.
110+
"""
111+
deadline = time.monotonic() + timeout
112+
last_state: str | None = None
113+
while time.monotonic() < deadline:
114+
detail_raw = await mcp_client.call_tool("ha_get_addon", {"slug": slug})
115+
payload = parse_mcp_result(detail_raw)
116+
addon = payload.get("addon") or {}
117+
last_state = addon.get("state")
118+
if last_state == expected:
119+
return str(last_state)
120+
await asyncio.sleep(_STATE_POLL_INTERVAL)
121+
pytest.fail(
122+
f"Addon {slug!r} state did not reach {expected!r} within "
123+
f"{timeout}s (last observed: {last_state!r})"
124+
)
125+
126+
91127
# ---------------------------------------------------------------------------
92128
# Config mode — options / boot / auto_update / watchdog round-trips
93129
# ---------------------------------------------------------------------------
@@ -234,6 +270,12 @@ async def test_proxy_http_request_headers_pass_through(mcp_client: Any) -> None:
234270
invalid", which proves the value crossed the wire.
235271
"""
236272
slug = await _resolve_slug(mcp_client, NODERED_NAME)
273+
# Supervisor returns success from ``addon_start`` before the addon's
274+
# container reaches ``state="started"``; without this wait the proxy
275+
# call lands while Node-RED is still in ``startup`` and the tool
276+
# responds with a structured ``SERVICE_CALL_FAILED`` instead of an
277+
# HTTP envelope. Same readiness race the lifecycle tests guard.
278+
await _wait_for_state(mcp_client, slug, "started")
237279
without = await safe_call_tool(
238280
mcp_client,
239281
"ha_manage_addon",

0 commit comments

Comments
 (0)