diff --git a/tests/entity_stublib.py b/tests/entity_stublib.py index 933b928..80f11c1 100644 --- a/tests/entity_stublib.py +++ b/tests/entity_stublib.py @@ -471,6 +471,11 @@ def load_entity_modules() -> dict: "media_player", "remote", "diagnostics", + # t41: button.py was the file nobody loaded — a deleted helper it still + # imported shipped silently. It is a platform like the others, so it + # belongs here; tests/test_module_coverage.py sweeps EVERY module, which + # is what keeps this list from having to be complete. + "button", "init", ): # "init" maps the integration __init__.py (t29 G8 async_setup) if name == "init" and "nsh.init" in sys.modules: diff --git a/tests/ha_api_drift.py b/tests/ha_api_drift.py index d28bb31..3e00429 100644 --- a/tests/ha_api_drift.py +++ b/tests/ha_api_drift.py @@ -37,7 +37,17 @@ dependency against an INSTALLED homeassistant: raises ``TypeError: 'method' object is not iterable`` on every state write and the entity can never render (t40 live install: ``media_player.py:828``). The stub suites call the method happily; this pass is pure AST and, unlike 1-3, - needs no homeassistant at all. + needs no homeassistant at all; +5. the package's OWN relative imports (t41): ``from .helpers import X`` must + resolve to a file of this package that defines ``X`` (a submodule counts). + ``button.py`` imported a helper that a refactor had deleted, and because no + harness loaded that file the ImportError waited for a user's log — pass 1 + cannot see it (it only judges ``homeassistant.*``). Also pure AST, with its + own entry point ``scan_package_imports()`` for the fast suite. + +Both AST passes run before the homeassistant availability check, and the file +walk is recursive (``_iter_python_files``), so a platform or helper one +directory down cannot hide from either of them. Known limits (deliberate, and why the sweep still catches the real classes): ``**kwargs`` splats cannot be checked statically, calls on objects other than @@ -106,8 +116,11 @@ HA_CALL_OWNERS: dict[str, tuple[str, str]] = { #: 60 imports, 35 checked arguments, 7 awaited HA calls) so a broken or #: mis-targeted walk fails loudly instead of printing zeros and exiting 0. FLOORS: dict[str, int] = { - "modules": 10, + # t41: the sweep is recursive, so the floor sits above the top-level-only + # count (13) — a regression to the shallow glob trips it. + "modules": 20, "imports": 40, + "relative_imports": 100, "call_args": 20, "awaited_calls": 5, } @@ -143,6 +156,7 @@ class Report: findings: list[Finding] = field(default_factory=list) modules: int = 0 imports: int = 0 + relative_imports: int = 0 call_args: int = 0 awaited_calls: int = 0 @@ -164,6 +178,7 @@ class Report: return ( "no Home-Assistant API drift " f"({self.modules} modules, {self.imports} imports, " + f"{self.relative_imports} relative imports, " f"{self.call_args} call arguments, " f"{self.awaited_calls} awaited HA calls)" ) @@ -173,6 +188,19 @@ class Report: # --------------------------------------------------------------------- imports +def _iter_python_files(root: Path) -> list[Path]: + """Every Python file under a target, recursively (t41). + + The sweep used to glob one level, so a platform or helper one directory down + was invisible to both passes. Exhaustive-by-construction beats a list. + """ + if not root.is_dir(): + return [root] + return sorted( + path for path in root.rglob("*.py") if "__pycache__" not in path.parts + ) + + def _check_import(module_name: str, attr: str | None) -> str | None: """Return a reason string when the import target is missing, else None.""" try: @@ -230,6 +258,206 @@ def _scan_imports(tree: ast.AST, path: str, report: Report) -> None: ) +# ------------------------------------------------------- the package's own imports +# +# Pure AST, no Home Assistant required — the same contract as the entity pass, so +# the fast suite can prove it even in the androidtvremote2-less variant. + + +def _package_root(path: Path) -> Path: + """The integration package a file belongs to. + + The topmost ancestor directory that still has an ``__init__.py``: that is + ``custom_components/nvidia_shield`` for both its top-level modules and the + vendored ``ha_adb_common`` subpackage, and it holds for a copied tree too + (tests scan temp copies). + """ + root = path.parent + while (root.parent / "__init__.py").exists(): + root = root.parent + return root + + +class _ModuleLevelNames(ast.NodeVisitor): + """Names a module binds at import time. + + Functions, classes, assignments and imports — including the ones inside + ``if``/``try`` guards (the optional androidtvremote2 import lives in one). + Function and class BODIES are skipped: a name bound at call time is not + importable, which is exactly the distinction this pass needs. + """ + + def __init__(self) -> None: + self.names: set[str] = set() + + def visit_FunctionDef(self, node: ast.AST) -> None: + self.names.add(node.name) + + def visit_AsyncFunctionDef(self, node: ast.AST) -> None: + self.names.add(node.name) + + def visit_ClassDef(self, node: ast.AST) -> None: + self.names.add(node.name) + + def visit_Import(self, node: ast.Import) -> None: + for alias in node.names: + self.names.add(alias.asname or alias.name.split(".")[0]) + + def visit_ImportFrom(self, node: ast.ImportFrom) -> None: + for alias in node.names: + if alias.name != "*": + self.names.add(alias.asname or alias.name) + + def _bind(self, target: ast.AST) -> None: + if isinstance(target, ast.Name): + self.names.add(target.id) + elif isinstance(target, (ast.Tuple, ast.List)): + for element in target.elts: + self._bind(element) + elif isinstance(target, ast.Starred): + self._bind(target.value) + + def visit_Assign(self, node: ast.Assign) -> None: + for target in node.targets: + self._bind(target) + + def visit_AnnAssign(self, node: ast.AnnAssign) -> None: + self._bind(node.target) + + def visit_AugAssign(self, node: ast.AugAssign) -> None: + self._bind(node.target) + + def visit_For(self, node: ast.For) -> None: + self._bind(node.target) + self.generic_visit(node) + + def visit_AsyncFor(self, node: ast.AsyncFor) -> None: + self._bind(node.target) + self.generic_visit(node) + + def visit_With(self, node: ast.With) -> None: + for item in node.items: + if item.optional_vars is not None: + self._bind(item.optional_vars) + self.generic_visit(node) + + def visit_AsyncWith(self, node: ast.AsyncWith) -> None: + for item in node.items: + if item.optional_vars is not None: + self._bind(item.optional_vars) + self.generic_visit(node) + + +def _module_level_names(tree: ast.AST) -> set[str]: + visitor = _ModuleLevelNames() + visitor.visit(tree) + return visitor.names + + +def _resolve_relative_import(path: Path, node: ast.ImportFrom, root: Path) -> Path | None: + """The file ``from ... import`` points at, or None when it leaves the package.""" + base = path.parent + for _ in range(node.level - 1): + base = base.parent + if not base.is_relative_to(root): + return None + if not node.module: + return base / "__init__.py" + target = base.joinpath(*node.module.split(".")) + if target.is_dir(): + return target / "__init__.py" + return target.with_suffix(".py") + + +def _scan_relative_imports(path: Path, tree: ast.AST, report: Report) -> None: + """t41: resolve the integration's OWN imports, not only ``homeassistant.*``. + + ``_scan_imports`` proves the HA surface; it says nothing about + ``from .helpers import discover_adb_host`` once that helper is deleted — the + exact defect that shipped from ``button.py`` because no harness loaded the + file. This pass is pure AST against the tree, so it needs no importable + Home Assistant and cannot be masked by a missing stub: the target file must + exist and must define every name imported from it (a submodule counts too). + """ + root = _package_root(path) + for node in ast.walk(tree): + if not isinstance(node, ast.ImportFrom) or not node.level: + continue # absolute imports are _scan_imports' business + spelling = f"{'.' * node.level}{node.module or ''}" + target = _resolve_relative_import(path, node, root) + if target is None: + report.findings.append( + Finding( + str(path), + node.lineno, + "broken-relative-import", + f"from {spelling} import ... escapes the package — the " + "integration may only import from itself", + ) + ) + continue + if not target.exists(): + report.findings.append( + Finding( + str(path), + node.lineno, + "broken-relative-import", + f"from {spelling} import ...: no module at " + f"{target.relative_to(root)}", + ) + ) + continue + defined = _module_level_names( + ast.parse(target.read_text(encoding="utf-8"), filename=str(target)) + ) + for alias in node.names: + if alias.name == "*": + continue + report.relative_imports += 1 + if alias.name in defined: + continue + # `from .package import submodule` is valid without the name being + # bound in __init__ — the import system resolves the submodule. + sibling = target.parent / f"{alias.name}.py" + sibling_package = target.parent / alias.name / "__init__.py" + if sibling.exists() or sibling_package.exists(): + continue + report.findings.append( + Finding( + str(path), + node.lineno, + "broken-relative-import", + f"from {spelling} import {alias.name}: not defined in " + f"{target.relative_to(root)}", + ) + ) + + +def _scan_package_imports(paths, report: Report) -> None: + """Walk the tree and resolve every one of the package's own imports.""" + for root in paths: + root = Path(root) + if not root.exists(): + continue # scan() reports bad-path for the same target + for file in _iter_python_files(root): + try: + tree = ast.parse(file.read_text(encoding="utf-8"), filename=str(file)) + except (OSError, SyntaxError): + continue + _scan_relative_imports(file, tree, report) + + +def scan_package_imports(paths=None) -> list[Finding]: + """The relative-import pass on its own (t41) — the fast suite's entry point.""" + if paths is None: + paths = [SHIELD_DIR] + elif isinstance(paths, (str, Path)): + paths = [paths] + report = Report() + _scan_package_imports(paths, report) + return report.findings + + # -------------------------------------------------------------------- keywords @@ -586,7 +814,7 @@ def _scan_entity_contracts(paths, report: Report) -> None: root = Path(root) if not root.exists(): continue # scan() reports bad-path for the same target - files = sorted(root.glob("*.py")) if root.is_dir() else [root] + files = _iter_python_files(root) for file in files: try: tree = ast.parse(file.read_text(encoding="utf-8"), filename=str(file)) @@ -639,9 +867,11 @@ def scan(paths=None) -> Report: paths = [paths] report = Report() - # t40: pure-AST shape check, so it runs even without homeassistant — that is - # the whole point of a contract stubs cannot enforce. + # t40/t41: pure-AST checks, so they run even without homeassistant — that is + # the whole point of a contract stubs cannot enforce (and a relative import + # needs no library to be wrong). _scan_entity_contracts(paths, report) + _scan_package_imports(paths, report) if not homeassistant_available(): # Neither a false green nor a wall of phantom findings: with no package # to resolve against, every import would "fail", so the sweep reports @@ -671,7 +901,7 @@ def scan(paths=None) -> Report: ) ) continue - files = sorted(root.glob("*.py")) if root.is_dir() else [root] + files = _iter_python_files(root) for file in files: report.modules += 1 tree = ast.parse(file.read_text(encoding="utf-8"), filename=str(file)) diff --git a/tests/test_entity_contract.py b/tests/test_entity_contract.py index 05a0176..309196b 100644 --- a/tests/test_entity_contract.py +++ b/tests/test_entity_contract.py @@ -108,11 +108,17 @@ def test_the_sweep_reports_the_same_findings_as_this_check(tmp_path): report = ha_api_drift.scan(tmp_path) kinds = [finding.kind for finding in report.findings] - # The contract pass runs FIRST (pure AST). Without homeassistant installed the - # sweep then adds "cannot judge the API surface" — that path is covered in - # tests/test_ha_api_drift.py. + # The contract pass runs FIRST (pure AST). What the API passes add after it + # depends on the interpreter: with homeassistant installed they judge the + # module's imports, without it the sweep says it cannot judge (both are + # covered here so the check holds in either variant). assert kinds[0] == "entity-attribute-is-a-method" - assert "no-homeassistant" in kinds + rest = set(kinds[1:]) + if ha_api_drift.homeassistant_available(): + # the API passes ran and found nothing wrong with this module + assert rest == set(), rest + else: + assert "no-homeassistant" in rest def test_the_check_ignores_missing_paths_instead_of_raising(): diff --git a/tests/test_module_coverage.py b/tests/test_module_coverage.py new file mode 100644 index 0000000..8159a3a --- /dev/null +++ b/tests/test_module_coverage.py @@ -0,0 +1,198 @@ +"""tests/test_module_coverage.py — nothing in the package may dangle (t41). + +Two defects motivated this file, and both shipped with a green suite: + +1. ``button.py`` imported a helper that had been deleted. No harness ever loaded + that file (``entity_stublib.load_entity_modules`` lists the platforms it + knows), so the ImportError would have appeared only in a user's log. The fix + is exhaustive by construction: the sweep walks the package for ``*.py`` files + in a clean interpreter, under the stub harness, and loads every one of them. + A new platform is covered the moment the file exists — there is no list to + update and no special case. + +2. The drift sweep resolved ``homeassistant.*`` imports only, so it could not + see a relative import of a name that no longer exists — the same defect one + layer up. ``ha_api_drift.scan_package_imports`` now resolves the package's own + imports from the AST, with no Home Assistant and therefore no stub needed. + +Each sweep carries its own red proof: a copy of the tree with the reverted +``discover_adb_host`` reference must fail it, or the check is decoration. +""" + +import shutil +import subprocess +import sys +from pathlib import Path + +import ha_api_drift +import pytest +from conftest import _SHIELD_DIR + +TESTS_DIR = Path(__file__).resolve().parent +REPO_ROOT = _SHIELD_DIR.parent.parent + +#: Loads every module of a package under the stub harness, in a clean process. +#: ``argv``: 1 = this tests directory, 2 = the package directory to sweep. +_DRIVER = r""" +import sys +from pathlib import Path + +tests_dir, package_root = Path(sys.argv[1]), Path(sys.argv[2]) +sys.path.insert(0, str(tests_dir)) +sys.path.insert(0, str(package_root)) + +import entity_stublib # noqa: E402 +from conftest import _load_module # noqa: E402 + +# conftest eagerly loads a few stdlib-only modules from the REAL tree (it cannot +# know about a copied one): point the synthetic package at the tree under test +# and drop what it imported, so every relative import resolves inside the copy. +sys.modules["nsh"].__path__ = [str(package_root)] +for name in [n for n in list(sys.modules) if n.startswith("nsh.")]: + del sys.modules[name] + +entity_stublib.install() + +loaded, failures = [], [] +for path in sorted(package_root.rglob("*.py")): + if "__pycache__" in path.parts: + continue + rel = path.relative_to(package_root) + if rel.name == "__init__.py": + dotted = ".".join(rel.parent.parts) + name = "nsh.init" if dotted in ("", ".") else f"nsh.{dotted}" + else: + name = "nsh." + ".".join(rel.with_suffix("").parts) + search = [str(path.parent)] if rel.name == "__init__.py" else None + try: + _load_module(name, path, search) + except Exception as exc: # noqa: BLE001 - the error IS the finding + failures.append(f"{rel}: {type(exc).__name__}: {exc}") + else: + loaded.append(str(rel)) + +for rel in loaded: + print(f"OK {rel}") +for failure in failures: + print(f"FAIL {failure}") +print(f"SUMMARY loaded={len(loaded)} failed={len(failures)} total={len(loaded) + len(failures)}") +sys.exit(1 if failures else 0) +""" + + +def _package_modules(root: Path) -> set[str]: + """Every Python file of a package, as the sweep reports them.""" + return { + str(path.relative_to(root)) + for path in root.rglob("*.py") + if "__pycache__" not in path.parts + } + + +def _sweep(root: Path) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, "-c", _DRIVER, str(TESTS_DIR), str(root)], + capture_output=True, + text=True, + cwd=str(REPO_ROOT), + check=False, + ) + + +def _reverted_copy(tmp_path: Path) -> Path: + """A copy of the package where button.py imports the deleted helper again.""" + target = tmp_path / "nvidia_shield" + shutil.copytree( + _SHIELD_DIR, + target, + ignore=shutil.ignore_patterns("__pycache__", "*.json", "*.png"), + ) + button = target / "button.py" + button.write_text( + "from .helpers import discover_adb_host # the reverted reference\n" + + button.read_text(encoding="utf-8"), + encoding="utf-8", + ) + return target + + +def test_every_module_imports_under_the_stub_harness(): + """The whole package loads in a clean interpreter — no module is skipped.""" + result = _sweep(_SHIELD_DIR) + output = result.stdout + result.stderr + + assert result.returncode == 0, output + reported = { + line.split(" ", 1)[1] + for line in result.stdout.splitlines() + if line.startswith(("OK ", "FAIL ")) + } + assert reported == _package_modules(_SHIELD_DIR), "the sweep skipped or invented a file" + assert "failed=0" in result.stdout, output + + +def test_the_sweep_fails_on_a_reverted_helper_reference(tmp_path): + """Red proof 1: the exact defect that motivated this check. + + button.py imported ``discover_adb_host`` from helpers after that helper was + deleted (t41 removed it with the entity path). The sweep must fail loudly. + """ + result = _sweep(_reverted_copy(tmp_path)) + output = result.stdout + result.stderr + + assert result.returncode != 0, output + assert "discover_adb_host" in output + assert "FAIL button.py" in result.stdout + + +def test_every_relative_import_resolves(): + """The same contract, statically: the package's own imports all resolve.""" + findings = ha_api_drift.scan_package_imports() + + assert findings == [], "\n".join(str(finding) for finding in findings) + + +def test_the_relative_import_pass_is_not_vacuous(): + """A pass that resolved nothing would be green for the wrong reason.""" + report = ha_api_drift.Report() + + ha_api_drift._scan_package_imports([_SHIELD_DIR], report) + + assert report.relative_imports >= ha_api_drift.FLOORS["relative_imports"] + # and the walk itself reached every module, not only the top level (the + # floor sits above the top-level-only count, so a shallow glob trips it) + assert len(_package_modules(_SHIELD_DIR)) >= ha_api_drift.FLOORS["modules"] + + +def test_the_static_sweep_fails_on_a_reverted_helper_reference(tmp_path): + """Red proof 2: the AST pass sees the same defect, with no Home Assistant.""" + findings = ha_api_drift.scan_package_imports(_reverted_copy(tmp_path)) + + assert [finding.kind for finding in findings] == ["broken-relative-import"] + assert "discover_adb_host" in findings[0].detail + assert findings[0].path.endswith("button.py") + + +@pytest.mark.parametrize( + ("source", "expected"), + [ + # a name nobody defines + ("from .helpers import no_such_helper\n", "broken-relative-import"), + # a relative import that leaves the package + ("from ..sibling import thing\n", "broken-relative-import"), + # a submodule import is valid without a binding in __init__ + ("from .ha_adb_common import adb_client\n", None), + ], +) +def test_the_pass_judges_the_cases_that_matter(tmp_path, source, expected): + package = tmp_path / "nvidia_shield" + (package / "ha_adb_common").mkdir(parents=True) + (package / "__init__.py").write_text("", encoding="utf-8") + (package / "helpers.py").write_text("def entry_host(entry):\n return ''\n", encoding="utf-8") + (package / "ha_adb_common" / "__init__.py").write_text("", encoding="utf-8") + (package / "ha_adb_common" / "adb_client.py").write_text("", encoding="utf-8") + (package / "module.py").write_text(source, encoding="utf-8") + + findings = ha_api_drift.scan_package_imports(package) + + assert [finding.kind for finding in findings] == ([expected] if expected else [])