From 2e1d162ef4478cf90ecabc7fc48d7381308fc5ab Mon Sep 17 00:00:00 2001 From: James Mackay Date: Sun, 11 Oct 2026 06:41:56 +0100 Subject: [PATCH] test(harness): make the stub scoping independent of file order (t44) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A subset of test files in an order the suite was not written for failed 3 test_init.py::TestImportStep tests with ModuleNotFoundError: voluptuous, while the full suite stayed green. Two order-only mechanisms, both now fixed at the source: 1. entity_stublib.install() guarded on ONE sentinel (`homeassistant.components.media_player`). A file whose teardown popped `voluptuous` but left the sentinel turned the next file's install() into a no-op, so importing config_flow failed. The guard now checks every face a file scopes on (`_STUB_FACES`, derived from ENTITIES_STUB_NAMES so the two cannot drift) and rebuilds the set when anything is missing. install() also registers the `homeassistant.helpers.selector` face itself, which the per-file installers had been adding separately — without it the derived list would never be complete. 2. Teardowns restored only the names their file had listed, so a module the file loaded (`nsh.diagnostics`, `nsh.media_player`) survived into the next file still holding stub objects — a stub exception class, the redact helper — that had since been replaced; comparisons against the new objects then failed (5 tests in test_command_routing on an unlucky order). Every file now ends through entity_stublib.snapshot_modules()/restore_modules(), which drops EVERYTHING the file created under the harness namespaces (homeassistant/voluptuous/nsh) and puts back everything it replaced. Hand-written lists were the bug, so there is none to keep in sync. The t7 no-leak discipline is unchanged: a file's stubs still never reach another file, and a file's loaded modules still do not survive it. A session-scoped baseline install was tried first and rejected: files whose own installer guards on "the stubs are already there" skipped installing their richer faces, which produced exactly the inconsistent state a baseline is meant to prevent (10 test_standalone_setup failures). tests/test_harness_scoping.py: the sentinel poisoning, the wholesale-rebuild invariant, a teardown that removes what the file loaded, the stale-object case (diagnostics vs the redact helper), and — because order is the thing under test — two subprocess runs of the reported orderings. Verified: the reported order (test_command_routing, test_protocol_transport, test_standalone_setup, test_init) 152 passed / 1 skipped; the minimal pair both ways; the previously failing 6-test order now 167 passed; every test file in REVERSE alphabetical order 369 passed / 2 skipped; two scrambled thirds green; full suite 369 / 2 and 370 / 1 with androidtvremote2; ruff clean. --- tests/conftest.py | 1 + tests/entity_stublib.py | 54 +++++---- tests/test_command_routing.py | 11 +- tests/test_config_flow_pairing.py | 10 +- tests/test_config_flow_protocol.py | 10 +- tests/test_config_flow_ux.py | 10 +- tests/test_harness_scoping.py | 181 +++++++++++++++++++++++++++++ tests/test_init.py | 9 +- tests/test_standalone_setup.py | 10 +- 9 files changed, 233 insertions(+), 63 deletions(-) create mode 100644 tests/test_harness_scoping.py diff --git a/tests/conftest.py b/tests/conftest.py index 6dd2a1f..db83453 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -50,3 +50,4 @@ _load_module( # Stdlib-only integration modules. for _mod in ("const", "adb_session", "connection"): _load_module(f"{_PKG}.{_mod}", _SHIELD_DIR / f"{_mod}.py") + diff --git a/tests/entity_stublib.py b/tests/entity_stublib.py index 3911371..f96a80e 100644 --- a/tests/entity_stublib.py +++ b/tests/entity_stublib.py @@ -178,26 +178,11 @@ class RemoteEntity: #: 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", +#: The faces install() provides, derived from the list every test file scopes on. +#: The ``nsh.*`` entries there are the integration modules the harness loads, not +#: stub faces, so they are filtered out here. +_STUB_FACES = tuple( + name for name in ENTITIES_STUB_NAMES if not name.startswith("nsh.") ) @@ -508,6 +493,35 @@ def install() -> None: sys.modules["homeassistant.helpers"] = helpers +#: Namespaces the stub harness owns. Anything under these prefixes that a test +#: file creates is harness state, so a file's teardown must remove it: a module +#: left behind keeps the stub objects it captured (an exception class, the redact +#: helper the diagnostics module holds) after the next file rebuilt them, which is +#: how a file-order-dependent failure looked (t44). +_TRACKED_PREFIXES = ("homeassistant", "voluptuous", "nsh") + + +def _tracked(name: str) -> bool: + return name.split(".", 1)[0] in _TRACKED_PREFIXES + + +def snapshot_modules() -> dict: + """Every stub/module name live right now — for a leak-free teardown.""" + return {name: module for name, module in sys.modules.items() if _tracked(name)} + + +def restore_modules(snapshot: dict) -> None: + """Undo everything the current test file did to those namespaces. + + Two halves, and the first is what a hand-written list of names kept getting + wrong: drop every tracked name the file created (so the next file loads it + fresh, against ITS stubs), then put back every object it replaced. + """ + for name in [name for name in sys.modules if _tracked(name) and name not in snapshot]: + del sys.modules[name] + sys.modules.update(snapshot) + + def load_entity_modules() -> dict: """Load the integration's HA-free-ish entity modules under nsh.""" install() diff --git a/tests/test_command_routing.py b/tests/test_command_routing.py index a5e6dd3..b350557 100644 --- a/tests/test_command_routing.py +++ b/tests/test_command_routing.py @@ -13,7 +13,7 @@ import sys import entity_stublib import pytest from conftest import _SHIELD_DIR -from entity_stublib import ENTITIES_STUB_NAMES, install, load_entity_modules +from entity_stublib import install, load_entity_modules from nsh.connection import ShieldConnectionManager from nsh.const import TIVIMATE_PACKAGE @@ -21,15 +21,10 @@ from nsh.const import TIVIMATE_PACKAGE @pytest.fixture(autouse=True) def scoped_entity_stubs(): """Container for the HA stubs + loaded entity modules (t7 discipline).""" - snapshot = {name: sys.modules.get(name) for name in ENTITIES_STUB_NAMES} + snapshot = entity_stublib.snapshot_modules() install() yield load_entity_modules() - for name in ENTITIES_STUB_NAMES: - original = snapshot[name] - if original is None: - sys.modules.pop(name, None) - else: - sys.modules[name] = original + entity_stublib.restore_modules(snapshot) class FakeProtocolClient: diff --git a/tests/test_config_flow_pairing.py b/tests/test_config_flow_pairing.py index 5a0b287..96ad0b7 100644 --- a/tests/test_config_flow_pairing.py +++ b/tests/test_config_flow_pairing.py @@ -11,6 +11,7 @@ import sys import types from unittest.mock import AsyncMock +import entity_stublib import pytest from conftest import _SHIELD_DIR, _load_module from nsh.const import ( @@ -187,15 +188,10 @@ def scoped_stubs(): teardown, so later tests never observe the fake homeassistant/voluptuous (nor the stub-loaded nsh.helpers/nsh.config_flow) modules. """ - snapshot = {name: sys.modules.get(name) for name in _STUB_NAMES} + snapshot = entity_stublib.snapshot_modules() _install_import_stubs() yield - for name in _STUB_NAMES: - original = snapshot[name] - if original is None: - sys.modules.pop(name, None) - else: - sys.modules[name] = original + entity_stublib.restore_modules(snapshot) @pytest.fixture diff --git a/tests/test_config_flow_protocol.py b/tests/test_config_flow_protocol.py index a62d2d0..c255d23 100644 --- a/tests/test_config_flow_protocol.py +++ b/tests/test_config_flow_protocol.py @@ -11,6 +11,7 @@ import sys import types from types import SimpleNamespace +import entity_stublib import pytest from conftest import _SHIELD_DIR, _load_module from nsh.const import ( @@ -278,15 +279,10 @@ def _entry_module(): @pytest.fixture(autouse=True) def scoped_stubs(): """Keep the import stubs inside this module's tests (t7 discipline).""" - snapshot = {name: sys.modules.get(name) for name in _STUB_NAMES} + snapshot = entity_stublib.snapshot_modules() _install_stubs() yield - for name in _STUB_NAMES: - original = snapshot[name] - if original is None: - sys.modules.pop(name, None) - else: - sys.modules[name] = original + entity_stublib.restore_modules(snapshot) def make_flow(tmp_dir: str, monkeypatch: pytest.MonkeyPatch, client: FakeProtocolClient): diff --git a/tests/test_config_flow_ux.py b/tests/test_config_flow_ux.py index 13cba8f..efe737f 100644 --- a/tests/test_config_flow_ux.py +++ b/tests/test_config_flow_ux.py @@ -26,6 +26,7 @@ from pathlib import Path from types import SimpleNamespace from unittest.mock import AsyncMock +import entity_stublib import pytest import voluptuous_mirror as vol_mirror from conftest import _SHIELD_DIR, _load_module @@ -331,17 +332,12 @@ def scoped_stubs(): another file left behind from shadowing the faithful voluptuous mirror this file's schema assertions depend on. """ - snapshot = {name: sys.modules.get(name) for name in _STUB_NAMES} + snapshot = entity_stublib.snapshot_modules() for name in _STUB_NAMES: sys.modules.pop(name, None) _install_stubs() yield - for name in _STUB_NAMES: - original = snapshot[name] - if original is None: - sys.modules.pop(name, None) - else: - sys.modules[name] = original + entity_stublib.restore_modules(snapshot) def _form_module(): diff --git a/tests/test_harness_scoping.py b/tests/test_harness_scoping.py new file mode 100644 index 0000000..bd8263f --- /dev/null +++ b/tests/test_harness_scoping.py @@ -0,0 +1,181 @@ +"""tests/test_harness_scoping.py — the stub harness must not depend on file order (t44). + +The failure this pins, as reported: a subset of test files in an order the suite was +not written for left three ``test_init.py::TestImportStep`` tests failing with +``ModuleNotFoundError: No module named 'voluptuous'``, while the full suite stayed +green. Two mechanisms produced it, and both are order-only: + +1. ``entity_stublib.install()`` trusted a single sentinel + (``homeassistant.components.media_player``). A file whose teardown popped + ``voluptuous`` while leaving the sentinel behind turned the next file's + ``install()`` into a no-op, so importing ``config_flow`` failed. +2. A file's teardown restored only the names it had listed. A module it loaded + (``nsh.diagnostics``, ``nsh.media_player``) therefore survived into the next + file still holding stub objects — an exception class, the redact helper — that + had since been replaced, and comparisons against the new objects failed. + +The fix keeps the t7 discipline (no stub leaks across files): ``install()`` checks +the whole face set and rebuilds when anything is missing, and every file's teardown +uses ``snapshot_modules()``/``restore_modules()``, which drops *everything* the file +created under the harness namespaces and puts back everything it replaced. + +The in-process tests cover both mechanisms directly; the subprocess cases re-run the +reported orderings end to end, because that is where the failure was visible. +""" + +import subprocess +import sys +from pathlib import Path + +import entity_stublib +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +TRACKED = ("homeassistant", "voluptuous", "nsh") + + +def _scoped_faces() -> list[str]: + """The HA/voluptuous faces every file scopes on (the nsh.* names are modules).""" + return [name for name in entity_stublib.ENTITIES_STUB_NAMES if not name.startswith("nsh.")] + + +def _run(*paths: str) -> subprocess.CompletedProcess: + return subprocess.run( + [ + sys.executable, + "-m", + "pytest", + *paths, + "-q", + "--no-header", + "-p", + "no:cacheprovider", + ], + cwd=REPO_ROOT, + capture_output=True, + text=True, + timeout=300, + ) + + +class TestStubFaceGuard: + """Mechanism 1: a partially popped face set must be repaired, not trusted.""" + + def test_install_provides_every_face_a_file_scopes_on(self): + """A face listed for scoping but never installed is a file-order trap.""" + snapshot = entity_stublib.snapshot_modules() + try: + entity_stublib.install() + + missing = [name for name in _scoped_faces() if name not in sys.modules] + + assert missing == [] + finally: + entity_stublib.restore_modules(snapshot) + + def test_a_complete_face_set_is_left_alone(self): + """The guard must still be an idempotence guard, not a rebuild.""" + snapshot = entity_stublib.snapshot_modules() + try: + entity_stublib.install() + before = {name: sys.modules[name] for name in _scoped_faces()} + + entity_stublib.install() + + assert {name: sys.modules[name] for name in _scoped_faces()} == before + finally: + entity_stublib.restore_modules(snapshot) + + def test_a_sentinel_left_behind_does_not_hide_a_missing_face(self): + """The reported poisoning: media_player present, voluptuous gone.""" + snapshot = entity_stublib.snapshot_modules() + try: + entity_stublib.install() + kept = sys.modules["homeassistant.components.media_player"] + del sys.modules["voluptuous"] + + entity_stublib.install() + + assert "voluptuous" in sys.modules + # the rebuild is wholesale, so the sentinel is a real face again + assert sys.modules["homeassistant.components.media_player"] is not None + assert kept is not None + finally: + entity_stublib.restore_modules(snapshot) + + +class TestTeardownLeaks: + """Mechanism 2: nothing a file created or replaced may survive its teardown.""" + + def test_teardown_removes_the_modules_a_file_loaded(self): + snapshot = entity_stublib.snapshot_modules() + try: + entity_stublib.install() + entity_stublib.load_entity_modules() + created = [ + name + for name in sys.modules + if name.startswith("nsh.") and name not in snapshot + ] + assert created, "premise: loading the entity modules creates nsh.* modules" + + entity_stublib.restore_modules(snapshot) + + assert [name for name in sys.modules if name.startswith("nsh.")] == [ + name for name in snapshot if name.startswith("nsh.") + ] + assert { + name: sys.modules.get(name) + for name in snapshot + if name.split(".", 1)[0] in TRACKED + } == snapshot + finally: + entity_stublib.restore_modules(snapshot) + + def test_a_reloaded_module_holds_the_current_stub_objects(self): + """The stale-object failure, exactly: diagnostics vs the redact helper.""" + snapshot = entity_stublib.snapshot_modules() + try: + entity_stublib.install() + stale_module = entity_stublib.load_entity_modules()["diagnostics"] + stale = stale_module.async_redact_data + entity_stublib.restore_modules(snapshot) # ...the file ends + + # ...the next file installs its own faces: a partial pop is how that + # actually looked, and it is what forces the rebuild + entity_stublib.install() + del sys.modules["homeassistant.helpers.redact"] + entity_stublib.install() + fresh_modules = entity_stublib.load_entity_modules() + + current = sys.modules["homeassistant.helpers.redact"].async_redact_data + assert stale is not current, "premise: the rebuild created a new helper" + assert fresh_modules["diagnostics"].async_redact_data is current + # ...and the stale module is not the one sitting in the import cache + assert sys.modules["nsh.diagnostics"] is fresh_modules["diagnostics"] + finally: + entity_stublib.restore_modules(snapshot) + + +@pytest.mark.parametrize( + "paths", + [ + # the reported reproduction, cut to the two files that trigger it + ("tests/test_standalone_setup.py", "tests/test_init.py::TestImportStep"), + # the other failure mode: a file that loads the entity modules before a + # file that compares them against freshly installed faces + ( + "tests/test_standalone_setup.py", + "tests/test_config_flow_protocol.py", + "tests/test_init.py", + "tests/test_command_routing.py::TestAdbCommandService", + "tests/test_command_routing.py::TestFaithfulHaStubs", + "tests/test_command_routing.py::TestServiceRegistryKey", + ), + ], +) +def test_arbitrary_file_orders_pass(paths): + """Order is the thing under test, so it is executed, not reasoned about.""" + result = _run(*paths) + + assert result.returncode == 0, result.stdout[-3000:] diff --git a/tests/test_init.py b/tests/test_init.py index ea4e34a..b44575b 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -39,19 +39,14 @@ _INIT_STUB_NAMES = ( @pytest.fixture(autouse=True) def _integration_module(): """HA stubs + the integration __init__ module under nsh.init.""" - snapshot = {name: sys.modules.get(name) for name in _INIT_STUB_NAMES} + snapshot = entity_stublib.snapshot_modules() entity_stublib.install() if "nsh.init" not in sys.modules: import conftest conftest._load_module("nsh.init", conftest._SHIELD_DIR / "__init__.py") yield sys.modules["nsh.init"] - for name in _INIT_STUB_NAMES: - original = snapshot[name] - if original is None: - sys.modules.pop(name, None) - else: - sys.modules[name] = original + entity_stublib.restore_modules(snapshot) class _SeedEntriesStub: diff --git a/tests/test_standalone_setup.py b/tests/test_standalone_setup.py index 8c1927d..8e87fb5 100644 --- a/tests/test_standalone_setup.py +++ b/tests/test_standalone_setup.py @@ -17,6 +17,7 @@ from pathlib import Path # noqa: E402 (F1 test key staging) from types import SimpleNamespace from unittest import mock +import entity_stublib import pytest import voluptuous_mirror as vol_mirror from conftest import _SHIELD_DIR, _load_module @@ -262,15 +263,10 @@ class FakeHass: @pytest.fixture(autouse=True) def scoped_stubs(): """Keep the import stubs inside this module's tests (t7 discipline).""" - snapshot = {name: sys.modules.get(name) for name in _STUB_NAMES} + snapshot = entity_stublib.snapshot_modules() _install_stubs() yield - for name in _STUB_NAMES: - original = snapshot[name] - if original is None: - sys.modules.pop(name, None) - else: - sys.modules[name] = original + entity_stublib.restore_modules(snapshot) def _form_module():