diff --git a/custom_components/nvidia_shield/config_flow.py b/custom_components/nvidia_shield/config_flow.py index 2486f4b..80056a5 100644 --- a/custom_components/nvidia_shield/config_flow.py +++ b/custom_components/nvidia_shield/config_flow.py @@ -417,18 +417,17 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): return self.async_abort(reason="already_configured") self._abort_if_unique_id_configured() else: - # t32 F2 (review rider): MAC-less discovery cannot carry a - # unique_id — HA's helper records the uniqueness-less discovery - # (the same discovery context can't stack duplicates) and the - # host guard keeps a second discovery of an already-configured - # host from stacking another prefill. - self._async_handle_discovery_without_unique_id() - for entry in self._async_current_entries(): - existing_host = entry.options.get( - CONF_PROTOCOL_HOST - ) or entry.options.get(CONF_ADB_HOST) - if existing_host and existing_host == host: - return self.async_abort(reason="already_configured") + # t32 F2 / t37 D3: MAC-less discovery cannot carry a unique_id, so + # HA's own helper owns the dedup — and it MUST be awaited. It is a + # coroutine that aborts `already_configured` when the handler + # already has entries, claims DEFAULT_DISCOVERY_UNIQUE_ID, and + # aborts `already_in_progress` when another flow for this handler + # is running. t32 shipped the call without the await, so none of + # that ran ("coroutine was never awaited") and the host-equality + # guard that used to sit here was the accidental dedup; the real + # helper is stricter (any entry for the domain, any in-flight + # flow), so nothing is left to do here. + await self._async_handle_discovery_without_unique_id() # First-time setup: pre-filled user step return await self.async_step_user() diff --git a/tests/entity_stublib.py b/tests/entity_stublib.py index cda738a..933b928 100644 --- a/tests/entity_stublib.py +++ b/tests/entity_stublib.py @@ -207,9 +207,13 @@ def install() -> None: async_set_unique_id = _stub_set_unique_id _abort_if_unique_id_configured = _noop_abort - def _async_handle_discovery_without_unique_id(self): - # t32 F2 (audited helper): recording no-op at hub scope - pass + async def _async_handle_discovery_without_unique_id(self): + # t37 D3: the real helper is a COROUTINE and core awaits it — a + # sync stub here would turn the production `await` into a + # TypeError. The abort semantics live in + # tests/test_standalone_setup.py, whose stub owns entry/in-progress + # state; at hub scope there is nothing to abort on. + return None def async_show_form( self, *, step_id, data_schema=None, errors=None, diff --git a/tests/test_config_flow_ux.py b/tests/test_config_flow_ux.py index e6011a6..9d23525 100644 --- a/tests/test_config_flow_ux.py +++ b/tests/test_config_flow_ux.py @@ -114,7 +114,14 @@ def _install_stubs() -> None: def _async_current_entries(self): return [] - def _async_handle_discovery_without_unique_id(self): + async def _async_handle_discovery_without_unique_id(self): + """Real HA helper is a coroutine (t37 D3) — awaiting is mandatory. + + This file never configures entries for the handler, so the real + aborts (already_configured / already_in_progress) cannot trigger; + tests/test_standalone_setup.py exercises those semantics. + """ + self._discovery_dedup_calls = getattr(self, "_discovery_dedup_calls", 0) + 1 return None def _get_reauth_entry(self, entry=None): @@ -496,6 +503,9 @@ class TestDiscoveryPrefill: assert result["type"] == "form" assert result["step_id"] == "user" + # t37 D3: MAC-less discovery needs the AWAITED coroutine helper — the + # counter only moves when the call is awaited + assert flow._discovery_dedup_calls == 1 defaults = vol_mirror.defaults_by_key(result["data_schema"]) # schema-level, not "the attribute was stored": this is precisely what # the old assertion missed while the form rendered blank diff --git a/tests/test_standalone_setup.py b/tests/test_standalone_setup.py index 1ecbd9e..158b928 100644 --- a/tests/test_standalone_setup.py +++ b/tests/test_standalone_setup.py @@ -8,8 +8,10 @@ migration validity for v2 entries without the field. """ import asyncio # noqa: F401 (async steps need the event loop) +import gc import sys import types +import warnings from pathlib import Path # noqa: E402 (F1 test key staging) from types import SimpleNamespace from unittest import mock @@ -42,6 +44,19 @@ _STUB_NAMES = ( ) +class DiscoveryDedupAbortError(Exception): + """Stands in for data_entry_flow.AbortFlow inside the discovery stub. + + HA's `ConfigFlow._async_handle_discovery_without_unique_id` RAISES on a + duplicate (the flow manager turns that into an abort result); modelling it + as a raise is what makes an un-awaited call fail loudly here. + """ + + def __init__(self, reason): + super().__init__(reason) + self.reason = reason + + def _install_stubs() -> None: """Import-time stand-ins (idempotent; same shape as the pairing tests). @@ -101,11 +116,23 @@ def _install_stubs() -> None: def _async_current_entries(self): return _zc_holder.entries - # t32 F2: the audited helper for uniqueness-less discovery — a - # recording no-op here; the flow's own host guard does the stacking - # prevention the acceptance pins. - def _async_handle_discovery_without_unique_id(self): + def _async_in_progress(self, include_uninitialized=False, **kwargs): + return _zc_holder.in_progress + + # t37 D3: HA's own helper, modelled faithfully — a COROUTINE that + # dedups uniqueness-less discovery by raising. An un-awaited call + # therefore does nothing at all, which is exactly the production bug + # this replaces. + async def _async_handle_discovery_without_unique_id(self): _zc_holder.discovery_without_unique_id_calls += 1 + if getattr(self, "_unique_id", None) is not None: + return + if self._async_current_entries(): + raise DiscoveryDedupAbortError("already_configured") + await self.async_set_unique_id(_zc_holder.discovery_unique_id) + self._abort_if_unique_id_configured() + if self._async_in_progress(include_uninitialized=True): + raise DiscoveryDedupAbortError("already_in_progress") class OptionsFlow: def async_show_form(self, *, step_id, data_schema=None, errors=None): @@ -140,7 +167,12 @@ def _install_stubs() -> None: # Shared holder so tests can swap the current-entry list through the # module object (the stub closure reads this same object). - _zc_holder = types.SimpleNamespace(entries=[], discovery_without_unique_id_calls=0) + _zc_holder = types.SimpleNamespace( + entries=[], + in_progress=[], + discovery_unique_id="default_discovery_unique_id", + discovery_without_unique_id_calls=0, + ) sys.modules[__name__]._zc_holder = _zc_holder class AbortIfConfiguredError(Exception): @@ -856,23 +888,27 @@ class TestMacLessDiscoveryF2: ip_address=host, host=host, properties=props or {} ) - async def test_mac_less_records_the_helper(self, tmp_path): + def _holder(self): + return sys.modules[__name__]._zc_holder + + async def test_mac_less_discovery_awaits_the_helper(self, tmp_path): module = _form_module() flow = module.NvidiaShieldConfigFlow() flow.hass = FakeHass(str(tmp_path)) - sys.modules[__name__]._zc_holder.discovery_without_unique_id_calls = 0 + self._holder().discovery_without_unique_id_calls = 0 result = await flow.async_step_zeroconf(self._discovery()) assert result["type"] == "form" # prefill still offered once - assert ( - sys.modules[__name__]._zc_holder.discovery_without_unique_id_calls - == 1 - ) + # t37 D3: only an AWAITED coroutine body runs — before the fix this + # counter stayed at 0 and Python warned "coroutine ... was never + # awaited", while HA's dedup never happened + assert self._holder().discovery_without_unique_id_calls == 1 + # the helper claims HA's discovery sentinel for the flow + assert flow._unique_id == self._holder().discovery_unique_id - async def test_mac_less_rediscovery_does_not_stack(self, tmp_path): - """A second MAC-less discovery of an already-configured host - aborts — one-entry outcome.""" + async def test_mac_less_discovery_with_entries_aborts(self, tmp_path): + """Real contract: any entry for the handler → already_configured.""" module = _form_module() flow = module.NvidiaShieldConfigFlow() flow.hass = FakeHass(str(tmp_path)) @@ -882,13 +918,45 @@ class TestMacLessDiscoveryF2: options={CONF_PROTOCOL_HOST: "192.168.1.50"}, title="Shield", ) - sys.modules[__name__]._zc_holder.entries[:] = [entry] + self._holder().entries[:] = [entry] - result = await flow.async_step_zeroconf(self._discovery()) + with pytest.raises(DiscoveryDedupAbortError) as raised: + await flow.async_step_zeroconf(self._discovery()) - assert result == {"type": "abort", "reason": "already_configured"} + assert raised.value.reason == "already_configured" # the entries list is untouched: one-entry outcome - assert len(sys.modules[__name__]._zc_holder.entries) == 1 + assert len(self._holder().entries) == 1 + + async def test_mac_less_discovery_while_another_flow_runs_aborts(self, tmp_path): + """The other half of the real contract: one flow at a time.""" + module = _form_module() + flow = module.NvidiaShieldConfigFlow() + flow.hass = FakeHass(str(tmp_path)) + self._holder().in_progress[:] = [{"flow_id": "other"}] + + with pytest.raises(DiscoveryDedupAbortError) as raised: + await flow.async_step_zeroconf(self._discovery()) + + assert raised.value.reason == "already_in_progress" + + async def test_mac_less_discovery_leaves_no_un_awaited_coroutine( + self, tmp_path + ): + """t37 D3: the helper must never be left un-awaited.""" + module = _form_module() + flow = module.NvidiaShieldConfigFlow() + flow.hass = FakeHass(str(tmp_path)) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + await flow.async_step_zeroconf(self._discovery()) + del flow + gc.collect() + + never_awaited = [ + str(w.message) for w in caught if "never awaited" in str(w.message) + ] + assert never_awaited == [] class TestSeedBlockShapeF4: