fix(config): degrade an unreachable Shield to the pairing form (t44 F1)
`_start_protocol_pairing` called androidtvremote2's cert generation and `async_start_pairing` with no try/except, so `CannotConnect` propagated straight out of `async_step_protocol_pair`. The caller already had the shape for a soft failure — `_async_begin_protocol_pairing()` returns `(client, None) | (None, error)` and `_async_restart_protocol_pairing` renders `errors["base"]` — the exception simply bypassed it, and the protocol_pair form never appeared: not the address field, not the "continue without pairing" escape. That made it a default-path crash rather than an edge case: t41 turned the protocol transport ON by default, so an asleep, rebooting or mis-addressed Shield (whenever someone adds the integration, or skips ADB pairing into this step) raised out of the flow instead of showing the form. The reviewer reproduced both under real HA. Now the step returns `(None, "protocol_unreachable")`, logs the address and the library error, discards the half-built client so a retry starts clean, and renders the same form the other failures render — with the address editable and the skip option intact. `protocol_unreachable` is documented in strings.json AND translations/en.json (a form error without copy shows a bare key in the UI). Tests (tests/test_config_flow_ux.py::TestUnreachableShieldDuringPairing): the fake client can now fail to start pairing, and five cases cover fresh setup, the ADB-skip route into the protocol step, reconfigure, reauth and the skip path that still finishes the entry — all as FORMS, never exceptions. Red proof on HEAD: the same drive raises CannotConnectStubError out of async_step_user. Verified: 361 passed / 2 skipped, 362 / 1 with androidtvremote2, ruff clean.
This commit is contained in:
1 parent
7608518d97
commit
3175582419
5 files changed
+240
-16
No files matched your search
@@ -416,12 +416,20 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
|
||||
|
||||
Certificates live under ``.storage/<domain>/`` (core androidtv_remote
|
||||
helpers.py pattern) and are shared between pairing and runtime. Returns
|
||||
None when no host is resolvable or ``androidtvremote2`` is missing —
|
||||
the caller turns that into an error key, never a crash.
|
||||
``(client, None)`` or ``(None, error_key)`` — no host, a missing
|
||||
``androidtvremote2`` and an unreachable Shield are all keys the form can
|
||||
show, never an exception out of the step.
|
||||
|
||||
t44 F1: androidtvremote2 raises ``CannotConnect`` from both calls below,
|
||||
and the protocol transport is ON by default, so an asleep, rebooting or
|
||||
mis-addressed Shield is the ordinary case rather than an edge one. Left
|
||||
uncaught it escaped the config flow entirely, and the protocol_pair form
|
||||
— the address field AND the "continue without pairing" escape — never
|
||||
rendered.
|
||||
"""
|
||||
host = self._host()
|
||||
if not host:
|
||||
return None
|
||||
return None, "protocol_no_host"
|
||||
certfile, keyfile = await self.hass.async_add_executor_job(
|
||||
protocol_cert_paths, self.hass
|
||||
)
|
||||
@@ -434,19 +442,26 @@ class NvidiaShieldConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
|
||||
if client is None:
|
||||
# build_protocol_client already logged the missing library;
|
||||
# pairing cannot start, and the form still offers the skip path
|
||||
return None
|
||||
await client.async_generate_cert_if_missing()
|
||||
await client.async_start_pairing()
|
||||
return client
|
||||
return None, "protocol_unavailable"
|
||||
try:
|
||||
await client.async_generate_cert_if_missing()
|
||||
await client.async_start_pairing()
|
||||
except Exception as err: # noqa: BLE001 - the library's error IS the finding
|
||||
_LOGGER.warning(
|
||||
"protocol pairing could not start at %s (%s: %s) — showing the "
|
||||
"form so the address can be corrected or pairing skipped",
|
||||
host,
|
||||
type(err).__name__,
|
||||
err,
|
||||
)
|
||||
with contextlib.suppress(Exception):
|
||||
client.disconnect()
|
||||
return None, "protocol_unreachable"
|
||||
return client, None
|
||||
|
||||
async def _async_begin_protocol_pairing(self):
|
||||
"""Start a PIN pairing session: ``(client, None)`` or ``(None, error)``."""
|
||||
if not self._host():
|
||||
return None, "protocol_no_host"
|
||||
client = await self._start_protocol_pairing()
|
||||
if client is None:
|
||||
return None, "protocol_unavailable"
|
||||
return client, None
|
||||
return await self._start_protocol_pairing()
|
||||
|
||||
def _show_protocol_pair_form(self, errors=None):
|
||||
"""protocol_pair form: the PIN, the address, and the skip escape.
|
||||
|
||||
@@ -67,6 +67,7 @@
|
||||
"protocol_no_host": "No IP address for the Shield yet. Fill in the host field above (a discovered Shield is pre-filled automatically).",
|
||||
"protocol_pair_failed": "The Shield rejected that pairing code. Check the code on the Shield and select Submit to try again.",
|
||||
"protocol_unavailable": "The androidtvremote2 library is not installed, so the remote protocol cannot pair. Install it and restart Home Assistant, or turn on 'Continue without protocol pairing' to finish setup anyway.",
|
||||
"protocol_unreachable": "The Shield did not answer at that address. Check the address above and that the Shield is awake, then select Submit to try again \u2014 or turn on 'Continue without protocol pairing' to finish setup without it.",
|
||||
"no_state_source": "No state source selected. Choose at least one: 'Add the Nvidia Remote Protocol' or 'Read state over ADB'.",
|
||||
"invalid_app_map": "Each line must be package=name with a non-empty package (no spaces) and a non-empty name \u2014 for example ar.tvplayer.tv=TiviMate.",
|
||||
"invalid_digit_delay": "The digit delay must be between 0.05 and 1.0 seconds."
|
||||
|
||||
@@ -67,6 +67,7 @@
|
||||
"protocol_no_host": "No IP address for the Shield yet. Fill in the host field above (a discovered Shield is pre-filled automatically).",
|
||||
"protocol_pair_failed": "The Shield rejected that pairing code. Check the code on the Shield and select Submit to try again.",
|
||||
"protocol_unavailable": "The androidtvremote2 library is not installed, so the remote protocol cannot pair. Install it and restart Home Assistant, or turn on 'Continue without protocol pairing' to finish setup anyway.",
|
||||
"protocol_unreachable": "The Shield did not answer at that address. Check the address above and that the Shield is awake, then select Submit to try again \u2014 or turn on 'Continue without protocol pairing' to finish setup without it.",
|
||||
"no_state_source": "No state source selected. Choose at least one: 'Add the Nvidia Remote Protocol' or 'Read state over ADB'.",
|
||||
"invalid_app_map": "Each line must be package=name with a non-empty package (no spaces) and a non-empty name \u2014 for example ar.tvplayer.tv=TiviMate.",
|
||||
"invalid_digit_delay": "The digit delay must be between 0.05 and 1.0 seconds."
|
||||
|
||||
+52
-2
@@ -172,9 +172,45 @@ class RemoteEntity:
|
||||
return None
|
||||
|
||||
|
||||
#: Every face install() registers. The guard below checks the WHOLE set, not one
|
||||
#: sentinel: test files scope their stubs per file (snapshot + pop at teardown, the
|
||||
#: t7 discipline), so one file can leave `homeassistant.components.media_player`
|
||||
#: behind while another name it popped — e.g. `voluptuous` — is gone. A sentinel
|
||||
#: guard then made install() a no-op in the next file, and whichever module needed
|
||||
#: the missing face failed with ModuleNotFoundError depending on file order (t44).
|
||||
_STUB_FACES = (
|
||||
"voluptuous",
|
||||
"homeassistant",
|
||||
"homeassistant.config_entries",
|
||||
"homeassistant.core",
|
||||
"homeassistant.const",
|
||||
"homeassistant.exceptions",
|
||||
"homeassistant.components",
|
||||
"homeassistant.components.media_player",
|
||||
"homeassistant.components.remote",
|
||||
"homeassistant.components.button",
|
||||
"homeassistant.components.sensor",
|
||||
"homeassistant.helpers",
|
||||
"homeassistant.helpers.event",
|
||||
"homeassistant.helpers.entity",
|
||||
"homeassistant.helpers.entity_platform",
|
||||
"homeassistant.helpers.device_registry",
|
||||
"homeassistant.helpers.entity_registry",
|
||||
"homeassistant.helpers.redact",
|
||||
"homeassistant.helpers.selector",
|
||||
)
|
||||
|
||||
|
||||
def install() -> None:
|
||||
"""Register the stub modules (idempotent)."""
|
||||
if "homeassistant.components.media_player" in sys.modules:
|
||||
"""Register the stub modules (idempotent, and self-healing).
|
||||
|
||||
Callers must be able to rely on this regardless of what a previous test file
|
||||
popped: a missing face rebuilds the whole set, so any subset or file order
|
||||
works. A complete set is left alone — a file that installed richer faces of
|
||||
its own (tests/test_config_flow_ux.py's voluptuous mirror, the discovery
|
||||
stub) keeps them.
|
||||
"""
|
||||
if all(name in sys.modules for name in _STUB_FACES):
|
||||
return
|
||||
|
||||
vol = types.ModuleType("voluptuous")
|
||||
@@ -455,6 +491,20 @@ def install() -> None:
|
||||
sys.modules["homeassistant.helpers.event"] = event
|
||||
sys.modules["homeassistant.helpers.entity"] = entity
|
||||
sys.modules["homeassistant.helpers.entity_platform"] = entity_platform
|
||||
|
||||
# t44: config_flow imports `selector` from this package, and the per-file stub
|
||||
# installers register it as its own face. Installing it here too keeps
|
||||
# `_STUB_FACES` honest: install() then really does provide every face a test
|
||||
# file may have popped, which is what its guard checks.
|
||||
selector = types.ModuleType("homeassistant.helpers.selector")
|
||||
selector.EntitySelector = lambda *args, **kwargs: None
|
||||
selector.EntitySelectorConfig = lambda *args, **kwargs: ""
|
||||
selector.SelectSelector = lambda *args, **kwargs: None
|
||||
selector.SelectSelectorConfig = lambda **kwargs: ""
|
||||
selector.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown")
|
||||
helpers.selector = selector
|
||||
sys.modules["homeassistant.helpers.selector"] = selector
|
||||
|
||||
sys.modules["homeassistant.helpers"] = helpers
|
||||
|
||||
|
||||
|
||||
@@ -282,21 +282,32 @@ class FakeHass:
|
||||
return self.async_create_task(coro)
|
||||
|
||||
|
||||
class CannotConnectStubError(ConnectionError):
|
||||
"""androidtvremote2``CannotConnect``: the Shield never answered (t44 F1)."""
|
||||
|
||||
|
||||
class FakeProtocolClient:
|
||||
"""Deterministic androidtvremote2 stand-in for the PIN pairing path."""
|
||||
|
||||
def __init__(self, fail_code: bool = False):
|
||||
def __init__(self, fail_code: bool = False, start_raises: bool = False):
|
||||
self.cert_generated = False
|
||||
self.pairing_started = False
|
||||
self.finished_codes: list[str] = []
|
||||
self.disconnects = 0
|
||||
self._fail_code = fail_code
|
||||
# the Shield is asleep / rebooting / mis-addressed: the real library
|
||||
# raises from async_start_pairing, which used to escape the config flow
|
||||
self._start_raises = start_raises
|
||||
|
||||
async def async_generate_cert_if_missing(self):
|
||||
if self._start_raises:
|
||||
raise CannotConnectStubError("cert generation could not reach the Shield")
|
||||
self.cert_generated = True
|
||||
return True
|
||||
|
||||
async def async_start_pairing(self):
|
||||
if self._start_raises:
|
||||
raise CannotConnectStubError("no answer from the Shield")
|
||||
self.pairing_started = True
|
||||
return FAKE_PAIRING_CODE
|
||||
|
||||
@@ -1219,6 +1230,152 @@ class TestProtocolReconfigure:
|
||||
assert result == {"type": "abort", "reason": "reconfigure_entry_missing"}
|
||||
|
||||
|
||||
class TestUnreachableShieldDuringPairing:
|
||||
"""t44 F1 (high): an unreachable Shield is a FORM, never an exception.
|
||||
|
||||
The protocol transport is ON by default, so an asleep or rebooting Shield is
|
||||
the ordinary case: `_start_protocol_pairing` used to let androidtvremote2's
|
||||
CannotConnect escape `async_step_protocol_pair`, and the protocol_pair form —
|
||||
the address field AND the "continue without pairing" escape — never rendered.
|
||||
"""
|
||||
|
||||
def _raising(self, monkeypatch, module):
|
||||
return _patch_protocol(
|
||||
monkeypatch, module, client=FakeProtocolClient(start_raises=True)
|
||||
)
|
||||
|
||||
async def test_fresh_setup_shows_the_form_instead_of_raising(
|
||||
self, tmp_path, monkeypatch
|
||||
):
|
||||
module = _form_module()
|
||||
self._raising(monkeypatch, module)
|
||||
flow, _hass = _new_flow(module, tmp_path)
|
||||
|
||||
form = await flow.async_step_user(
|
||||
_user_input(
|
||||
**{
|
||||
CONF_ADB_ENABLED: False,
|
||||
CONF_PROTOCOL_ENABLED: True,
|
||||
CONF_HOST: DISCOVERED_HOST,
|
||||
}
|
||||
)
|
||||
)
|
||||
|
||||
assert form["type"] == "form"
|
||||
assert form["step_id"] == "protocol_pair"
|
||||
assert form["errors"] == {"base": "protocol_unreachable"}
|
||||
# both escapes are on the form: a corrected address and the skip
|
||||
fields = vol_mirror.declared_fields(form["data_schema"])
|
||||
assert CONF_HOST in fields
|
||||
assert CONF_SKIP_PAIRING in fields
|
||||
|
||||
async def test_the_adb_skip_route_into_the_protocol_step_also_shows_the_form(
|
||||
self, tmp_path, monkeypatch
|
||||
):
|
||||
"""Both transports are on by default, so ADB pairs first, then protocol."""
|
||||
module = _form_module()
|
||||
self._raising(monkeypatch, module)
|
||||
_patch_adb(monkeypatch, module)
|
||||
flow, _hass = _new_flow(module, tmp_path)
|
||||
|
||||
adb_form = await flow.async_step_user(
|
||||
_user_input(
|
||||
**{
|
||||
CONF_ADB_ENABLED: True,
|
||||
CONF_PROTOCOL_ENABLED: True,
|
||||
CONF_HOST: DISCOVERED_HOST,
|
||||
}
|
||||
)
|
||||
)
|
||||
assert adb_form["step_id"] == "adb_pair"
|
||||
|
||||
result = await flow.async_step_adb_pair({CONF_SKIP_PAIRING: True})
|
||||
|
||||
assert result["type"] == "form"
|
||||
assert result["step_id"] == "protocol_pair"
|
||||
assert result["errors"] == {"base": "protocol_unreachable"}
|
||||
|
||||
async def test_the_skip_path_still_finishes_the_entry(self, tmp_path, monkeypatch):
|
||||
"""The whole point of degrading to the form: setup stays completable."""
|
||||
module = _form_module()
|
||||
self._raising(monkeypatch, module)
|
||||
_patch_adb(monkeypatch, module)
|
||||
flow, _hass = _new_flow(module, tmp_path)
|
||||
|
||||
await flow.async_step_user(
|
||||
_user_input(
|
||||
**{
|
||||
CONF_ADB_ENABLED: True,
|
||||
CONF_PROTOCOL_ENABLED: True,
|
||||
CONF_HOST: DISCOVERED_HOST,
|
||||
}
|
||||
)
|
||||
)
|
||||
await flow.async_step_adb_pair({CONF_SKIP_PAIRING: True})
|
||||
|
||||
result = await flow.async_step_protocol_pair({CONF_SKIP_PAIRING: True})
|
||||
|
||||
assert result["type"] == "create_entry"
|
||||
assert result["options"][CONF_PROTOCOL_ENABLED] is False # passive
|
||||
assert result["options"][CONF_ADB_ENABLED] is True
|
||||
|
||||
async def test_reconfigure_survives_an_unreachable_shield(
|
||||
self, tmp_path, monkeypatch
|
||||
):
|
||||
module = _form_module()
|
||||
self._raising(monkeypatch, module)
|
||||
entry = SimpleNamespace(
|
||||
entry_id="e1",
|
||||
title="mdarcy",
|
||||
data={},
|
||||
options={
|
||||
CONF_PROTOCOL_ENABLED: False,
|
||||
CONF_ADB_ENABLED: True,
|
||||
CONF_HOST: "10.0.0.9",
|
||||
CONF_ADB_PORT: 5555,
|
||||
},
|
||||
unique_id=None,
|
||||
)
|
||||
flow, hass = _new_flow(module, tmp_path)
|
||||
hass.config_entries.by_id["e1"] = entry
|
||||
flow.context = {"entry_id": "e1", "source": "reconfigure"}
|
||||
|
||||
form = await flow.async_step_reconfigure()
|
||||
|
||||
assert form["type"] == "form"
|
||||
assert form["step_id"] == "protocol_pair"
|
||||
assert form["errors"] == {"base": "protocol_unreachable"}
|
||||
|
||||
async def test_reauth_survives_an_unreachable_shield(self, tmp_path, monkeypatch):
|
||||
module = _form_module()
|
||||
self._raising(monkeypatch, module)
|
||||
entry = SimpleNamespace(
|
||||
entry_id="e1",
|
||||
title="mdarcy",
|
||||
data={},
|
||||
options={CONF_PROTOCOL_ENABLED: True, CONF_HOST: "10.0.0.9"},
|
||||
)
|
||||
flow, hass = _new_flow(module, tmp_path)
|
||||
hass.config_entries.by_id["e1"] = entry
|
||||
flow.context = {"entry_id": "e1"}
|
||||
|
||||
shown = await flow.async_step_reauth({})
|
||||
assert shown["step_id"] == "reauth_confirm"
|
||||
|
||||
form = await flow.async_step_reauth_confirm({"confirm": True})
|
||||
|
||||
assert form["type"] == "form"
|
||||
assert form["step_id"] == "protocol_pair"
|
||||
assert form["errors"] == {"base": "protocol_unreachable"}
|
||||
|
||||
def test_the_new_error_key_is_documented_in_both_string_files(self):
|
||||
"""A form error with no copy renders as a bare key in the UI."""
|
||||
for file in (_strings(), _en_strings()):
|
||||
text = file["config"]["error"]["protocol_unreachable"]
|
||||
assert "did not answer" in text
|
||||
assert "Continue without protocol pairing" in text
|
||||
|
||||
|
||||
class TestCopyContract:
|
||||
"""F-C: plain, self-consistent copy with no unreachable-step pointers."""
|
||||
|
||||
|
||||
Reference in new issue
Block a user