From 0eda7aa1b9d574b978365f482b5e81b2d2f561eb Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 22 Sep 2026 14:09:07 -0400 Subject: [PATCH 1/4] test: prove the execution registry validator reddens unrelated pull requests `tests/test-execution.toml` requires an entry for every `tests/*.py`, and the preflight validator fails on any unregistered file no matter which pull request is being checked. A test that lands on main without an entry therefore turns every open pull request red until somebody registers it. That happened three times today: #13615's landing left 9 unregistered tests (#13710), then `test_ci_r2_cache_census.py` (#13731), then `test_cmux_settings_jsonc.py` and `test_sync_test_wiring.py` (#13739). This commit adds the regression only, so CI shows it red before the fix. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci-guards.yml | 4 + tests/test-execution.toml | 4 + tests/test_ci_test_execution_registry.py | 197 +++++++++++++++++++++++ 3 files changed, 205 insertions(+) create mode 100644 tests/test_ci_test_execution_registry.py diff --git a/.github/workflows/ci-guards.yml b/.github/workflows/ci-guards.yml index 362939cd1610..6ac17c97f35d 100644 --- a/.github/workflows/ci-guards.yml +++ b/.github/workflows/ci-guards.yml @@ -145,6 +145,10 @@ jobs: fi python3 scripts/ci/validate_test_execution_registry.py "${args[@]}" + - name: Validate Python test execution registry blast radius + if: ${{ matrix.group == 'preflight' }} + run: python3 tests/test_ci_test_execution_registry.py + - name: Validate dev-fleet warm-slot lifecycle if: ${{ matrix.group == 'preflight' }} run: | diff --git a/tests/test-execution.toml b/tests/test-execution.toml index a5be05416726..65485d24bcc1 100644 --- a/tests/test-execution.toml +++ b/tests/test-execution.toml @@ -410,6 +410,10 @@ lane = "linux-guard" path = "tests/test_ci_source_lint_guard_structure.py" lane = "linux-guard" +[[test]] +path = "tests/test_ci_test_execution_registry.py" +lane = "linux-guard" + [[test]] path = "tests/test_ci_testbox_broker_guard.py" lane = "linux-guard" diff --git a/tests/test_ci_test_execution_registry.py b/tests/test_ci_test_execution_registry.py new file mode 100644 index 000000000000..1f5703bd0e09 --- /dev/null +++ b/tests/test_ci_test_execution_registry.py @@ -0,0 +1,197 @@ +#!/usr/bin/env python3 +"""The execution registry validator only fails the pull request at fault. + +An unregistered test that a pull request adds is that pull request's problem. +An unregistered test that was already on the base branch is not, and failing +on it turns every open pull request red for a reason its author cannot fix. +""" + +from __future__ import annotations + +import importlib.util +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[1] +VALIDATOR = ROOT / "scripts" / "ci" / "validate_test_execution_registry.py" + +spec = importlib.util.spec_from_file_location("validate_test_execution_registry", VALIDATOR) +assert spec and spec.loader +validator = importlib.util.module_from_spec(spec) +sys.modules[spec.name] = validator +spec.loader.exec_module(validator) + + +GUARD_WORKFLOW = """\ +name: CI guards +jobs: + workflow-guard-tests: + steps: + - name: Validate the kept test + run: python3 tests/test_kept.py + - name: Run the CLI lane + run: scripts/ci/run_python_test_lane.py --lane macos-cli-no-socket +""" + + +class RegistryBlastRadiusTests(unittest.TestCase): + def make_root(self, *, tests: list[str], registry: str) -> Path: + root = Path(tempfile.mkdtemp(prefix="cmux-test-execution-registry-")) + self.addCleanup(shutil.rmtree, root, ignore_errors=True) + (root / "tests").mkdir() + (root / ".github" / "workflows").mkdir(parents=True) + (root / ".github" / "workflows" / "ci-guards.yml").write_text( + GUARD_WORKFLOW, encoding="utf-8" + ) + for name in tests: + (root / "tests" / name).write_text("#!/usr/bin/env python3\n", encoding="utf-8") + (root / "tests" / "test-execution.toml").write_text(registry, encoding="utf-8") + return root + + def kept_registry(self) -> str: + return 'version = 1\n\n[[test]]\npath = "tests/test_kept.py"\nlane = "linux-guard"\n' + + def test_pre_existing_unregistered_test_warns_instead_of_failing(self) -> None: + root = self.make_root( + tests=["test_kept.py", "test_orphan.py"], + registry=self.kept_registry(), + ) + errors, warnings, _ = validator.validate(root, added=set()) + + self.assertEqual(errors, []) + self.assertTrue( + any("tests/test_orphan.py" in warning for warning in warnings), + warnings, + ) + + def test_unregistered_test_added_by_this_branch_fails(self) -> None: + root = self.make_root( + tests=["test_kept.py", "test_orphan.py"], + registry=self.kept_registry(), + ) + errors, _, _ = validator.validate(root, added={"tests/test_orphan.py"}) + + self.assertTrue( + any("tests/test_orphan.py" in error for error in errors), + errors, + ) + + def test_unknown_added_set_never_fails_on_unregistered_tests(self) -> None: + """No base sha means no comparison, so nothing unrelated can go red.""" + root = self.make_root( + tests=["test_kept.py", "test_orphan.py"], + registry=self.kept_registry(), + ) + errors, warnings, _ = validator.validate(root) + + self.assertEqual(errors, []) + self.assertTrue(warnings) + + def test_stale_entry_fails_even_for_an_unrelated_branch(self) -> None: + root = self.make_root( + tests=["test_kept.py"], + registry=self.kept_registry() + + '\n[[test]]\npath = "tests/test_deleted.py"\nlane = "linux-guard"\n', + ) + errors, _, _ = validator.validate(root, added=set()) + + self.assertTrue( + any("tests/test_deleted.py: registry entry points to a missing test" in error for error in errors), + errors, + ) + + def test_malformed_entry_fails_even_for_an_unrelated_branch(self) -> None: + root = self.make_root( + tests=["test_kept.py"], + registry=self.kept_registry() + + '\n[[test]]\npath = "tests/test_kept.py"\nlane = "manual"\n', + ) + errors, _, _ = validator.validate(root, added=set()) + + self.assertTrue(any("registered more than once" in error for error in errors), errors) + self.assertTrue(any("manual tests require a reason" in error for error in errors), errors) + + def test_dead_lane_fails_even_for_an_unrelated_branch(self) -> None: + root = self.make_root( + tests=["test_kept.py", "test_lane.py"], + registry=self.kept_registry() + + '\n[[test]]\npath = "tests/test_lane.py"\nlane = "no-such-lane"\n', + ) + errors, _, _ = validator.validate(root, added=set()) + + self.assertTrue( + any("lane 'no-such-lane' has no workflow invocation" in error for error in errors), + errors, + ) + + def test_failure_names_the_lane_a_workflow_already_runs(self) -> None: + root = self.make_root(tests=["test_kept.py"], registry='version = 1\n\n[[test]]\npath = "tests/test_lane.py"\nlane = "manual"\nreason = "placeholder"\n') + (root / "tests" / "test_lane.py").write_text("#!/usr/bin/env python3\n", encoding="utf-8") + errors, _, _ = validator.validate(root, added={"tests/test_kept.py"}) + + message = next(error for error in errors if error.startswith("tests/test_kept.py:")) + self.assertIn("[[test]]", message) + self.assertIn('path = "tests/test_kept.py"', message) + self.assertIn('lane = "linux-guard"', message) + self.assertIn("ci-guards.yml", message) + + def test_hint_offers_the_live_lanes_when_no_workflow_runs_the_file(self) -> None: + root = self.make_root(tests=["test_kept.py", "test_orphan.py"], registry=self.kept_registry()) + errors, _, _ = validator.validate(root, added={"tests/test_orphan.py"}) + + message = next(error for error in errors if error.startswith("tests/test_orphan.py:")) + self.assertIn('path = "tests/test_orphan.py"', message) + self.assertIn('lane = ""', message) + self.assertIn("macos-cli-no-socket", message) + + def test_added_tests_are_measured_from_the_merge_base(self) -> None: + root = Path(tempfile.mkdtemp(prefix="cmux-test-execution-registry-git-")) + self.addCleanup(shutil.rmtree, root, ignore_errors=True) + + def git(*args: str) -> None: + subprocess.run( + ["git", "-c", "user.email=ci@example.com", "-c", "user.name=ci", *args], + cwd=root, + check=True, + capture_output=True, + ) + + (root / "tests").mkdir() + git("init", "-b", "main") + (root / "tests" / "test_base.py").write_text("", encoding="utf-8") + git("add", "-A") + git("commit", "-m", "base") + branch_point = subprocess.check_output( + ["git", "rev-parse", "HEAD"], cwd=root, text=True + ).strip() + + git("checkout", "-b", "feature") + (root / "tests" / "test_mine.py").write_text("", encoding="utf-8") + git("add", "-A") + git("commit", "-m", "mine") + + git("checkout", "main") + (root / "tests" / "test_theirs.py").write_text("", encoding="utf-8") + git("add", "-A") + git("commit", "-m", "theirs") + base_tip = subprocess.check_output( + ["git", "rev-parse", "HEAD"], cwd=root, text=True + ).strip() + git("checkout", "feature") + + self.assertNotEqual(base_tip, branch_point) + # Somebody else's unregistered test landed on main after this branch + # started. It must not be attributed to this branch. + self.assertEqual( + validator.newly_added_tests(base_tip, root), + {"tests/test_mine.py"}, + ) + + +if __name__ == "__main__": + unittest.main() From 1f42c9e7bcd42a607348abdbf4825dce3999b672 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 22 Sep 2026 14:09:09 -0400 Subject: [PATCH 2/4] ci: fail only the pull request that adds an unregistered test The validator now compares `tests/` against the merge base with the pull request's base commit. A test file with no registry entry is a hard failure only when this branch added it; one that was already unregistered on the base branch becomes a warning, printed as a GitHub annotation and a step summary note, so an unrelated pull request stays green. Everything a branch can only break by editing the registry itself stays a hard failure: entries pointing at missing tests, malformed or duplicated entries, unsupported requirements, manual entries without a reason, and lanes no workflow invokes. The failure now prints the exact TOML block to paste. When a workflow already runs the file directly, the block names `lane = "linux-guard"` and cites the workflow it derived that from; otherwise it lists the live runner lanes. CI never writes the registry itself. Co-Authored-By: Claude Opus 5 --- .../ci/validate_test_execution_registry.py | 224 +++++++++++++++--- 1 file changed, 187 insertions(+), 37 deletions(-) diff --git a/scripts/ci/validate_test_execution_registry.py b/scripts/ci/validate_test_execution_registry.py index 8cf73326f4f9..c1cda84004ef 100644 --- a/scripts/ci/validate_test_execution_registry.py +++ b/scripts/ci/validate_test_execution_registry.py @@ -1,9 +1,23 @@ #!/usr/bin/env python3 -"""Validate that every Python regression has a declared, live execution path.""" +"""Validate that every Python regression has a declared, live execution path. + +Enforcement is scoped to the pull request that causes the problem. + +A test file with no registry entry is a hard failure only when this pull +request is the one that added it (compared against the merge base). A test +that was already unregistered on the base branch is reported as a warning: +it is somebody else's oversight, and failing here would turn every open pull +request red for a reason its author cannot fix. + +Everything a pull request can only break by editing the registry itself -- +malformed entries, unknown fields, duplicate registrations, entries pointing +at files that no longer exist, lanes no workflow runs -- stays a hard failure. +""" from __future__ import annotations import argparse +import os import re import subprocess import sys @@ -26,6 +40,9 @@ ALLOWED_FIELDS = {"path", "lane", "requirements", "reason"} INVENTORY_LANES = {"legacy", "manual"} SUPPORTED_REQUIREMENTS = {"cmux-cli", "fish"} +# A workflow that names a test file in a `run:` step executes it directly, +# which is exactly what the linux-guard lane means. +DIRECT_RUN_LANE = "linux-guard" def runner_lanes_from_workflow_text(text: str) -> set[str]: @@ -36,7 +53,11 @@ def runner_lanes_from_workflow_text(text: str) -> set[str]: return lanes -def all_workflow_text() -> str: +def workflow_files(workflows: Path = WORKFLOWS) -> list[Path]: + return sorted(workflows.glob("*.y*ml")) + + +def all_workflow_text(workflows: Path = WORKFLOWS) -> str: """Every workflow's text, for asking whether a path is executed anywhere. A Linux guard does not have to live in ci-guards.yml to be live. The @@ -46,42 +67,99 @@ def all_workflow_text() -> str: pull request. """ return "\n".join( - workflow.read_text(encoding="utf-8") - for workflow in sorted(WORKFLOWS.glob("*.y*ml")) + workflow.read_text(encoding="utf-8") for workflow in workflow_files(workflows) ) -def runner_lanes() -> set[str]: +def runner_lanes(workflows: Path = WORKFLOWS) -> set[str]: lanes: set[str] = set() - for workflow in sorted(WORKFLOWS.glob("*.y*ml")): + for workflow in workflow_files(workflows): lanes.update(runner_lanes_from_workflow_text(workflow.read_text(encoding="utf-8"))) return lanes -def newly_added_tests(base_sha: str) -> set[str]: +def workflow_running(path: str, workflows: Path = WORKFLOWS) -> str | None: + """The first workflow whose text already runs `path`, if any.""" + for workflow in workflow_files(workflows): + if path in workflow.read_text(encoding="utf-8"): + return workflow.name + return None + + +def registration_hint(path: str, workflows: Path = WORKFLOWS, live_lanes: set[str] | None = None) -> str: + """The exact TOML block to paste, with a lane filled in where we can derive one.""" + workflow = workflow_running(path, workflows) + if workflow is not None: + lane = DIRECT_RUN_LANE + rationale = f"Lane derived from .github/workflows/{workflow}, which already runs {path}." + else: + lane = "" + lanes = sorted(live_lanes or runner_lanes(workflows)) + choices = ", ".join(lanes) if lanes else "(no runner lane is wired up)" + rationale = ( + f"No workflow runs {path} directly, so pick the lane that should own it.\n" + f" Runner lanes: {choices}.\n" + f' Use lane = "{DIRECT_RUN_LANE}" once a ci-guards.yml step runs the file,\n' + f' or lane = "manual" with a reason = "..." when it cannot run in CI.' + ) + return ( + "\n Paste into tests/test-execution.toml:\n\n" + " [[test]]\n" + f' path = "{path}"\n' + f' lane = "{lane}"\n\n' + f" {rationale}" + ) + + +def merge_base(base_sha: str, root: Path = ROOT) -> str | None: + """The merge base of `base_sha` and HEAD, or None on a shallow clone.""" + try: + result = subprocess.run( + ["git", "merge-base", base_sha, "HEAD"], + cwd=root, + text=True, + capture_output=True, + check=True, + ) + except (OSError, subprocess.CalledProcessError): + return None + return result.stdout.strip() or None + + +def newly_added_tests(base_sha: str, root: Path = ROOT) -> set[str]: + """Test files this branch adds, relative to the merge base with `base_sha`. + + `git diff --diff-filter=A HEAD` is `...HEAD`. When + the clone is too shallow for a merge base -- CI fetches the base commit at + depth 1 -- fall back to diffing the base commit itself, which still names + only files this branch has and the base branch does not. + """ + point = merge_base(base_sha, root) or base_sha output = subprocess.check_output( - ["git", "diff", "--name-only", "--diff-filter=A", base_sha, "HEAD", "--", "tests"], - cwd=ROOT, + ["git", "diff", "--name-only", "--diff-filter=A", point, "HEAD", "--", "tests"], + cwd=root, text=True, ) return {line.strip() for line in output.splitlines() if TEST_PATH_RE.fullmatch(line.strip())} -def main(argv: list[str]) -> int: - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument("--base-sha", default="") - args = parser.parse_args(argv) +def validate( + root: Path = ROOT, + base_sha: str = "", + added: set[str] | None = None, +) -> tuple[list[str], list[str], Counter[str]]: + """Return (hard errors, warnings, lane counts) for the registry under `root`.""" + manifest = root / "tests" / "test-execution.toml" + workflows = root / ".github" / "workflows" errors: list[str] = [] - try: - entries = load_registry(MANIFEST) - except (OSError, ValueError) as error: - print(error, file=sys.stderr) - return 1 + warnings: list[str] = [] + + entries = load_registry(manifest) discovered = { - path.relative_to(ROOT).as_posix() - for path in (ROOT / "tests").glob("test_*.py") + path.relative_to(root).as_posix() + for path in (root / "tests").glob("test_*.py") if path.is_file() } @@ -121,33 +199,105 @@ def main(argv: list[str]) -> int: if count != 1: errors.append(f"{path}: registered more than once") - for path in sorted(discovered - set(paths)): - errors.append(f"{path}: test exists but has no execution registry entry") + # A stale entry can only be produced by the pull request that deletes or + # renames the test, so it stays a hard failure. for path in sorted(set(paths) - discovered): errors.append(f"{path}: registry entry points to a missing test") - live_runner_lanes = runner_lanes() - guard_text = all_workflow_text() + live_runner_lanes = runner_lanes(workflows) + + if added is None and base_sha: + try: + added = newly_added_tests(base_sha, root) + except (OSError, subprocess.CalledProcessError) as error: + # Losing the comparison is an infrastructure problem, not something + # this pull request did. Warn instead of reddening it. + warnings.append( + f"could not compare new tests against {base_sha}: {error}; " + "unregistered tests are reported as warnings only" + ) + + for path in sorted(discovered - set(paths)): + hint = registration_hint(path, workflows, live_runner_lanes) + if added is not None and path in added: + errors.append(f"{path}: added by this pull request with no execution registry entry.{hint}") + else: + warnings.append( + f"{path}: test exists but has no execution registry entry. " + "It was not added by this pull request, so this is a warning " + f"and does not fail the build.{hint}" + ) + + guard_text = all_workflow_text(workflows) for path, entry in sorted(by_path.items()): lane = entry.get("lane") if lane in INVENTORY_LANES: continue - if lane == "linux-guard": + if lane == DIRECT_RUN_LANE: if path not in guard_text: errors.append(f"{path}: linux-guard lane is not run by any workflow") elif lane not in live_runner_lanes: errors.append(f"{path}: lane {lane!r} has no workflow invocation") - if args.base_sha: - try: - added = newly_added_tests(args.base_sha) - except subprocess.CalledProcessError as error: - errors.append(f"could not compare new tests against {args.base_sha}: {error}") - else: - for path in sorted(added): - entry = by_path.get(path) - if entry and entry.get("lane") == "legacy": - errors.append(f"{path}: newly added tests may not enter the legacy migration lane") + if added is not None: + for path in sorted(added): + entry = by_path.get(path) + if entry and entry.get("lane") == "legacy": + errors.append(f"{path}: newly added tests may not enter the legacy migration lane") + + lane_counts = Counter(str(entry["lane"]) for entry in entries if "lane" in entry) + return errors, warnings, lane_counts + + +def report_warnings(warnings: list[str]) -> None: + """Print warnings, annotate them in Actions, and note them in the step summary.""" + if not warnings: + return + + for warning in warnings: + print(f"warning: {warning}") + + if os.environ.get("GITHUB_ACTIONS") == "true": + for warning in warnings: + headline = warning.split("\n", 1)[0] + path = headline.split(":", 1)[0] + if TEST_PATH_RE.fullmatch(path): + print(f"::warning file={path}::{headline}") + else: + print(f"::warning::{headline}") + + summary_path = os.environ.get("GITHUB_STEP_SUMMARY") + if not summary_path: + return + lines = ["### Python test execution registry warnings", ""] + lines.extend(f"- {warning.splitlines()[0]}" for warning in warnings) + lines.extend( + [ + "", + "These tests are unregistered but were not added by this pull request, " + "so they do not fail it. Register them in `tests/test-execution.toml`.", + "", + ] + ) + try: + with open(summary_path, "a", encoding="utf-8") as handle: + handle.write("\n".join(lines) + "\n") + except OSError: + pass + + +def main(argv: list[str]) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--base-sha", default="") + args = parser.parse_args(argv) + + try: + errors, warnings, lane_counts = validate(ROOT, args.base_sha) + except (OSError, ValueError) as error: + print(error, file=sys.stderr) + return 1 + + report_warnings(warnings) if errors: print("Python test execution registry validation failed:", file=sys.stderr) @@ -155,9 +305,9 @@ def main(argv: list[str]) -> int: print(f" - {error}", file=sys.stderr) return 1 - lane_counts = Counter(str(entry["lane"]) for entry in entries if "lane" in entry) summary = ", ".join(f"{lane}={count}" for lane, count in sorted(lane_counts.items())) - print(f"Python test execution registry valid: {len(discovered)} tests ({summary})") + discovered = sum(1 for path in (ROOT / "tests").glob("test_*.py") if path.is_file()) + print(f"Python test execution registry valid: {discovered} tests ({summary})") return 0 From 9d6d0be8973380de27a2316e66fc9ed0b365bcf3 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 22 Sep 2026 14:18:36 -0400 Subject: [PATCH 3/4] ci: drop the duplicate test_sync_test_wiring registration #13738 and #13739 each added a `[[test]]` block for `tests/test_sync_test_wiring.py`. The two merged cleanly and left main with the test registered twice, which the registry validator rejects, so every open pull request is currently red on `guards / workflow-guard-tests / preflight`. Both blocks were identical; this removes the second one. Co-Authored-By: Claude Opus 5 --- tests/test-execution.toml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/test-execution.toml b/tests/test-execution.toml index 65485d24bcc1..6ac05692a691 100644 --- a/tests/test-execution.toml +++ b/tests/test-execution.toml @@ -898,10 +898,6 @@ lane = "legacy" path = "tests/test_ssh_relay_report_tty_retry.py" lane = "legacy" -[[test]] -path = "tests/test_sync_test_wiring.py" -lane = "linux-guard" - [[test]] path = "tests/test_terminal_hang_release_gate.py" lane = "linux-guard" From 7dcd6aa47b0dea394a5988368aa4f1a9077c50f1 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 22 Sep 2026 14:18:39 -0400 Subject: [PATCH 4/4] ci: warn instead of fail on a duplicate registration the base branch already has A duplicate entry was treated as something only the pull request at fault could produce. Today showed otherwise: two pull requests each registering the same test merge cleanly into a duplicate neither one wrote, and it reddens every open pull request exactly like an unregistered test does. The validator now reads the registry at the merge base. A path already duplicated there warns; a duplicate this branch introduces still fails. Parsing moved into `parse_registry(text, label)` so the base revision can be read through `git show` without a file on disk. Co-Authored-By: Claude Opus 5 --- scripts/ci/test_execution_registry.py | 12 ++- .../ci/validate_test_execution_registry.py | 82 ++++++++++++++++--- tests/test_ci_test_execution_registry.py | 29 ++++++- 3 files changed, 108 insertions(+), 15 deletions(-) diff --git a/scripts/ci/test_execution_registry.py b/scripts/ci/test_execution_registry.py index 7827fb3ea4f8..3db07e44b58f 100644 --- a/scripts/ci/test_execution_registry.py +++ b/scripts/ci/test_execution_registry.py @@ -8,11 +8,21 @@ def load_registry(path: Path) -> list[dict[str, object]]: + return parse_registry(path.read_text(encoding="utf-8"), str(path)) + + +def parse_registry(text: str, label: str) -> list[dict[str, object]]: + """Parse registry text. `label` names the source in error messages. + + The validator reads the base branch's registry through `git show`, which + has no path on disk, so parsing is separate from reading. + """ version: object = None tests: list[dict[str, object]] = [] current: Optional[dict[str, object]] = None + path = label - for line_number, raw_line in enumerate(path.read_text(encoding="utf-8").splitlines(), start=1): + for line_number, raw_line in enumerate(text.splitlines(), start=1): line = raw_line.strip() if not line or line.startswith("#"): continue diff --git a/scripts/ci/validate_test_execution_registry.py b/scripts/ci/validate_test_execution_registry.py index c1cda84004ef..d7e099290b2e 100644 --- a/scripts/ci/validate_test_execution_registry.py +++ b/scripts/ci/validate_test_execution_registry.py @@ -10,8 +10,13 @@ request red for a reason its author cannot fix. Everything a pull request can only break by editing the registry itself -- -malformed entries, unknown fields, duplicate registrations, entries pointing -at files that no longer exist, lanes no workflow runs -- stays a hard failure. +malformed entries, unknown fields, entries pointing at files that no longer +exist, lanes no workflow runs -- stays a hard failure. + +Duplicate registrations sit between the two. Two pull requests that each +register the same test merge cleanly into a duplicate nobody wrote, so a +duplicate already present on the base branch warns and one this branch +introduces fails. """ from __future__ import annotations @@ -28,7 +33,7 @@ # to sys.path the way running it as a script does. sys.path.insert(0, str(Path(__file__).resolve().parent)) -from test_execution_registry import load_registry # noqa: E402 +from test_execution_registry import load_registry, parse_registry # noqa: E402 ROOT = Path(__file__).resolve().parents[2] @@ -126,27 +131,65 @@ def merge_base(base_sha: str, root: Path = ROOT) -> str | None: return result.stdout.strip() or None -def newly_added_tests(base_sha: str, root: Path = ROOT) -> set[str]: - """Test files this branch adds, relative to the merge base with `base_sha`. +def comparison_point(base_sha: str, root: Path = ROOT) -> str: + """Where to diff this branch from. - `git diff --diff-filter=A HEAD` is `...HEAD`. When - the clone is too shallow for a merge base -- CI fetches the base commit at - depth 1 -- fall back to diffing the base commit itself, which still names - only files this branch has and the base branch does not. + `git merge-base HEAD` is the `...HEAD` base. When the + clone is too shallow for a merge base -- CI fetches the base commit at + depth 1 -- fall back to the base commit itself, which still names only + files this branch has and the base branch does not. """ - point = merge_base(base_sha, root) or base_sha + return merge_base(base_sha, root) or base_sha + + +def newly_added_tests(base_sha: str, root: Path = ROOT) -> set[str]: + """Test files this branch adds, relative to the merge base with `base_sha`.""" output = subprocess.check_output( - ["git", "diff", "--name-only", "--diff-filter=A", point, "HEAD", "--", "tests"], + [ + "git", + "diff", + "--name-only", + "--diff-filter=A", + comparison_point(base_sha, root), + "HEAD", + "--", + "tests", + ], cwd=root, text=True, ) return {line.strip() for line in output.splitlines() if TEST_PATH_RE.fullmatch(line.strip())} +def duplicated_paths(entries: list[dict[str, object]]) -> set[str]: + counts = Counter( + str(entry["path"]) for entry in entries if isinstance(entry.get("path"), str) + ) + return {path for path, count in counts.items() if count != 1} + + +def base_duplicated_paths(base_sha: str, root: Path = ROOT) -> set[str]: + """Paths already registered twice on the base branch. + + Two pull requests that each register the same test merge cleanly and land a + duplicate nobody wrote, so a duplicate this branch did not create is not + this branch's failure. `tests/test_sync_test_wiring.py` reached main this + way through #13738 and #13739. + """ + point = comparison_point(base_sha, root) + text = subprocess.check_output( + ["git", "show", f"{point}:tests/test-execution.toml"], + cwd=root, + text=True, + ) + return duplicated_paths(parse_registry(text, f"{point}:tests/test-execution.toml")) + + def validate( root: Path = ROOT, base_sha: str = "", added: set[str] | None = None, + base_duplicates: set[str] | None = None, ) -> tuple[list[str], list[str], Counter[str]]: """Return (hard errors, warnings, lane counts) for the registry under `root`.""" manifest = root / "tests" / "test-execution.toml" @@ -195,8 +238,21 @@ def validate( paths.append(path) by_path[path] = entry - for path, count in Counter(paths).items(): - if count != 1: + if base_duplicates is None and base_sha: + try: + base_duplicates = base_duplicated_paths(base_sha, root) + except (OSError, ValueError, subprocess.CalledProcessError) as error: + warnings.append(f"could not read the base registry at {base_sha}: {error}") + already_duplicated = base_duplicates or set() + + for path in sorted(path for path, count in Counter(paths).items() if count != 1): + if path in already_duplicated: + warnings.append( + f"{path}: registered more than once. The duplicate is already on the base " + "branch, so this is a warning and does not fail the build. Delete one of " + "its [[test]] blocks in tests/test-execution.toml." + ) + else: errors.append(f"{path}: registered more than once") # A stale entry can only be produced by the pull request that deletes or diff --git a/tests/test_ci_test_execution_registry.py b/tests/test_ci_test_execution_registry.py index 1f5703bd0e09..1d2754db158a 100644 --- a/tests/test_ci_test_execution_registry.py +++ b/tests/test_ci_test_execution_registry.py @@ -113,9 +113,36 @@ def test_malformed_entry_fails_even_for_an_unrelated_branch(self) -> None: ) errors, _, _ = validator.validate(root, added=set()) - self.assertTrue(any("registered more than once" in error for error in errors), errors) self.assertTrue(any("manual tests require a reason" in error for error in errors), errors) + def test_duplicate_this_branch_introduces_fails(self) -> None: + root = self.make_root( + tests=["test_kept.py"], + registry=self.kept_registry() + "\n" + self.kept_registry().partition("\n\n")[2], + ) + errors, _, _ = validator.validate(root, added=set(), base_duplicates=set()) + + self.assertTrue( + any("tests/test_kept.py: registered more than once" in error for error in errors), + errors, + ) + + def test_duplicate_already_on_the_base_branch_only_warns(self) -> None: + """#13738 and #13739 each registered tests/test_sync_test_wiring.py.""" + root = self.make_root( + tests=["test_kept.py"], + registry=self.kept_registry() + "\n" + self.kept_registry().partition("\n\n")[2], + ) + errors, warnings, _ = validator.validate( + root, added=set(), base_duplicates={"tests/test_kept.py"} + ) + + self.assertEqual(errors, []) + self.assertTrue( + any("tests/test_kept.py: registered more than once" in warning for warning in warnings), + warnings, + ) + def test_dead_lane_fails_even_for_an_unrelated_branch(self) -> None: root = self.make_root( tests=["test_kept.py", "test_lane.py"],