From a8dc0d1220851040a9c3e869f91ecf56ec957723 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Fri, 12 Jun 2026 15:45:16 -0700 Subject: [PATCH 1/2] Barrel-transparent test selection + import-distance ordering (#3182) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two selector changes so make test narrowing survives (and benefits from) the #3111 decomposition program: 1. Barrel transparency: packages whose __init__.py is a pure re-export barrel no longer reconstitute the decomposed file's full blast radius. When a submodule behind a barrel changes, consumers of the barrel join the reverse closure only if they statically use a symbol backed by a tainted submodule (from- imports, attribute access on module aliases, or dotted string literals such as mock.patch targets). Everything the analysis cannot bound — impure barrels, star imports, escaping module objects, unparsable sources, graphs without the direct-importers API — falls back to full taint, and a changed module whose transparent closure reaches zero tests falls back to its opaque closure (never-zero ratchet). The zero-downstream full-suite guard keeps its pre-existing opaque semantics. 2. Import-distance ordering: selected test files are emitted direct-importers-first (BFS depth from the changed modules, alphabetical within a tier) so a wide selection surfaces the most likely failure early; pytest collects in the order given. Already effective on main: 16 pure barrels exist today (e.g. shared.egg_config, shared.egg_contracts). Measured: a change to shared.egg_config.base narrows 228 -> 4 selected tests (exactly the tests/egg_config suite); shared.egg_contracts.agent_recovery narrows 59 -> 2. Closes #3182. --- docs/guides/testing.md | 31 +- scripts/select_tests/__init__.py | 19 +- scripts/select_tests/_cli.py | 33 +- scripts/select_tests/_graph.py | 639 ++++++++++++++++++++++-- tests/tools/test_select_tests_barrel.py | 560 +++++++++++++++++++++ 5 files changed, 1228 insertions(+), 54 deletions(-) create mode 100644 tests/tools/test_select_tests_barrel.py diff --git a/docs/guides/testing.md b/docs/guides/testing.md index a06a542448..2e67ba71f4 100644 --- a/docs/guides/testing.md +++ b/docs/guides/testing.md @@ -110,6 +110,30 @@ The algorithm is: gateway production module importable by bare name), so the AST resolver bridges those edges the same way it does for the sys.path-injected packages. See §7 for the full rationale. + + **Barrel transparency (#3182).** Packages whose `__init__.py` is a + *pure re-export barrel* (the shape mandated by + `docs/guides/decomposition-pattern.md`: docstring, imports — + including the `try/except ImportError` dual-import idiom — and an + `__all__` of string constants, nothing else) are treated + transparently by the walk. When a submodule behind such a barrel + changes, a consumer of the barrel joins the closure only if its + source statically uses a symbol backed by a tainted submodule — + via `from pkg import X`, attribute access on a whole-module + import (`pkg.X`), or a dotted string literal such as a + `unittest.mock.patch("pkg._sub.f")` target. Anything the analysis + cannot bound (impure barrel, star import, the module object + escaping into non-attribute contexts, unparsable source) falls + back to full taint, and a changed module whose transparent + closure reaches **zero** tests falls back to its opaque closure + (the *never-zero ratchet* — transparency may sharpen a selection, + never empty one). Editing a barrel `__init__.py` itself keeps the + full package-mode blast radius. Known accepted gap: a consumer + that imports a barrel only for a submodule's import-time side + effects (referencing no symbol) is not selected when that + submodule changes — pure barrels bind names; packages whose + import-time behavior is load-bearing (e.g. gateway's `@app.route` + registration) are impure by construction and stay opaque. 6. **Map modules → test files.** Intersect the downstream set with the pre-collected set of every `test_*.py` / `*_test.py` file in the graph. The selector emits the resulting set of test file @@ -119,7 +143,12 @@ The algorithm is: 8. **Run pytest.** The `make test` recipe pipes stdout into `pytest $(SELECTED) -v $(PYTEST_ARGS)`. If the selector emits zero lines, the recipe skips the pytest invocation and prints - `no tests selected`. + `no tests selected`. Selected files are emitted **direct + importers first** (ordered by import distance from the changed + modules, alphabetical within a distance tier; #3182): pytest + collects files in the order given, so a wide selection surfaces + the most likely failure in the first files run rather than + wherever the alphabet put it. A green narrow run **does not** update the LKG sidecar — only `make test-all` writes LKG, because only a full-suite green proves diff --git a/scripts/select_tests/__init__.py b/scripts/select_tests/__init__.py index 16c89bf9ef..06c6c6ff1e 100755 --- a/scripts/select_tests/__init__.py +++ b/scripts/select_tests/__init__.py @@ -140,22 +140,31 @@ TEST_ROOT_DIRS, ) -# Graph + closure + bare-name resolver + PYTEST_ARGS classifier. +# Graph + closure + bare-name resolver + barrel transparency (#3182) +# + PYTEST_ARGS classifier. from ._graph import ( _TEST_ROOT_PREFIXES, GraphBundle, + _barrel_name_forms, + _barrel_symbols_backed_by, + _direct_importers, _enumerate_source_paths, _extract_imports, _module_to_filesystem_path, _scan_dynamic_imports, + _used_symbols, _walk_upstream_combined, + _walk_upstream_with_depth, build_bare_name_index, build_bare_name_upstream_edges, + build_barrel_exports, build_graph, is_dynamic_import_touched, map_modules_to_test_files, + parse_barrel_exports, pytest_args_have_explicit_path, reverse_closure, + reverse_closure_with_depth, ) # Git, sidecar, baseline, and changed-files helpers. @@ -230,18 +239,26 @@ # Graph helpers (public + private) "GraphBundle", "_TEST_ROOT_PREFIXES", + "_barrel_name_forms", + "_barrel_symbols_backed_by", + "_direct_importers", "_enumerate_source_paths", "_extract_imports", "_module_to_filesystem_path", "_scan_dynamic_imports", + "_used_symbols", "_walk_upstream_combined", + "_walk_upstream_with_depth", "build_bare_name_index", "build_bare_name_upstream_edges", + "build_barrel_exports", "build_graph", "is_dynamic_import_touched", "map_modules_to_test_files", + "parse_barrel_exports", "pytest_args_have_explicit_path", "reverse_closure", + "reverse_closure_with_depth", # CLI helpers (public + private) "_build_arg_parser", "_fnmatch", diff --git a/scripts/select_tests/_cli.py b/scripts/select_tests/_cli.py index 4391393963..aa53b994e8 100644 --- a/scripts/select_tests/_cli.py +++ b/scripts/select_tests/_cli.py @@ -32,11 +32,13 @@ from ._graph import ( GraphBundle, _walk_upstream_combined, + _walk_upstream_with_depth, build_graph, is_dynamic_import_touched, map_modules_to_test_files, pytest_args_have_explicit_path, reverse_closure, + reverse_closure_with_depth, ) from ._io import ( RecordGoodValidationError, @@ -514,13 +516,28 @@ def _run_narrow_or_fallback(repo_root: Path) -> int: # neither analysis can see (e.g., subprocess invocation, runtime # plugin discovery, or a bare-name import we haven't taught the # resolver about). + rescue_depths: dict[str, int] = {} if bundle is not None and trigger is None and module_path_pairs: zero_downstream_offenders: list[str] = [] for module, _path in module_path_pairs: if module in bundle.all_test_modules: continue # editing a test pulls only itself; that's fine reachable = _walk_upstream_combined(bundle, [module]) - if not (reachable & bundle.all_test_modules): + if reachable & bundle.all_test_modules: + continue + # The (barrel-transparent, #3182) walk found no tests for + # this module. Distinguish a true blind spot from + # transparency filtering every consumer: the never-zero + # ratchet — transparency may sharpen a module's selection + # but never zero it out, so when the OPAQUE walk still + # reaches tests, select those instead of widening. Only + # when neither walk reaches a test does the pre-#3182 + # full-suite trigger fire. + opaque_depths = _walk_upstream_with_depth(bundle, {module: 0}, barrel_aware=False) + if set(opaque_depths) & bundle.all_test_modules: + for m, d in opaque_depths.items(): + rescue_depths[m] = min(rescue_depths.get(m, d), d) + else: zero_downstream_offenders.append(module) if zero_downstream_offenders: trigger = f"no downstream tests for changed module: {zero_downstream_offenders[0]}" @@ -556,8 +573,18 @@ def _run_narrow_or_fallback(repo_root: Path) -> int: # ---- Narrow path ---- assert bundle is not None # narrowed above - closure = reverse_closure(bundle, module_path_pairs) - test_files = map_modules_to_test_files(bundle, closure, repo_root) + closure_depths = reverse_closure_with_depth(bundle, module_path_pairs) + for module, depth in rescue_depths.items(): + if module not in closure_depths or depth < closure_depths[module]: + closure_depths[module] = depth + test_files = map_modules_to_test_files(bundle, set(closure_depths), repo_root) + # Emit direct-importers-first (#3182): pytest collects files in the + # order given on the command line, so sorting by import distance + # from the changed modules surfaces the most likely failure early + # in a wide selection. `map_modules_to_test_files` returns the + # paths alphabetically; the stable sort keeps that as the + # within-depth tiebreak. + test_files.sort(key=lambda p: closure_depths.get(path_to_module(p) or "", 1 << 30)) selected_count = len(test_files) for path in test_files: diff --git a/scripts/select_tests/_graph.py b/scripts/select_tests/_graph.py index d6e8bad3fd..234aacbfd5 100644 --- a/scripts/select_tests/_graph.py +++ b/scripts/select_tests/_graph.py @@ -17,6 +17,7 @@ import ast import os import sys +from collections import deque from collections.abc import Iterable from pathlib import Path from typing import Any @@ -53,6 +54,19 @@ class GraphBundle: production module via bare name. Supplements grimp for the 406/407 test files that use the bare-name pattern. + - barrel_exports: dict[str, dict[str, set[str]]] — for every + package whose `__init__.py` is a PURE + re-export barrel (#3182), maps each + re-exported symbol to the set of backing + FQ modules. Empty/missing entry means + "not a pure barrel" — the closure walk + treats such packages opaquely (status quo). + - repo_root: Path | None — repo root the graph was + built against; required for the lazy + per-consumer symbol-usage parse the + barrel-transparent walk performs. When + None (hand-built test bundles), usage + lookups fall open to "opaque consumer". """ def __init__( @@ -63,6 +77,8 @@ def __init__( dynamic_import_modules: set[str], missing_source_paths: list[str], bare_name_upstream: dict[str, set[str]] | None = None, + barrel_exports: dict[str, dict[str, set[str]]] | None = None, + repo_root: Path | None = None, ) -> None: self.graph = graph self.all_modules = all_modules @@ -70,6 +86,20 @@ def __init__( self.dynamic_import_modules = dynamic_import_modules self.missing_source_paths = missing_source_paths self.bare_name_upstream = bare_name_upstream if bare_name_upstream is not None else {} + self.barrel_exports = barrel_exports if barrel_exports is not None else {} + self.repo_root = repo_root + # Lazy memo caches for the barrel-transparent walk. Keyed by + # module id; populated on first use, never invalidated (the + # bundle is built fresh per selector invocation). + self._usage_cache: dict[str, _ModuleUsage | None] = {} + self._bare_name_index: dict[str, set[str]] | None = None + + def bare_name_index(self) -> dict[str, set[str]]: + """Memoized bare-name → FQ-candidates index (see + `build_bare_name_index`).""" + if self._bare_name_index is None: + self._bare_name_index = build_bare_name_index(self.all_modules) + return self._bare_name_index def _enumerate_source_paths(repo_root: Path) -> Iterable[Path]: @@ -276,32 +306,506 @@ def build_bare_name_upstream_edges(all_modules: set[str], repo_root: Path) -> di return upstream -def _walk_upstream_combined(bundle: GraphBundle, seeds: Iterable[str]) -> set[str]: - """BFS over importers of every seed, combining grimp's transitive - closure (`find_downstream_modules`, which in grimp's terminology - means consumers — modules that import the given module) with the - AST resolver's bare-name reverse edges. +# ---------------------------------------------------------------------- +# Barrel-transparent closure (#3182) +# +# The decomposition pattern (docs/guides/decomposition-pattern.md) +# turns each oversize file into a sub-package whose `__init__.py` is a +# pure re-export barrel. Under a module-level reverse walk, that +# barrel reconstitutes the original file's full blast radius: a change +# to `pkg._sub` taints `pkg` (the barrel imports it), and every +# importer of `pkg` taints in turn — selection-wise the decomposition +# never happened. +# +# The walk below treats a PURE barrel as transparent instead: when the +# frontier reaches a barrel through one of its own submodules, only +# the re-exported symbols backed by tainted submodules are considered +# tainted, and a consumer of the barrel is pulled into the closure +# only if its source statically uses one of those symbols (via +# `from pkg import X`, attribute access on a whole-module import, or +# a dotted string literal such as a `unittest.mock.patch` target). +# +# Soundness posture (unchanged from the rest of the selector — "never +# skip a test that exercises a changed code path"): +# - anything that is not a *provably pure* barrel stays opaque; +# - any consumer whose usage the AST scan cannot fully see (module +# object escaping, star import, unparsable source, missing file) +# is fully tainted; +# - when the direct-importers API is unavailable the walk falls back +# to grimp's transitive closure per node, which subsumes barrel +# consumers and silently disables transparency; +# - `_run_narrow_or_fallback` adds a never-zero ratchet: a changed +# module whose transparent closure reaches no test falls back to +# its opaque closure (transparency may sharpen a selection, never +# zero one out). +# +# Known accepted gap: a consumer that imports the barrel only for a +# submodule's import-time side effects (no symbol reference) is not +# selected when that submodule changes. Pure barrels bind names and +# import submodules — modules whose import-time behaviour is +# load-bearing (e.g. gateway's `@app.route` registration, decision-8 +# of #2261) make their barrel impure and stay opaque. +# ---------------------------------------------------------------------- - Returns the set of every module reachable from any seed via either - edge source, including the seeds themselves. + +class _ModuleUsage: + """Per-consumer record of which symbols it uses from which modules. + + - symbols_by_target: maps an FQ module id to the set of symbol + names this consumer statically references on it, or ``None`` + when the consumer's use of that module cannot be bounded (star + import, module object escaping into non-attribute contexts). + - dotted_strings: string literals in the consumer that name a + barrel or a dotted path under one (pre-filtered against the + bundle's known barrel name forms) — covers + ``patch("routes.pipelines._foo")``-style runtime references. + """ + + __slots__ = ("symbols_by_target", "dotted_strings") + + def __init__(self) -> None: + self.symbols_by_target: dict[str, set[str] | None] = {} + self.dotted_strings: set[str] = set() + + +def _handler_catches_import_error(handler: ast.ExceptHandler) -> bool: + """True iff the except clause catches (only) import errors.""" + node = handler.type + if isinstance(node, ast.Name): + return node.id in ("ImportError", "ModuleNotFoundError") + if isinstance(node, ast.Tuple): + return bool(node.elts) and all( + isinstance(e, ast.Name) and e.id in ("ImportError", "ModuleNotFoundError") + for e in node.elts + ) + return False + + +def parse_barrel_exports(source: str, package: str) -> dict[str, set[str]] | None: + """Parse a package ``__init__.py``; return its re-export map when it + is a PURE barrel, else None. + + A pure barrel contains only: a docstring (or other bare string + expressions), ``from __future__`` imports, imports (absolute or + single-level relative; the repo's ``try/except ImportError`` + dual-import idiom is allowed when both arms contain only imports), + and an ``__all__`` assignment of string constants. Star imports, + multi-level relative imports (``from ..``), and ANY other + statement (defs, decorators, conditionals, calls) disqualify it — + such packages stay opaque to the closure walk. + + The returned map is ``{bound_symbol: {backing_module, …}}``. + Relative imports map to FQ submodules of ``package``; absolute + imports map to their literal targets (which can never match a + tainted submodule of this package — recorded so consumer lookups + of those symbols stay precise rather than falling open). + """ + + def absorb_import(node: ast.stmt, exports: dict[str, set[str]]) -> bool: + if isinstance(node, ast.Import): + for alias in node.names: + bound = alias.asname or alias.name.split(".")[0] + exports.setdefault(bound, set()).add(alias.name) + return True + if isinstance(node, ast.ImportFrom): + if node.module == "__future__" and node.level == 0: + return True + if node.level >= 2: + return False + if any(alias.name == "*" for alias in node.names): + return False + for alias in node.names: + bound = alias.asname or alias.name + if node.level == 1: + if node.module: + backing = f"{package}.{node.module}" + else: + backing = f"{package}.{alias.name}" + else: + backing = f"{node.module}.{alias.name}" if node.module else alias.name + exports.setdefault(bound, set()).add(backing) + return True + return False + + try: + tree = ast.parse(source) + except SyntaxError, ValueError: + return None + + exports: dict[str, set[str]] = {} + for stmt in tree.body: + if isinstance(stmt, (ast.Import, ast.ImportFrom)): + if not absorb_import(stmt, exports): + return None + continue + if ( + isinstance(stmt, ast.Expr) + and isinstance(stmt.value, ast.Constant) + and isinstance(stmt.value.value, str) + ): + continue # docstring / bare string expression + if isinstance(stmt, ast.Assign): + if ( + len(stmt.targets) == 1 + and isinstance(stmt.targets[0], ast.Name) + and stmt.targets[0].id == "__all__" + and isinstance(stmt.value, (ast.Tuple, ast.List)) + and all( + isinstance(e, ast.Constant) and isinstance(e.value, str) + for e in stmt.value.elts + ) + ): + continue + return None + if isinstance(stmt, ast.Try): + # Dual-import idiom (non-negotiable #4 of #3111): + # try: from ._sub import X + # except ImportError: from _sub import X + if stmt.orelse or stmt.finalbody: + return None + if not stmt.body or not all( + isinstance(s, (ast.Import, ast.ImportFrom)) for s in stmt.body + ): + return None + if not stmt.handlers or not all( + _handler_catches_import_error(h) + and all(isinstance(s, (ast.Import, ast.ImportFrom, ast.Pass)) for s in h.body) + for h in stmt.handlers + ): + return None + for s in stmt.body: + if not absorb_import(s, exports): + return None + for h in stmt.handlers: + for s in h.body: + if not isinstance(s, ast.Pass) and not absorb_import(s, exports): + return None + continue + return None + return exports + + +def build_barrel_exports(all_modules: set[str], repo_root: Path) -> dict[str, dict[str, set[str]]]: + """Scan every package ``__init__.py`` in the graph; return the + re-export maps of the pure barrels (see `parse_barrel_exports`). + + Packages with an EMPTY ``__init__.py`` are excluded — an empty + barrel exports nothing and has no import edges to filter, so + registering it would only add an always-empty partial state to the + walk. Read/parse failures fail open (package stays opaque). """ - closure: set[str] = set(seeds) - frontier: set[str] = set(closure) - while frontier: - module = frontier.pop() + barrels: dict[str, dict[str, set[str]]] = {} + for module in all_modules: + init = repo_root / module.replace(".", os.sep) / "__init__.py" + if not init.is_file(): + continue + # Mirror `_module_to_filesystem_path` precedence: a leaf .py + # shadowing the package name wins, so skip the ambiguous case. + if (repo_root / (module.replace(".", os.sep) + ".py")).is_file(): + continue try: - grimp_consumers = bundle.graph.find_downstream_modules(module, as_package=False) + exports = parse_barrel_exports( + init.read_text(encoding="utf-8", errors="replace"), module + ) + except OSError: + continue + if exports: + barrels[module] = exports + return barrels + + +def _barrel_name_forms(barrel: str) -> tuple[str, ...]: + """Every name a consumer might write for `barrel`: the FQ id plus + each prefix-stripped bare form (mirrors `build_bare_name_index`).""" + forms = [barrel] + for prefix in BARE_NAME_STRIP_PREFIXES: + if barrel.startswith(prefix): + bare = barrel[len(prefix) :] + if bare: + forms.append(bare) + return tuple(forms) + + +def _compute_module_usage(bundle: GraphBundle, module: str) -> _ModuleUsage | None: + """AST-scan `module`; return its symbol-usage record, or None when + the scan cannot run (no repo root, unreadable/unparsable source) — + the caller treats None as "fully opaque consumer".""" + if bundle.repo_root is None: + return None + source_path = _module_to_filesystem_path(module, bundle.repo_root) + if source_path is None: + return None + try: + source = source_path.read_text(encoding="utf-8", errors="replace") + tree = ast.parse(source, filename=str(source_path)) + except SyntaxError, OSError, ValueError: + return None + + index = bundle.bare_name_index() + usage = _ModuleUsage() + # Bound name (import alias) -> FQ candidates of the module object + # it references. Only module objects participate in the attribute + # walk; `from pkg import symbol` bindings are recorded directly. + alias_map: dict[str, set[str]] = {} + + def resolve(target: str) -> set[str]: + candidates = set(index.get(target, ())) + if target in bundle.all_modules: + candidates.add(target) + return candidates + + def add_symbol(candidates: set[str], symbol: str) -> None: + for fq in candidates: + existing = usage.symbols_by_target.get(fq, set()) + if existing is None: + continue # already opaque for this target + existing.add(symbol) + usage.symbols_by_target[fq] = existing + + def mark_opaque(candidates: set[str]) -> None: + for fq in candidates: + usage.symbols_by_target[fq] = None + + def add_prefix_symbols(dotted: str) -> None: + # `import a.b.c` / `from a.b import x` traverses each parent + # package: record `b` as a used symbol of `a`, `c` of `a.b`, … + parts = dotted.split(".") + for i in range(1, len(parts)): + add_symbol(resolve(".".join(parts[:i])), parts[i]) + + is_package = source_path.name == "__init__.py" + pkg_parts = module.split(".") if is_package else module.split(".")[:-1] + + for node in ast.walk(tree): + if isinstance(node, ast.Import): + for alias in node.names: + add_prefix_symbols(alias.name) + if alias.asname: + alias_map.setdefault(alias.asname, set()).update(resolve(alias.name)) + else: + first = alias.name.split(".")[0] + alias_map.setdefault(first, set()).update(resolve(first)) + elif isinstance(node, ast.ImportFrom): + if node.level == 0: + if node.module is None: + continue + target = node.module + else: + if node.level - 1 > len(pkg_parts): + continue # walks above the package root — leave unseen (conservative) + anchor = pkg_parts[: len(pkg_parts) - (node.level - 1)] + target = ".".join(anchor + (node.module.split(".") if node.module else [])) + if not target: + continue + add_prefix_symbols(target) + target_candidates = resolve(target) + for alias in node.names: + if alias.name == "*": + mark_opaque(target_candidates) + continue + add_symbol(target_candidates, alias.name) + # `from pkg import _sub` binds a MODULE object; track it + # so `_sub.attr` accesses resolve to pkg._sub symbols. + sub_candidates = resolve(f"{target}.{alias.name}") + if sub_candidates: + alias_map.setdefault(alias.asname or alias.name, set()).update(sub_candidates) + + barrel_forms: list[str] = [] + for barrel in bundle.barrel_exports: + barrel_forms.extend(_barrel_name_forms(barrel)) + + class _UsageVisitor(ast.NodeVisitor): + def visit_Attribute(self, node: ast.Attribute) -> None: + value = node.value + if isinstance(value, ast.Name) and value.id in alias_map: + add_symbol(alias_map[value.id], node.attr) + return # the Name is a legitimate attribute base, not an escape + self.generic_visit(node) + + def visit_Name(self, node: ast.Name) -> None: + # A module alias used outside attribute access — the module + # object escapes (passed to reload(), getattr(), …); we can + # no longer bound which symbols are reached through it. + if node.id in alias_map: + mark_opaque(alias_map[node.id]) + + def visit_Constant(self, node: ast.Constant) -> None: + if isinstance(node.value, str): + s = node.value + for form in barrel_forms: + if s == form or s.startswith(form + "."): + usage.dotted_strings.add(s) + break + + _UsageVisitor().visit(tree) + return usage + + +def _module_usage(bundle: GraphBundle, module: str) -> _ModuleUsage | None: + if module not in bundle._usage_cache: + bundle._usage_cache[module] = _compute_module_usage(bundle, module) + return bundle._usage_cache[module] + + +def _used_symbols(bundle: GraphBundle, consumer: str, barrel: str) -> set[str] | None: + """The set of `barrel` symbols `consumer` statically uses, or None + when the usage cannot be bounded (treat as: uses everything).""" + usage = _module_usage(bundle, consumer) + if usage is None: + return None + referenced = False + symbols: set[str] = set() + direct = usage.symbols_by_target.get(barrel, set()) + if direct is None: + return None + if barrel in usage.symbols_by_target: + referenced = True + symbols |= direct + for form in _barrel_name_forms(barrel): + prefix = form + "." + for s in usage.dotted_strings: + if s == form: + return None # whole-barrel runtime reference (dynamic import) + if s.startswith(prefix): + referenced = True + symbols.add(s[len(prefix) :].split(".")[0]) + if not referenced: + # The graph has an edge we cannot explain from the source scan + # — conservative. + return None + return symbols + + +def _direct_importers(bundle: GraphBundle, module: str) -> set[str]: + """Direct importers of `module`: grimp's direct-edge API plus the + bare-name resolver's reverse edges. Falls back to grimp's + TRANSITIVE downstream set when the direct API is unavailable — + sound (a superset of the direct importers), and because none of + those transitive consumers are barrels-of-`module`'s-package in + the eyes of `_barrel_symbols_backed_by` they all taint fully, so + the fallback silently disables transparency rather than narrowing + incorrectly.""" + graph = bundle.graph + try: + consumers = set(graph.find_modules_that_directly_import(module)) + except Exception: # noqa: BLE001 — fail-open to the transitive API + try: + consumers = set(graph.find_downstream_modules(module, as_package=False)) except Exception: # noqa: BLE001 — fail-open - grimp_consumers = set() - for c in grimp_consumers: - if c not in closure: - closure.add(c) - frontier.add(c) - for c in bundle.bare_name_upstream.get(module, ()): - if c not in closure: - closure.add(c) - frontier.add(c) - return closure + consumers = set() + consumers.update(bundle.bare_name_upstream.get(module, ())) + return consumers + + +def _barrel_symbols_backed_by(bundle: GraphBundle, candidate: str, tainted: str) -> set[str] | None: + """When `candidate` is a pure barrel whose package contains + `tainted`, return the re-exported symbols backed by it (possibly + empty); otherwise None (candidate is not eligible for transparent + treatment on this edge).""" + exports = bundle.barrel_exports.get(candidate) + if not exports: + return None + if not tainted.startswith(candidate + "."): + return None + symbols: set[str] = set() + for symbol, backings in exports.items(): + for backing in backings: + if ( + backing == tainted + or backing.startswith(tainted + ".") + or tainted.startswith(backing + ".") + ): + symbols.add(symbol) + break + return symbols + + +def _walk_upstream_with_depth( + bundle: GraphBundle, seed_depths: dict[str, int], *, barrel_aware: bool = True +) -> dict[str, int]: + """Barrel-aware BFS over direct importers. Returns every module + reachable from the seeds mapped to its (approximate) import + distance — seeds keep their given depth, direct importers are one + step further, and so on. + + Modules are in one of three states: untouched, *partially* tainted + (pure barrels reached through their own submodules — tracked with + the set of tainted re-exported symbols), or *fully* tainted. A + consumer of a partially-tainted barrel is pulled in only when its + statically-visible usage intersects the tainted symbol set (or the + usage cannot be bounded). Full taint always supersedes partial. + """ + full: dict[str, int] = {} + partial_symbols: dict[str, set[str]] = {} + partial_depth: dict[str, int] = {} + queue: deque[str] = deque() + + def taint_full(module: str, depth: int) -> None: + full[module] = depth + partial_symbols.pop(module, None) + queue.append(module) + + for seed, depth in seed_depths.items(): + if seed not in full or depth < full[seed]: + full[seed] = depth + queue.append(seed) + + while queue: + module = queue.popleft() + if module in full: + depth = full[module] + for consumer in _direct_importers(bundle, module): + if consumer in full: + continue + symbols = ( + _barrel_symbols_backed_by(bundle, consumer, module) if barrel_aware else None + ) + if not symbols: + # Not a barrel-of-this-package (None), or a barrel + # whose re-export map claims nothing from the + # tainted module despite the import edge (empty set + # — analysis gap): conservative full taint. + taint_full(consumer, depth + 1) + continue + known = partial_symbols.get(consumer) + if known is None or not symbols <= known: + partial_symbols.setdefault(consumer, set()).update(symbols) + partial_depth[consumer] = min(partial_depth.get(consumer, depth + 1), depth + 1) + queue.append(consumer) + else: + tainted_symbols = partial_symbols.get(module) + if tainted_symbols is None: + continue # upgraded to full (handled) or stale queue entry + depth = partial_depth[module] + for consumer in _direct_importers(bundle, module): + if consumer in full: + continue + used = _used_symbols(bundle, consumer, module) + if used is None or used & tainted_symbols: + taint_full(consumer, depth + 1) + + result = dict(full) + for barrel, depth in partial_depth.items(): + if barrel in partial_symbols and barrel not in result: + result[barrel] = depth + return result + + +def _walk_upstream_combined( + bundle: GraphBundle, seeds: Iterable[str], *, barrel_aware: bool = True +) -> set[str]: + """BFS over importers of every seed, combining grimp's edges with + the AST resolver's bare-name reverse edges. Pure re-export + barrels are treated transparently unless ``barrel_aware=False`` + (see `_walk_upstream_with_depth`); bundles without barrel data + behave exactly as the pre-#3182 transitive walk. + + Returns the set of every module reachable from any seed via either + edge source, including the seeds themselves. + """ + return set( + _walk_upstream_with_depth(bundle, dict.fromkeys(seeds, 0), barrel_aware=barrel_aware) + ) def build_graph(repo_root: Path | None = None, packages: tuple[str, ...] = PACKAGES) -> GraphBundle: @@ -457,6 +961,10 @@ def build_graph(repo_root: Path | None = None, packages: tuple[str, ...] = PACKA # comment above `_module_to_filesystem_path` for context. bare_name_upstream = build_bare_name_upstream_edges(all_modules, root) + # Pure re-export barrels (#3182) — enables the barrel-transparent + # closure walk. See the section comment above `_ModuleUsage`. + barrel_exports = build_barrel_exports(all_modules, root) + return GraphBundle( graph=graph, all_modules=all_modules, @@ -464,6 +972,8 @@ def build_graph(repo_root: Path | None = None, packages: tuple[str, ...] = PACKA dynamic_import_modules=dynamic_import_modules, missing_source_paths=missing_source_paths, bare_name_upstream=bare_name_upstream, + barrel_exports=barrel_exports, + repo_root=root, ) @@ -472,24 +982,19 @@ def build_graph(repo_root: Path | None = None, packages: tuple[str, ...] = PACKA # ---------------------------------------------------------------------- -def reverse_closure(bundle: GraphBundle, module_path_pairs: Iterable[tuple[str, str]]) -> set[str]: - """Return the transitive set of modules that import any changed module. - - Mixed `as_package` strategy (algorithm §6): - - If the changed path is an `__init__.py`, treat the module as a - package and call `find_downstream_modules(pkg, as_package=True)`. - - Otherwise (regular leaf), call with `as_package=False`. +def reverse_closure_with_depth( + bundle: GraphBundle, module_path_pairs: Iterable[tuple[str, str]] +) -> dict[str, int]: + """Like `reverse_closure`, but maps every reachable module to its + (approximate) import distance from the changed set — changed + modules at 0, their direct importers at 1, and so on. Modules + pulled in by the package-mode closure of an `__init__.py` seed are + assigned depth 1 (grimp's package-mode call is transitive, so no + finer distance is available for them). - Callers MUST pass aligned `(module, path)` tuples — building the - pairing inside the function (rather than zipping two lists at the - call site) prevents any chance of `__init__.py` detection misfiring - when the unresolvable-paths filter shortens one list relative to - the other (reviewer_contract feedback on the v1 proposal). - - The walk combines grimp's transitive closure with the AST - resolver's bare-name reverse edges (`bundle.bare_name_upstream`), - so consumers that import the changed module via bare name — - grimp's structural blind spot in this repo — are still picked up. + `_run_narrow_or_fallback` uses the depths to emit the selected + test files direct-importers-first (#3182) so pytest surfaces the + most likely failure early in a wide selection. """ init_modules: set[str] = set() leaf_modules: set[str] = set() @@ -502,21 +1007,49 @@ def reverse_closure(bundle: GraphBundle, module_path_pairs: Iterable[tuple[str, # Step 1: grimp's package-mode closure for `__init__.py` seeds (a # package edit can affect anything downstream of the WHOLE package, # not just the __init__ leaf). Leaf seeds are handled by the - # combined walker below. - closure: set[str] = set(init_modules) | set(leaf_modules) + # combined walker below. Barrel transparency intentionally does + # NOT apply to these seeds — editing a barrel itself affects every + # consumer. + seed_depths: dict[str, int] = dict.fromkeys(init_modules | leaf_modules, 0) for module in init_modules: try: - closure |= set(bundle.graph.find_downstream_modules(module, as_package=True)) + package_downstream = set(bundle.graph.find_downstream_modules(module, as_package=True)) except Exception: # noqa: BLE001 — fail-open at upper layer continue + for downstream in package_downstream: + if downstream not in seed_depths: + seed_depths[downstream] = 1 - # Step 2: combined BFS — extends `closure` via grimp's leaf-mode - # closure AND the bare-name resolver's reverse edges. Re-walking - # from every node already in `closure` is correct (set-membership - # checks short-circuit visited nodes) and ensures bare-name edges - # discovered downstream of the package-mode closure are still - # followed transitively. - return _walk_upstream_combined(bundle, closure) + # Step 2: combined BFS — extends the seed set via grimp's edges AND + # the bare-name resolver's reverse edges. Re-walking from every + # seed is correct (visited nodes short-circuit) and ensures + # bare-name edges discovered downstream of the package-mode + # closure are still followed transitively. + return _walk_upstream_with_depth(bundle, seed_depths) + + +def reverse_closure(bundle: GraphBundle, module_path_pairs: Iterable[tuple[str, str]]) -> set[str]: + """Return the transitive set of modules that import any changed module. + + Mixed `as_package` strategy (algorithm §6): + - If the changed path is an `__init__.py`, treat the module as a + package and call `find_downstream_modules(pkg, as_package=True)`. + - Otherwise (regular leaf), call with `as_package=False`. + + Callers MUST pass aligned `(module, path)` tuples — building the + pairing inside the function (rather than zipping two lists at the + call site) prevents any chance of `__init__.py` detection misfiring + when the unresolvable-paths filter shortens one list relative to + the other (reviewer_contract feedback on the v1 proposal). + + The walk combines grimp's edges with the AST resolver's bare-name + reverse edges (`bundle.bare_name_upstream`), so consumers that + import the changed module via bare name — grimp's structural blind + spot in this repo — are still picked up. Pure re-export barrels + are treated transparently (#3182); bundles without barrel data + behave exactly as before. + """ + return set(reverse_closure_with_depth(bundle, module_path_pairs)) def is_dynamic_import_touched(bundle: GraphBundle, changed_modules: Iterable[str]) -> bool: @@ -603,16 +1136,24 @@ def pytest_args_have_explicit_path(args: Iterable[str], repo_root: Path) -> bool __all__ = ( "GraphBundle", "_TEST_ROOT_PREFIXES", + "_barrel_name_forms", + "_barrel_symbols_backed_by", + "_direct_importers", "_enumerate_source_paths", "_extract_imports", "_module_to_filesystem_path", "_scan_dynamic_imports", + "_used_symbols", "_walk_upstream_combined", + "_walk_upstream_with_depth", "build_bare_name_index", "build_bare_name_upstream_edges", + "build_barrel_exports", "build_graph", "is_dynamic_import_touched", "map_modules_to_test_files", + "parse_barrel_exports", "pytest_args_have_explicit_path", "reverse_closure", + "reverse_closure_with_depth", ) diff --git a/tests/tools/test_select_tests_barrel.py b/tests/tools/test_select_tests_barrel.py new file mode 100644 index 0000000000..281f42a412 --- /dev/null +++ b/tests/tools/test_select_tests_barrel.py @@ -0,0 +1,560 @@ +"""Tests for barrel-transparent narrowing + import-distance ordering +in ``scripts/select_tests/`` (#3182). + +The decomposition pattern (#3111 / docs/guides/decomposition-pattern.md) +turns oversize files into sub-packages fronted by a pure re-export +barrel ``__init__.py``. Under a module-level reverse walk that barrel +reconstitutes the original file's full blast radius — a change to one +submodule taints every consumer of the barrel. These tests pin: + + 1. ``parse_barrel_exports`` — the purity classifier + re-export map; + 2. ``_used_symbols`` — the consumer-side symbol-usage scan; + 3. ``_walk_upstream_with_depth`` — the transparent closure walk and + its conservative fallbacks; + 4. the never-zero ratchet and direct-importers-first output ordering + in ``_run_narrow_or_fallback``. +""" + +from __future__ import annotations + +import json +import textwrap +from pathlib import Path + +import pytest + +from tests.tools._select_tests_helpers import load_selector + +selector = load_selector() + + +# ---------------------------------------------------------------------- +# `parse_barrel_exports` — purity classifier + re-export map. +# ---------------------------------------------------------------------- + + +def _parse_barrel(source: str, package: str = "pkg") -> dict[str, set[str]] | None: + return selector.parse_barrel_exports(textwrap.dedent(source), package) + + +def test_pure_barrel_relative_from_imports() -> None: + exports = _parse_barrel( + ''' + """Docstring.""" + + from __future__ import annotations + + from ._sub import sub_func, OtherThing + from ._other import other_func + + __all__ = ("OtherThing", "other_func", "sub_func") + ''' + ) + assert exports == { + "sub_func": {"pkg._sub"}, + "OtherThing": {"pkg._sub"}, + "other_func": {"pkg._other"}, + } + + +def test_pure_barrel_from_dot_import_binds_submodule() -> None: + """``from . import _io`` re-exports the SUBMODULE object — tests + reach internals through it (``selector._io._run_git``), so the + bound name must map to the submodule itself.""" + exports = _parse_barrel("from . import _io, _cli\n") + assert exports == {"_io": {"pkg._io"}, "_cli": {"pkg._cli"}} + + +def test_pure_barrel_asname_binds_the_alias() -> None: + exports = _parse_barrel("from ._sub import inner as public\n") + assert exports == {"public": {"pkg._sub"}} + + +def test_pure_barrel_dual_import_idiom() -> None: + """The repo's ``try/except ImportError`` dual-import shape + (non-negotiable #4 of #3111) must not disqualify a barrel; the + relative arm provides the FQ backing.""" + exports = _parse_barrel( + """ + try: + from ._sub import sub_func + except ImportError: + from _sub import sub_func + """ + ) + assert exports is not None + assert exports["sub_func"] >= {"pkg._sub"} + + +def test_pure_barrel_allows_all_assignment_list_form() -> None: + exports = _parse_barrel('from ._a import x\n__all__ = ["x"]\n') + assert exports == {"x": {"pkg._a"}} + + +def test_absolute_imports_keep_external_backing() -> None: + """Absolute imports don't disqualify a barrel; their bound names + map to external targets that can never match a tainted submodule, + so consumers of those symbols are not pulled in by package + changes.""" + exports = _parse_barrel("from typing import Any\nfrom ._sub import x\n") + assert exports is not None + assert exports["x"] == {"pkg._sub"} + assert exports["Any"] == {"typing.Any"} + + +@pytest.mark.parametrize( + "source", + [ + "from ._sub import *\n", # star re-export — per-symbol map impossible + "from ..sibling import x\n", # multi-level relative — outside the package + "def helper():\n pass\n", # def + "from ._sub import x\nVERSION = '1.0'\n", # non-__all__ assignment + "import os\nif os.name == 'posix':\n from ._a import x\n", # conditional + "from ._sub import x\nx.register()\n", # call + "try:\n from ._a import x\nexcept Exception:\n from _a import x\n", # wrong exc + "try:\n from ._a import x\nexcept ImportError:\n x = None\n", # non-import arm + "def x(:\n", # syntax error + ], +) +def test_impure_barrels_return_none(source: str) -> None: + assert _parse_barrel(source) is None + + +def test_build_barrel_exports_scans_packages(tmp_path: Path) -> None: + pure = tmp_path / "pure_pkg" + pure.mkdir() + (pure / "__init__.py").write_text("from ._sub import x\n", encoding="utf-8") + (pure / "_sub.py").write_text("x = 1\n", encoding="utf-8") + impure = tmp_path / "impure_pkg" + impure.mkdir() + (impure / "__init__.py").write_text("def f():\n pass\n", encoding="utf-8") + empty = tmp_path / "empty_pkg" + empty.mkdir() + (empty / "__init__.py").write_text("", encoding="utf-8") + + barrels = selector.build_barrel_exports( + {"pure_pkg", "pure_pkg._sub", "impure_pkg", "empty_pkg"}, tmp_path + ) + assert set(barrels) == {"pure_pkg"} + assert barrels["pure_pkg"] == {"x": {"pure_pkg._sub"}} + + +# ---------------------------------------------------------------------- +# `_barrel_symbols_backed_by` — edge-eligibility test. +# ---------------------------------------------------------------------- + + +def _bundle( + *, + direct_importers: dict[str, set[str]] | None = None, + package_downstream: dict[str, set[str]] | None = None, + all_modules: set[str] | None = None, + all_test_modules: set[str] | None = None, + bare_name_upstream: dict[str, set[str]] | None = None, + barrel_exports: dict[str, dict[str, set[str]]] | None = None, + repo_root: Path | None = None, +) -> object: + return selector.GraphBundle( + graph=_DirectEdgeGraph(direct_importers or {}, package_downstream or {}), + all_modules=all_modules or set(), + all_test_modules=all_test_modules or set(), + dynamic_import_modules=set(), + missing_source_paths=[], + bare_name_upstream=bare_name_upstream or {}, + barrel_exports=barrel_exports, + repo_root=repo_root, + ) + + +class _DirectEdgeGraph: + """Fake grimp graph exposing the direct-importers API the + barrel-aware walk prefers, plus the package-mode transitive call + `reverse_closure` uses for `__init__.py` seeds.""" + + def __init__( + self, + direct_importers: dict[str, set[str]], + package_downstream: dict[str, set[str]] | None = None, + ) -> None: + self._direct = direct_importers + self._package_downstream = package_downstream or {} + + def find_modules_that_directly_import(self, module: str) -> set[str]: + return set(self._direct.get(module, set())) + + def find_downstream_modules(self, module: str, *, as_package: bool = False) -> set[str]: + if as_package: + return set(self._package_downstream.get(module, set())) + # Transitive leaf closure over the direct edges. + closure: set[str] = set() + frontier = [module] + while frontier: + for consumer in self._direct.get(frontier.pop(), set()): + if consumer not in closure: + closure.add(consumer) + frontier.append(consumer) + return closure + + +def test_barrel_symbols_backed_by_matches_exact_and_subtree() -> None: + bundle = _bundle( + barrel_exports={"pkg": {"x": {"pkg._sub"}, "y": {"pkg._other"}, "z": {"pkg._sub.deep"}}} + ) + assert selector._barrel_symbols_backed_by(bundle, "pkg", "pkg._sub") == {"x", "z"} + assert selector._barrel_symbols_backed_by(bundle, "pkg", "pkg._sub.deep") == {"x", "z"} + assert selector._barrel_symbols_backed_by(bundle, "pkg", "pkg._other") == {"y"} + + +def test_barrel_symbols_backed_by_rejects_non_barrels_and_outsiders() -> None: + bundle = _bundle(barrel_exports={"pkg": {"x": {"pkg._sub"}}}) + # Not a barrel at all: + assert selector._barrel_symbols_backed_by(bundle, "other", "other._sub") is None + # Tainted module outside the barrel's package: + assert selector._barrel_symbols_backed_by(bundle, "pkg", "elsewhere._sub") is None + + +# ---------------------------------------------------------------------- +# `_used_symbols` — consumer-side usage scan. +# ---------------------------------------------------------------------- + + +def _usage_fixture(tmp_path: Path, consumer_source: str) -> object: + """A bundle whose graph contains the barrel ``shared.mypkg`` and a + single consumer ``tests.test_consumer`` with the given source.""" + pkg = tmp_path / "shared" / "mypkg" + pkg.mkdir(parents=True) + (pkg / "__init__.py").write_text( + "from ._sub import sub_func\nfrom ._other import other_func\n", encoding="utf-8" + ) + (pkg / "_sub.py").write_text("def sub_func():\n pass\n", encoding="utf-8") + (pkg / "_other.py").write_text("def other_func():\n pass\n", encoding="utf-8") + tests_dir = tmp_path / "tests" + tests_dir.mkdir() + (tests_dir / "test_consumer.py").write_text(textwrap.dedent(consumer_source), encoding="utf-8") + return _bundle( + all_modules={ + "shared.mypkg", + "shared.mypkg._sub", + "shared.mypkg._other", + "tests.test_consumer", + }, + barrel_exports={ + "shared.mypkg": { + "sub_func": {"shared.mypkg._sub"}, + "other_func": {"shared.mypkg._other"}, + } + }, + repo_root=tmp_path, + ) + + +def test_used_symbols_from_import_via_bare_name(tmp_path: Path) -> None: + """``from mypkg import sub_func`` — the dominant repo idiom; the + bare name must resolve to the FQ barrel.""" + bundle = _usage_fixture(tmp_path, "from mypkg import sub_func\n") + assert selector._used_symbols(bundle, "tests.test_consumer", "shared.mypkg") == {"sub_func"} + + +def test_used_symbols_attribute_access_on_module_alias(tmp_path: Path) -> None: + bundle = _usage_fixture( + tmp_path, + """ + import mypkg as m + + def test_x(): + m.other_func() + m.other_func.cache_clear() + """, + ) + assert selector._used_symbols(bundle, "tests.test_consumer", "shared.mypkg") == {"other_func"} + + +def test_used_symbols_patch_string_target(tmp_path: Path) -> None: + """``patch("mypkg._sub.helper")``-style string references must + count as usage of the first component under the barrel.""" + bundle = _usage_fixture( + tmp_path, + """ + from unittest.mock import patch + import mypkg + + def test_x(): + with patch("mypkg._sub"): + mypkg.sub_func() + """, + ) + used = selector._used_symbols(bundle, "tests.test_consumer", "shared.mypkg") + assert used == {"sub_func", "_sub"} + + +@pytest.mark.parametrize( + ("source", "reason"), + [ + ("import mypkg\nimportlib = None\nx = mypkg\n", "module object escapes"), + ("from mypkg import *\n", "star import"), + ("def broken(:\n", "unparsable source"), + ("import os\n", "edge exists but no visible reference"), + ], +) +def test_used_symbols_unbounded_cases_return_none(tmp_path: Path, source: str, reason: str) -> None: + bundle = _usage_fixture(tmp_path, source) + assert selector._used_symbols(bundle, "tests.test_consumer", "shared.mypkg") is None, reason + + +def test_used_symbols_without_repo_root_is_opaque(tmp_path: Path) -> None: + bundle = _bundle(barrel_exports={"pkg": {"x": {"pkg._sub"}}}, repo_root=None) + assert selector._used_symbols(bundle, "anything", "pkg") is None + + +# ---------------------------------------------------------------------- +# `_walk_upstream_with_depth` — transparent closure. +# ---------------------------------------------------------------------- + + +def _closure_fixture(tmp_path: Path) -> object: + """Barrel ``shared.mypkg`` with two submodules; three test + consumers: one uses the ``_sub``-backed symbol, one the + ``_other``-backed symbol, one lets the module object escape.""" + pkg = tmp_path / "shared" / "mypkg" + pkg.mkdir(parents=True) + (pkg / "__init__.py").write_text( + "from ._sub import sub_func\nfrom ._other import other_func\n", encoding="utf-8" + ) + (pkg / "_sub.py").write_text("def sub_func():\n pass\n", encoding="utf-8") + (pkg / "_other.py").write_text("def other_func():\n pass\n", encoding="utf-8") + tests_dir = tmp_path / "tests" + tests_dir.mkdir() + (tests_dir / "test_uses_sub.py").write_text("from mypkg import sub_func\n", encoding="utf-8") + (tests_dir / "test_uses_other.py").write_text( + "from mypkg import other_func\n", encoding="utf-8" + ) + (tests_dir / "test_escape.py").write_text("import mypkg\nx = mypkg\n", encoding="utf-8") + consumers = {"tests.test_uses_sub", "tests.test_uses_other", "tests.test_escape"} + return _bundle( + direct_importers={ + "shared.mypkg._sub": {"shared.mypkg"}, + "shared.mypkg._other": {"shared.mypkg"}, + "shared.mypkg": set(consumers), + }, + package_downstream={"shared.mypkg": set(consumers)}, + all_modules={"shared.mypkg", "shared.mypkg._sub", "shared.mypkg._other"} | consumers, + all_test_modules=consumers, + barrel_exports={ + "shared.mypkg": { + "sub_func": {"shared.mypkg._sub"}, + "other_func": {"shared.mypkg._other"}, + } + }, + repo_root=tmp_path, + ) + + +def test_transparent_walk_filters_unrelated_barrel_consumers(tmp_path: Path) -> None: + bundle = _closure_fixture(tmp_path) + closure = selector._walk_upstream_combined(bundle, ["shared.mypkg._sub"]) + # Symbol user of the tainted submodule: selected. + assert "tests.test_uses_sub" in closure + # Unbounded consumer (module object escapes): conservatively selected. + assert "tests.test_escape" in closure + # Consumer of the OTHER submodule's symbol: skipped — the win. + assert "tests.test_uses_other" not in closure + + +def test_opaque_walk_keeps_pre_3182_behavior(tmp_path: Path) -> None: + bundle = _closure_fixture(tmp_path) + closure = selector._walk_upstream_combined(bundle, ["shared.mypkg._sub"], barrel_aware=False) + assert {"tests.test_uses_sub", "tests.test_uses_other", "tests.test_escape"} <= closure + + +def test_transparent_walk_is_subset_of_opaque_walk(tmp_path: Path) -> None: + bundle = _closure_fixture(tmp_path) + transparent = selector._walk_upstream_combined(bundle, ["shared.mypkg._sub"]) + opaque = selector._walk_upstream_combined(bundle, ["shared.mypkg._sub"], barrel_aware=False) + assert transparent <= opaque + + +def test_walk_depths_count_import_distance(tmp_path: Path) -> None: + bundle = _closure_fixture(tmp_path) + depths = selector._walk_upstream_with_depth(bundle, {"shared.mypkg._sub": 0}) + assert depths["shared.mypkg._sub"] == 0 + assert depths["shared.mypkg"] == 1 + assert depths["tests.test_uses_sub"] == 2 + + +def test_changed_barrel_init_taints_every_consumer(tmp_path: Path) -> None: + """Editing the barrel `__init__.py` itself must keep the full + package-mode blast radius — transparency applies only to changes + in submodules BEHIND the barrel.""" + bundle = _closure_fixture(tmp_path) + closure = selector.reverse_closure(bundle, [("shared.mypkg", "shared/mypkg/__init__.py")]) + assert {"tests.test_uses_sub", "tests.test_uses_other", "tests.test_escape"} <= closure + + +def test_barrel_with_unmapped_import_edge_taints_fully(tmp_path: Path) -> None: + """A barrel that imports a submodule but re-exports nothing from + it (analysis gap — empty backed-symbol set on a real edge) must + fall back to full taint.""" + bundle = _bundle( + direct_importers={ + "pkg._hidden": {"pkg"}, + "pkg": {"tests.test_x"}, + }, + all_modules={"pkg", "pkg._hidden", "tests.test_x"}, + all_test_modules={"tests.test_x"}, + # Exports exist, but none are backed by _hidden. + barrel_exports={"pkg": {"y": {"pkg._y"}}}, + repo_root=tmp_path, + ) + closure = selector._walk_upstream_combined(bundle, ["pkg._hidden"]) + assert "tests.test_x" in closure + + +def test_walk_without_direct_api_falls_back_to_transitive(tmp_path: Path) -> None: + """Graphs lacking ``find_modules_that_directly_import`` (older + grimp, hand-rolled stubs) must keep the pre-#3182 transitive + behavior: transparency silently off, closure unchanged.""" + + class _TransitiveOnlyGraph: + def find_downstream_modules(self, module: str, *, as_package: bool = False) -> set[str]: + return {"consumer_a", "consumer_b"} if module == "m" else set() + + bundle = selector.GraphBundle( + graph=_TransitiveOnlyGraph(), + all_modules=set(), + all_test_modules=set(), + dynamic_import_modules=set(), + missing_source_paths=[], + bare_name_upstream={}, + ) + assert selector._walk_upstream_combined(bundle, ["m"]) == {"m", "consumer_a", "consumer_b"} + + +# ---------------------------------------------------------------------- +# `_run_narrow_or_fallback` — never-zero ratchet + output ordering. +# ---------------------------------------------------------------------- + + +def _drive_narrow( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + bundle: object, + changed_paths: list[str], +) -> tuple[int, str]: + """Run ``_run_narrow_or_fallback`` in-process against a synthetic + repo: stub git (clean baseline, given diff) and graph build.""" + import contextlib + import io as _io_mod + + fake_head = "0" * 39 + "a" + fake_baseline = "0" * 39 + "b" + + def fake_run_git(args: list[str], cwd: Path | None = None) -> tuple[int, str, str]: + if args[:2] == ["rev-parse", "HEAD"]: + return 0, fake_head + "\n", "" + if args[:2] == ["rev-parse", "--abbrev-ref"]: + return 0, "main\n", "" + if args[:2] == ["merge-base", "HEAD"]: + return 0, fake_baseline + "\n", "" + if args[:1] == ["merge-base"] and "--is-ancestor" in args: + return 0, "", "" + if args[:2] == ["cat-file", "-e"]: + return 0, "", "" + if args[:1] == ["diff"]: + return 0, "".join(p + "\n" for p in changed_paths), "" + return 0, "", "" + + monkeypatch.setattr(selector._io, "_run_git", fake_run_git) + monkeypatch.setattr(selector._cli, "build_graph", lambda repo_root: bundle) + monkeypatch.delenv("PYTEST_ARGS_RAW", raising=False) + monkeypatch.delenv("EGG_AGENT_ROLE", raising=False) + monkeypatch.chdir(tmp_path) + + stdout = _io_mod.StringIO() + with contextlib.redirect_stdout(stdout): + rc = selector._run_narrow_or_fallback(tmp_path) + return rc, stdout.getvalue() + + +def test_never_zero_ratchet_falls_back_to_opaque_selection( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """When transparency filters EVERY test for a changed module but + the opaque walk still reaches some, the opaque selection must be + used — transparency may sharpen a selection, never zero it out.""" + bundle = _closure_fixture(tmp_path) + # Remove the sub-symbol consumer and the escape consumer so the + # transparent walk from _sub reaches no test at all. + (tmp_path / "tests" / "test_uses_sub.py").write_text( + "from mypkg import other_func\n", encoding="utf-8" + ) + (tmp_path / "tests" / "test_escape.py").write_text( + "from mypkg import other_func\n", encoding="utf-8" + ) + bundle._usage_cache.clear() + + rc, out = _drive_narrow(monkeypatch, tmp_path, bundle, ["shared/mypkg/_sub.py"]) + assert rc == 0 + selected = out.splitlines() + # Opaque rescue: all three barrel consumers selected, no full-suite + # fallback (which would emit the four test ROOT directories). + assert sorted(selected) == [ + "tests/test_escape.py", + "tests/test_uses_other.py", + "tests/test_uses_sub.py", + ] + record = json.loads( + (tmp_path / ".egg-state" / "selection" / ("0" * 39 + "a.json")).read_text(encoding="utf-8") + ) + assert record["mode"] == "narrow" + assert record["trigger"] == "none" + + +def test_truly_zero_downstream_still_triggers_full_suite( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A changed module with no test consumers under EITHER walk keeps + the pre-#3182 blind-spot trigger.""" + (tmp_path / "shared").mkdir() + (tmp_path / "shared" / "orphan.py").write_text("x = 1\n", encoding="utf-8") + bundle = _bundle(all_modules={"shared.orphan"}, repo_root=tmp_path) + + rc, out = _drive_narrow(monkeypatch, tmp_path, bundle, ["shared/orphan.py"]) + assert rc == 0 + assert out.splitlines() == list(selector.TEST_ROOT_DIRS) + record = json.loads( + (tmp_path / ".egg-state" / "selection" / ("0" * 39 + "a.json")).read_text(encoding="utf-8") + ) + assert record["mode"] == "full_suite" + assert record["trigger"] == "no downstream tests for changed module: shared.orphan" + + +def test_selected_tests_emitted_direct_importers_first( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """#3182 ordering: a direct importer of the changed module must be + emitted before a transitively-reached test even when alphabetical + order says otherwise; pytest collects in the order given.""" + tests_dir = tmp_path / "tests" + tests_dir.mkdir() + (tests_dir / "test_aa_far.py").write_text("import mid\n", encoding="utf-8") + (tests_dir / "test_zz_direct.py").write_text("import changed\n", encoding="utf-8") + (tmp_path / "shared").mkdir() + (tmp_path / "shared" / "changed.py").write_text("x = 1\n", encoding="utf-8") + bundle = _bundle( + direct_importers={ + "shared.changed": {"tests.test_zz_direct", "shared.mid"}, + "shared.mid": {"tests.test_aa_far"}, + }, + all_modules={ + "shared.changed", + "shared.mid", + "tests.test_zz_direct", + "tests.test_aa_far", + }, + all_test_modules={"tests.test_zz_direct", "tests.test_aa_far"}, + repo_root=tmp_path, + ) + + rc, out = _drive_narrow(monkeypatch, tmp_path, bundle, ["shared/changed.py"]) + assert rc == 0 + assert out.splitlines() == ["tests/test_zz_direct.py", "tests/test_aa_far.py"] From 4fe3dfc583d16baaf720f99502687bdb3f19a8bc Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Fri, 12 Jun 2026 23:21:31 +0000 Subject: [PATCH 2/2] Address review: soften barrel gap justification, note nested-barrel limit, drop superfluous noqa --- docs/guides/testing.md | 15 ++++++++++++--- scripts/select_tests/_graph.py | 27 ++++++++++++++++++++++----- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/docs/guides/testing.md b/docs/guides/testing.md index 2e67ba71f4..435147472a 100644 --- a/docs/guides/testing.md +++ b/docs/guides/testing.md @@ -131,9 +131,18 @@ The algorithm is: full package-mode blast radius. Known accepted gap: a consumer that imports a barrel only for a submodule's import-time side effects (referencing no symbol) is not selected when that - submodule changes — pure barrels bind names; packages whose - import-time behavior is load-bearing (e.g. gateway's `@app.route` - registration) are impure by construction and stay opaque. + submodule changes. This is *wider* than "the barrel is impure": + a perfectly pure re-export barrel can still front a submodule + whose import is load-bearing (registering into a global on + import), and a consumer relying on that registration while using + only an unrelated symbol is missed. The backstop is not the + purity classifier but `make test-all` — full-suite CI ground + truth — exactly as for the dynamic-import heuristic, so a + transparency miss costs a narrowed inner-loop run, never a + merge-gating one. Registration that runs at the barrel's *own* + import time (e.g. gateway's `@app.route`) is the separate case the + classifier *does* catch: it makes the `__init__.py` itself impure, + so the barrel stays opaque. 6. **Map modules → test files.** Intersect the downstream set with the pre-collected set of every `test_*.py` / `*_test.py` file in the graph. The selector emits the resulting set of test file diff --git a/scripts/select_tests/_graph.py b/scripts/select_tests/_graph.py index 234aacbfd5..5a8ac62dde 100644 --- a/scripts/select_tests/_graph.py +++ b/scripts/select_tests/_graph.py @@ -287,7 +287,7 @@ def build_bare_name_upstream_edges(all_modules: set[str], repo_root: Path) -> di try: source = source_path.read_text(encoding="utf-8", errors="replace") tree = ast.parse(source, filename=str(source_path)) - except SyntaxError, OSError, ValueError: # noqa: B014 — PEP 758 form + except SyntaxError, OSError, ValueError: # ValueError covers null-byte source etc. PEP 758 (Python # 3.14+) makes the unparenthesised tuple form the canonical # ``except`` shape and ruff format normalises @@ -341,10 +341,20 @@ def build_bare_name_upstream_edges(all_modules: set[str], repo_root: Path) -> di # # Known accepted gap: a consumer that imports the barrel only for a # submodule's import-time side effects (no symbol reference) is not -# selected when that submodule changes. Pure barrels bind names and -# import submodules — modules whose import-time behaviour is -# load-bearing (e.g. gateway's `@app.route` registration, decision-8 -# of #2261) make their barrel impure and stay opaque. +# selected when that submodule changes. Note this is *wider* than +# "the barrel is impure": a perfectly pure re-export barrel +# (`from ._sub import foo`, `__all__`) can still front a `_sub` whose +# import is load-bearing (e.g. registering into a global on import), +# and a consumer that uses only an `_other`-backed symbol while relying +# on `_sub`'s registration via the barrel import is missed. The +# backstop for this gap is not the purity classifier — it is the same +# one the dynamic-import heuristic leans on: `make test-all` is CI +# ground truth and re-runs the full suite, so a transparency miss +# costs a narrowed inner-loop run, never a merge-gating one. +# Decorator-style registration that runs at the barrel's *own* import +# time (e.g. gateway's `@app.route`, decision-8 of #2261) is a separate +# case the classifier *does* catch: that statement makes the +# `__init__.py` itself impure, so the barrel stays opaque. # ---------------------------------------------------------------------- @@ -780,6 +790,13 @@ def taint_full(module: str, depth: int) -> None: for consumer in _direct_importers(bundle, module): if consumer in full: continue + # Deliberate limitation: a consumer that is itself a + # barrel re-exporting from `module` is `taint_full`'d + # here rather than kept partial with only the symbols it + # re-exports. Transparency therefore stops one level in + # — nested/chained barrels (A re-exports B re-exports a + # submodule) over-select but stay sound. Worth + # revisiting if #3111's program ever nests barrels. used = _used_symbols(bundle, consumer, module) if used is None or used & tainted_symbols: taint_full(consumer, depth + 1)