diff --git a/AGENTS.md b/AGENTS.md index 1522b313..7033401a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2044,6 +2044,21 @@ paths select nothing. Add new production areas by editing `AFFECTED_RULES` and `FEATURES` in `tests/e2e/policy.py`. Classify a new E2E module by adding its module/class prefix to the right feature's `selectors`. +`--affected` fails closed: when Git change discovery itself fails (invalid +`--base` ref, unusable checkout, missing/failing `git`, or untracked-file +discovery failure) the runner exits nonzero with a diagnostic instead of +reporting zero affected tests. A genuine empty diff remains a successful no-op. +`--base` must name a single revision: a leading `-` is rejected, the base is +verified to resolve to one commit (a range like `a..b` compares commit to +commit and would drop the working tree), and the diff terminates revision +parsing with `--`. An option-like, range or path-like base therefore fails +closed instead of quietly answering a different question. + +Benchmark/profile runs never train history. `--profile` / `--no-seed-cache` +**and** their environment equivalents (`PRKS_E2E_PROFILE`, +`PRKS_E2E_SEED_CACHE=0`) suppress `.tests/e2e-timings.json` and last-failed +persistence; `policy.benchmark_modes()` is the single decision. + Optional stress: `tests/e2e/stress_cache_offline.py` — never part of normal iteration or the full gate. diff --git a/docs/e2e-performance.md b/docs/e2e-performance.md index 48d5c923..f22f3287 100644 --- a/docs/e2e-performance.md +++ b/docs/e2e-performance.md @@ -94,6 +94,15 @@ parallel run to a serial run when judging an individual-test optimization. instrumented or cache-off durations into them would poison later normal runs. Profile/slowest output for the current run still prints as usual. +The same applies when the run is configured through the environment instead of +the flags: `PRKS_E2E_PROFILE=1` or `PRKS_E2E_SEED_CACHE=0` make the run +non-representative on their own, so the runner suppresses history persistence +for them too and prints which benchmark mode is active. `tests/e2e/policy.py` +(`benchmark_modes()`) makes that decision once, from the effective +configuration after CLI flags are exported into the environment, and the +infrastructure-profile report prints for `PRKS_E2E_PROFILE=1` exactly as it +does for `--profile`. Those exports last for the one runner invocation. + ### Opt-in hang diagnostics and Chromium recycle `PRKS_E2E_DIAGNOSTIC=1` prints privacy-safe stage heartbeats (`START`, diff --git a/tests/e2e/harness.py b/tests/e2e/harness.py index 77783806..fe36f4ce 100644 --- a/tests/e2e/harness.py +++ b/tests/e2e/harness.py @@ -29,29 +29,31 @@ installed_chromium_revisions, playwright_chromium_revision, ) +from tests.e2e.policy import ( + PROFILE_ENV, + SEED_CACHE_ENV, + env_flag_enabled, +) REPO = Path(__file__).resolve().parents[2] HOST = "127.0.0.1" READY_TIMEOUT_S = 25.0 STOP_TIMEOUT_S = 8.0 -_FALSE_ENV_VALUES = {"0", "false", "no", "off"} _ACTIVE_PROFILE = None _SEED_CACHE_TMP = None _SEED_SNAPSHOTS = {} def _env_enabled(name: str, default: bool = False) -> bool: - raw = os.environ.get(name) - if raw is None: - return default - return raw.strip().lower() not in _FALSE_ENV_VALUES + """Shared with the runner's benchmark-mode decision (tests.e2e.policy).""" + return env_flag_enabled(name, default) def start_test_profile(test_id: str) -> None: """Begin opt-in per-test infrastructure profiling for the runner.""" global _ACTIVE_PROFILE - if not _env_enabled("PRKS_E2E_PROFILE"): + if not _env_enabled(PROFILE_ENV): _ACTIVE_PROFILE = None return _ACTIVE_PROFILE = {"test_id": test_id, "phases": {}} @@ -81,7 +83,7 @@ def finish_test_profile(test_id: str) -> dict: def seed_cache_enabled() -> bool: """Worker-local immutable seed snapshots are on unless explicitly disabled.""" - return _env_enabled("PRKS_E2E_SEED_CACHE", default=True) + return _env_enabled(SEED_CACHE_ENV, default=True) def diagnostic_enabled() -> bool: diff --git a/tests/e2e/policy.py b/tests/e2e/policy.py index 45b7457d..1c84c2a0 100644 --- a/tests/e2e/policy.py +++ b/tests/e2e/policy.py @@ -1,6 +1,7 @@ """Declarative E2E selection: tiers, feature groups, affected-file mapping. -Pure logic — no Playwright, no subprocesses. Covered by +Pure selection logic — no Playwright, no browser; the only subprocesses are +read-only Git queries for ``--affected`` discovery. Covered by `tests/test_e2e_policy.py`. The runner (`tests/e2e/run.py`) is the only entry point that applies these selections to a real Chromium run. """ @@ -9,10 +10,57 @@ import json import os import subprocess +import tempfile from pathlib import Path LAST_FAILED_PATH = Path(".tests") / "e2e-last-failed.json" +# --------------------------------------------------------------------------- +# Effective runtime configuration (CLI flags and the equivalent environment) +# --------------------------------------------------------------------------- + +# Runner/harness env switches. The harness reads them to configure a run; the +# runner reads the same names to decide whether a run may train history. +PROFILE_ENV = "PRKS_E2E_PROFILE" +SEED_CACHE_ENV = "PRKS_E2E_SEED_CACHE" + +FALSE_ENV_VALUES = frozenset({"0", "false", "no", "off"}) + + +def env_flag_enabled(name: str, default: bool = False, environ=None) -> bool: + """Canonical truthiness for PRKS E2E env switches (unset → default).""" + env = os.environ if environ is None else environ + raw = env.get(name) + if raw is None: + return default + return raw.strip().lower() not in FALSE_ENV_VALUES + + +def benchmark_modes(environ=None) -> tuple: + """Active non-representative modes for the *effective* configuration. + + ``--profile`` adds instrumentation overhead and ``--no-seed-cache`` measures + a slower non-default configuration; both are also reachable through + ``PRKS_E2E_PROFILE`` / ``PRKS_E2E_SEED_CACHE`` without the matching flag. + The runner exports its flags into the environment before asking, so this is + the single decision covering CLI- and environment-driven benchmark runs. + """ + modes = [] + if env_flag_enabled(PROFILE_ENV, environ=environ): + modes.append("profile") + if not env_flag_enabled(SEED_CACHE_ENV, default=True, environ=environ): + modes.append("no-seed-cache") + return tuple(modes) + + +class ChangeDiscoveryError(RuntimeError): + """Git could not report the working-tree changes ``--affected`` needs. + + Distinct from "Git reported no changes": the first must fail closed, the + second is an ordinary successful no-op. + """ + + # --------------------------------------------------------------------------- # Tiers # --------------------------------------------------------------------------- @@ -770,6 +818,56 @@ def select_affected(all_ids, changed_paths): ) +def _git_lines(repo: Path, args, what: str) -> list: + """Run one read-only git command; raise ChangeDiscoveryError on any failure. + + Git failures (missing executable, damaged checkout, invalid base ref) must + never be indistinguishable from "no changes" — see fail-closed note on + ``list_changed_paths``. + """ + cmd = ["git", "-C", str(repo), *args] + try: + proc = subprocess.run(cmd, capture_output=True, text=True, check=False) + except OSError as exc: + raise ChangeDiscoveryError( + "%s failed: could not run `git %s` (%s)" + % (what, " ".join(args), exc.__class__.__name__) + ) from exc + if proc.returncode != 0: + detail = [line.strip() for line in (proc.stderr or "").splitlines() if line.strip()] + raise ChangeDiscoveryError( + "%s failed: `git %s` exited %d%s" + % ( + what, + " ".join(args), + proc.returncode, + (" — " + detail[0]) if detail else "", + ) + ) + return [line.strip() for line in proc.stdout.splitlines() if line.strip()] + + +def _verify_base_revision(repo: Path, base: str) -> None: + """--affected compares the working tree to ONE base commit. + + Git accepts a range (`a..b`) and silently switches `git diff` to + commit-vs-commit, dropping the working tree from the comparison, so a base + that is not a single resolvable revision has to fail closed rather than + answer a different question. + """ + try: + _git_lines( + repo, + ["rev-parse", "--verify", "%s^{commit}" % base], + "base revision check vs %s" % base, + ) + except ChangeDiscoveryError as exc: + raise ChangeDiscoveryError( + "invalid --base %r: not a single revision this repository resolves " + "(a range like 'a..b', a path, or an unknown ref) — %s" % (base, exc) + ) from exc + + def list_changed_paths(repo: Path, base: str | None = None, include_untracked=True): """Working-tree changes vs base (default: HEAD). Explicit --base overrides. @@ -777,57 +875,51 @@ def list_changed_paths(repo: Path, base: str | None = None, include_untracked=Tr against HEAD (or against `base` when provided). Includes Added/Copied/ Modified/Renamed/Deleted (D). Also includes untracked files under frontend/, backend/, tests/e2e/, tools/, scripts/ when include_untracked. + + Fails closed: any Git/change-discovery failure raises ChangeDiscoveryError + instead of degrading to an empty (and therefore "nothing affected") list. + A genuinely empty diff still returns []. + + `base` must name a single revision. A leading "-" is rejected (git would + parse it as an option), the base is then verified to resolve to one commit + (a range or a path does not), and the diff terminates revision parsing with + "--". Each of those otherwise answers a different question, or exits 0 with + no paths at all, which is the fail-open this guards against. """ repo = Path(repo) + if base is not None and base.startswith("-"): + raise ChangeDiscoveryError( + "invalid --base %r: a revision cannot start with '-' " + "(git would read it as an option and report no changes)" % base + ) + if base is not None and base != "HEAD": + _verify_base_revision(repo, base) ref = base or "HEAD" paths = [] # Staged + unstaged vs ref — include deletes so removed production/E2E # files still drive feature selection. - cmd = ["git", "-C", str(repo), "diff", "--name-only", "--diff-filter=ACMRD", ref] - try: - out = subprocess.check_output(cmd, text=True, stderr=subprocess.DEVNULL) - except (subprocess.CalledProcessError, FileNotFoundError): - out = "" - for line in out.splitlines(): - line = line.strip() - if line: - paths.append(line) + for line in _git_lines( + repo, + ["diff", "--name-only", "--diff-filter=ACMRD", ref, "--"], + "change discovery vs %s" % ref, + ): + paths.append(line) # Also include staged-only relative to HEAD when base is HEAD — already covered # by diff HEAD. When base is another ref, also include uncommitted local work: if base and base != "HEAD": - try: - local = subprocess.check_output( - [ - "git", - "-C", - str(repo), - "diff", - "--name-only", - "--diff-filter=ACMRD", - "HEAD", - ], - text=True, - stderr=subprocess.DEVNULL, - ) - for line in local.splitlines(): - line = line.strip() - if line and line not in paths: - paths.append(line) - except (subprocess.CalledProcessError, FileNotFoundError): - pass + for line in _git_lines( + repo, + ["diff", "--name-only", "--diff-filter=ACMRD", "HEAD", "--"], + "local change discovery vs HEAD", + ): + if line not in paths: + paths.append(line) if include_untracked: - try: - untracked = subprocess.check_output( - ["git", "-C", str(repo), "ls-files", "--others", "--exclude-standard"], - text=True, - stderr=subprocess.DEVNULL, - ) - except (subprocess.CalledProcessError, FileNotFoundError): - untracked = "" - for line in untracked.splitlines(): - line = line.strip() - if not line: - continue + for line in _git_lines( + repo, + ["ls-files", "--others", "--exclude-standard"], + "untracked change discovery", + ): if any( line.startswith(p) or line == p.rstrip("/") for p in UNTRACKED_AFFECTED_PREFIXES @@ -869,14 +961,40 @@ def merge_last_failed(previous_ids, executed_ids, current_failed_ids, known_ids= return merged -def save_last_failed(path: Path, test_ids, meta=None): +def save_last_failed(path: Path, test_ids, meta=None) -> bool: + """Persist unresolved failures atomically; returns False when nothing was written. + + Same temp-file + os.replace commit used for timing history + (tests/e2e/sharding.save_timings): a crash or disk error before the replace + leaves the previous valid file untouched rather than a truncated one that + load_last_failed would report as "no saved failures". + + The temp file gets a unique name so two runners sharing one checkout cannot + write into, commit, or clean up each other's uncommitted state. + """ path = Path(path) - path.parent.mkdir(parents=True, exist_ok=True) payload = { "test_ids": list(test_ids), "meta": meta or {}, } - path.write_text(json.dumps(payload, indent=2, sort_keys=True) + "\n", encoding="utf-8") + tmp = None + try: + path.parent.mkdir(parents=True, exist_ok=True) + handle_fd, raw_tmp = tempfile.mkstemp( + dir=str(path.parent), prefix=path.name + ".", suffix=".tmp" + ) + tmp = Path(raw_tmp) + with os.fdopen(handle_fd, "w", encoding="utf-8") as handle: + handle.write(json.dumps(payload, indent=2, sort_keys=True) + "\n") + os.replace(tmp, path) + return True + except OSError: + if tmp is not None: + try: + tmp.unlink() + except OSError: + pass + return False def load_last_failed(path: Path): diff --git a/tests/e2e/run.py b/tests/e2e/run.py index 2b6b7485..a4bcbf95 100644 --- a/tests/e2e/run.py +++ b/tests/e2e/run.py @@ -42,6 +42,10 @@ from tests.e2e.install_browser import ensure_chromium_installed from tests.e2e.policy import ( LAST_FAILED_PATH, + PROFILE_ENV, + SEED_CACHE_ENV, + ChangeDiscoveryError, + benchmark_modes, format_feature_catalog, full_gate_timeout_s, list_changed_paths, @@ -364,14 +368,21 @@ def _persist_timings(observed, known_ids): save_timings(path, merged) -def _is_benchmark_mode(args) -> bool: - """True for non-representative runs that must not train LPT / last-failed history. +def _apply_runtime_modes(args) -> tuple: + """Export CLI benchmark flags, then decide the effective benchmark modes once. - ``--profile`` adds instrumentation overhead; ``--no-seed-cache`` measures a - slower non-default configuration. Persisting either into - ``.tests/e2e-timings.json`` poisons later normal-gate shard balancing. + ``--profile`` / ``--no-seed-cache`` configure the harness through + PRKS_E2E_PROFILE / PRKS_E2E_SEED_CACHE, and those variables are equally + supported on their own. Exporting first and asking the environment after + keeps one canonical decision for both entry paths, so an env-only benchmark + run cannot train ``.tests/e2e-timings.json`` (poisoning later shard + balancing) or mutate last-failed state that ordinary runs rely on. """ - return bool(getattr(args, "profile", False) or getattr(args, "no_seed_cache", False)) + if args.profile: + os.environ[PROFILE_ENV] = "1" + if args.no_seed_cache: + os.environ[SEED_CACHE_ENV] = "0" + return benchmark_modes(os.environ) # --- worker mode --------------------------------------------------------- @@ -894,7 +905,8 @@ def build_parser(): help=( "Measure per-test E2E infrastructure phases (seed build/clone, server startup, " "browser context, app readiness, async waits, request routing, shutdown). " - "Does not update .tests/e2e-timings.json or last-failed history." + "Does not update .tests/e2e-timings.json or last-failed history " + "(same for PRKS_E2E_PROFILE=1 without this flag)." ), ) parser.add_argument( @@ -902,7 +914,8 @@ def build_parser(): action="store_true", help=( "Disable worker-local immutable fixture seed snapshots for A/B benchmarking. " - "Does not update .tests/e2e-timings.json or last-failed history." + "Does not update .tests/e2e-timings.json or last-failed history " + "(same for PRKS_E2E_SEED_CACHE=0 without this flag)." ), ) # Internal: how the parent invokes one shard. @@ -1026,12 +1039,27 @@ def _resolve_selection(args, all_ids): def main(argv=None) -> int: + """Entry point; runtime-mode env exports stay scoped to this invocation. + + _apply_runtime_modes() publishes the CLI flags through the environment so + workers and the harness see them, which would otherwise leak into a later + in-process main() and silently suppress that run's history persistence. + """ + saved_modes = {name: os.environ.get(name) for name in (PROFILE_ENV, SEED_CACHE_ENV)} + try: + return _main(argv) + finally: + for name, value in saved_modes.items(): + if value is None: + os.environ.pop(name, None) + else: + os.environ[name] = value + + +def _main(argv=None) -> int: args = build_parser().parse_args(sys.argv[1:] if argv is None else argv) - if args.profile: - os.environ["PRKS_E2E_PROFILE"] = "1" - if args.no_seed_cache: - os.environ["PRKS_E2E_SEED_CACHE"] = "0" + active_benchmark_modes = _apply_runtime_modes(args) if args.worker_index is not None: return run_worker( @@ -1048,6 +1076,16 @@ def main(argv=None) -> int: all_ids = discover_test_ids() try: tier, test_ids, note = _resolve_selection(args, all_ids) + except ChangeDiscoveryError as exc: + # Fail closed: an unusable Git comparison is not "nothing changed". + print("affected: %s" % exc, file=sys.stderr) + print( + "refusing to report zero affected tests from failed change discovery; " + "check --base/the checkout, or select tests explicitly with " + "--smoke / --feature.", + file=sys.stderr, + ) + return 2 except ValueError as exc: print(str(exc), file=sys.stderr) return 2 @@ -1078,6 +1116,13 @@ def main(argv=None) -> int: return 0 if tier == "last-failed-stale": + if active_benchmark_modes: + # Clearing the file is a history mutation like any other. + print( + "last-failed: every persisted failure is stale, but benchmark mode " + "(%s) leaves the state untouched" % ",".join(active_benchmark_modes) + ) + return 0 last_failed_path = REPO / LAST_FAILED_PATH cleared = True if last_failed_path.is_file(): @@ -1146,7 +1191,7 @@ def main(argv=None) -> int: test_ids, jobs, timings, args.fail_fast ) - if args.profile and phase_timings: + if "profile" in active_benchmark_modes and phase_timings: totals = {} for phases in phase_timings.values(): for name, seconds in phases.items(): @@ -1176,7 +1221,12 @@ def main(argv=None) -> int: targeted = tier != "full" # Benchmark modes still print profile / slowest for this run, but must not # train LPT history or mutate last-failed — those files drive ordinary gates. - persist_history = not _is_benchmark_mode(args) + persist_history = not active_benchmark_modes + if active_benchmark_modes: + print( + "benchmark mode (%s): not persisting timing or last-failed history" + % ",".join(active_benchmark_modes) + ) if persist_history: _persist_timings(observed, test_ids if not targeted else None) _print_slowest({**timings, **observed} if targeted else observed) @@ -1192,7 +1242,7 @@ def main(argv=None) -> int: ) last_failed_path = REPO / LAST_FAILED_PATH if unresolved: - save_last_failed( + written = save_last_failed( last_failed_path, unresolved, meta={ @@ -1203,7 +1253,14 @@ def main(argv=None) -> int: "failed_this_run": len(failed_ids), }, ) - print("Wrote last-failed (%d) → %s" % (len(unresolved), LAST_FAILED_PATH)) + if written: + print("Wrote last-failed (%d) → %s" % (len(unresolved), LAST_FAILED_PATH)) + else: + # Atomic write never committed: any previous state is still valid. + print( + "warning: could not write last-failed state → %s" % LAST_FAILED_PATH, + file=sys.stderr, + ) elif last_failed_path.is_file(): try: last_failed_path.unlink() diff --git a/tests/test_e2e_policy.py b/tests/test_e2e_policy.py index a8d964c9..da533d01 100644 --- a/tests/test_e2e_policy.py +++ b/tests/test_e2e_policy.py @@ -1,8 +1,10 @@ """Unit coverage for tests.e2e.policy — no Chromium.""" from __future__ import annotations +import contextlib import json import os +import shutil import signal import subprocess import sys @@ -246,6 +248,89 @@ def test_round_trip(self): self.assertEqual(data["test_ids"], ["a.b.C.test_x"]) self.assertEqual(data["meta"]["tier"], "feature") + def test_save_commits_atomically_and_leaves_no_temp_file(self): + with tempfile.TemporaryDirectory() as raw: + path = Path(raw) / "nested" / "e2e-last-failed.json" + self.assertTrue(policy.save_last_failed(path, ["a.b.C.test_x"])) + self.assertEqual(policy.load_last_failed(path)["test_ids"], ["a.b.C.test_x"]) + self.assertEqual( + sorted(q.name for q in path.parent.iterdir()), + ["e2e-last-failed.json"], + ) + + def test_failed_commit_preserves_previous_valid_state(self): + """An interrupted write must never destroy usable last-failed state.""" + with tempfile.TemporaryDirectory() as raw: + path = Path(raw) / "e2e-last-failed.json" + policy.save_last_failed(path, ["a.b.C.test_keep"], meta={"tier": "full"}) + before = path.read_bytes() + + with mock.patch("os.replace", side_effect=OSError("boom")): + self.assertFalse(policy.save_last_failed(path, ["a.b.C.test_new"])) + self.assertEqual(path.read_bytes(), before) + self.assertEqual( + policy.load_last_failed(path)["test_ids"], ["a.b.C.test_keep"] + ) + # The uncommitted temp file must not linger next to the real state. + self.assertEqual( + sorted(q.name for q in path.parent.iterdir()), + ["e2e-last-failed.json"], + ) + + def test_failed_temp_write_preserves_previous_valid_state(self): + def _fail_after_opening(fd, *args, **kwargs): + os.close(fd) + raise OSError("no space left on device") + + with tempfile.TemporaryDirectory() as raw: + path = Path(raw) / "e2e-last-failed.json" + policy.save_last_failed(path, ["a.b.C.test_keep"]) + before = path.read_bytes() + + with mock.patch("os.fdopen", side_effect=_fail_after_opening): + self.assertFalse(policy.save_last_failed(path, ["a.b.C.test_new"])) + self.assertEqual(path.read_bytes(), before) + self.assertEqual( + policy.load_last_failed(path)["test_ids"], ["a.b.C.test_keep"] + ) + self.assertEqual( + sorted(q.name for q in path.parent.iterdir()), + ["e2e-last-failed.json"], + ) + + def test_unavailable_temp_file_preserves_previous_valid_state(self): + with tempfile.TemporaryDirectory() as raw: + path = Path(raw) / "e2e-last-failed.json" + policy.save_last_failed(path, ["a.b.C.test_keep"]) + before = path.read_bytes() + + with mock.patch("tempfile.mkstemp", side_effect=OSError("read-only")): + self.assertFalse(policy.save_last_failed(path, ["a.b.C.test_new"])) + self.assertEqual(path.read_bytes(), before) + + def test_concurrent_writers_do_not_share_a_temp_file(self): + """Two runners in one checkout must never write the same uncommitted file.""" + seen = [] + real_mkstemp = tempfile.mkstemp + + def _record(*args, **kwargs): + fd, name = real_mkstemp(*args, **kwargs) + seen.append(name) + return fd, name + + with tempfile.TemporaryDirectory() as raw: + path = Path(raw) / "e2e-last-failed.json" + with mock.patch("tempfile.mkstemp", side_effect=_record): + self.assertTrue(policy.save_last_failed(path, ["a.b.C.test_one"])) + self.assertTrue(policy.save_last_failed(path, ["a.b.C.test_two"])) + self.assertEqual(len(seen), 2) + self.assertNotEqual(seen[0], seen[1]) + for name in seen: + self.assertEqual(Path(name).parent, path.parent) + self.assertEqual( + policy.load_last_failed(path)["test_ids"], ["a.b.C.test_two"] + ) + def test_corrupt_or_missing_returns_none(self): self.assertIsNone(policy.load_last_failed(Path("/no/such/file.json"))) with tempfile.TemporaryDirectory() as raw: @@ -312,13 +397,22 @@ def test_glob_double_star(self): self.assertFalse(policy._path_matches("backend/server.py", "backend/work_tag_sync.py")) +def _git_ok(stdout: str = "") -> subprocess.CompletedProcess: + return subprocess.CompletedProcess(args=["git"], returncode=0, stdout=stdout, stderr="") + + +def _git_fail(stderr: str, code: int = 128) -> subprocess.CompletedProcess: + return subprocess.CompletedProcess(args=["git"], returncode=code, stdout="", stderr=stderr) + + class ListChangedPathsTests(unittest.TestCase): def test_invokes_git_diff_against_base_including_deletes(self): - with mock.patch("subprocess.check_output") as check: - check.side_effect = [ - "frontend/js/app.js\nfrontend/js/gone.js\n", # diff vs base (incl D) - "frontend/js/app.js\nbackend/x.py\n", # local vs HEAD when base != HEAD - "scripts/e2e\nfrontend/js/new.js\n", # untracked + with mock.patch("subprocess.run") as run: + run.side_effect = [ + _git_ok("deadbeef\n"), # rev-parse: base resolves to one commit + _git_ok("frontend/js/app.js\nfrontend/js/gone.js\n"), # diff vs base (incl D) + _git_ok("frontend/js/app.js\nbackend/x.py\n"), # local vs HEAD when base != HEAD + _git_ok("scripts/e2e\nfrontend/js/new.js\n"), # untracked ] paths = policy.list_changed_paths( Path("/tmp/repo"), base="origin/master", include_untracked=True @@ -329,14 +423,15 @@ def test_invokes_git_diff_against_base_including_deletes(self): self.assertIn("frontend/js/new.js", paths) self.assertIn("scripts/e2e", paths) # Diff filter must include Deleted (D). - first_cmd = check.call_args_list[0][0][0] - self.assertIn("--diff-filter=ACMRD", first_cmd) + diff_cmd = run.call_args_list[1][0][0] + self.assertIn("--diff-filter=ACMRD", diff_cmd) + self.assertIn("rev-parse", run.call_args_list[0][0][0]) def test_untracked_scripts_and_e2e_policy_are_discoverable(self): - with mock.patch("subprocess.check_output") as check: - check.side_effect = [ - "", # diff vs HEAD - "scripts/e2e\ntests/e2e/policy.py\ntmp/scratch.txt\n", + with mock.patch("subprocess.run") as run: + run.side_effect = [ + _git_ok(""), # diff vs HEAD + _git_ok("scripts/e2e\ntests/e2e/policy.py\ntmp/scratch.txt\n"), ] paths = policy.list_changed_paths( Path("/tmp/repo"), base=None, include_untracked=True @@ -345,6 +440,201 @@ def test_untracked_scripts_and_e2e_policy_are_discoverable(self): self.assertIn("tests/e2e/policy.py", paths) self.assertNotIn("tmp/scratch.txt", paths) + def test_genuine_empty_diff_is_not_an_error(self): + """No changes must stay an ordinary empty result — not a discovery failure.""" + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_ok(""), _git_ok("")] + self.assertEqual( + policy.list_changed_paths(Path("/tmp/repo"), include_untracked=True), [] + ) + + def test_invalid_base_ref_fails_closed(self): + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_fail("fatal: bad revision 'origin/nope'")] + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(Path("/tmp/repo"), base="origin/nope") + message = str(ctx.exception) + self.assertIn("origin/nope", message) + self.assertIn("bad revision", message) + + def test_option_like_base_fails_closed_without_running_git(self): + """A leading '-' would be parsed as a git option and report no changes.""" + with mock.patch("subprocess.run") as run: + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths( + Path("/tmp/repo"), base="--relative=definitely-no-such-prefix" + ) + self.assertIn("cannot start with", str(ctx.exception)) + run.assert_not_called() + + def test_diff_commands_terminate_revision_parsing(self): + """`--` stops git reading a base that is also a path as a pathspec.""" + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_ok("deadbeef\n"), _git_ok(""), _git_ok(""), _git_ok("")] + policy.list_changed_paths(Path("/tmp/repo"), base="origin/master") + diff_calls = [ + call[0][0] for call in run.call_args_list if "diff" in call[0][0] + ] + self.assertEqual(len(diff_calls), 2) + for cmd in diff_calls: + self.assertEqual(cmd[-1], "--") + + def test_missing_git_executable_fails_closed(self): + with mock.patch("subprocess.run", side_effect=FileNotFoundError("git")): + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(Path("/tmp/repo")) + self.assertIn("FileNotFoundError", str(ctx.exception)) + + def test_local_diff_failure_against_explicit_base_fails_closed(self): + """The secondary working-tree diff is required too — never silently skipped.""" + with mock.patch("subprocess.run") as run: + run.side_effect = [ + _git_ok("deadbeef\n"), + _git_ok("frontend/js/app.js\n"), + _git_fail("fatal: not a git repository"), + ] + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(Path("/tmp/repo"), base="origin/master") + self.assertIn("local change discovery", str(ctx.exception)) + + def test_untracked_discovery_failure_fails_closed(self): + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_ok(""), _git_fail("fatal: unable to read index")] + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(Path("/tmp/repo"), include_untracked=True) + self.assertIn("untracked change discovery", str(ctx.exception)) + + def test_revision_range_base_fails_closed(self): + """A range switches git diff to commit-vs-commit and drops the working tree.""" + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_fail("fatal: Needed a single revision")] + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(Path("/tmp/repo"), base="release..main") + message = str(ctx.exception) + self.assertIn("release..main", message) + self.assertIn("not a single revision", message) + # Rejected before any diff runs. + self.assertEqual(run.call_count, 1) + self.assertIn("rev-parse", run.call_args_list[0][0][0]) + + def test_untracked_failure_is_irrelevant_when_discovery_is_disabled(self): + with mock.patch("subprocess.run") as run: + run.side_effect = [_git_ok("backend/server.py\n")] + self.assertEqual( + policy.list_changed_paths(Path("/tmp/repo"), include_untracked=False), + ["backend/server.py"], + ) + + +@unittest.skipUnless(shutil.which("git"), "git executable not available") +class ListChangedPathsRealGitTests(unittest.TestCase): + """The fail-closed contract against a real git process, not just mocks.""" + + def _git(self, repo, *args): + subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + ) + + def _seeded_repo(self, repo: Path) -> Path: + """A repo with one commit touching backend/server.py.""" + self._git(repo, "init", "--quiet") + (repo / "backend").mkdir() + (repo / "backend" / "server.py").write_text("x = 1\n", encoding="utf-8") + self._git(repo, "add", "backend/server.py") + self._git( + repo, + "-c", + "user.email=e2e@example.invalid", + "-c", + "user.name=E2E", + "commit", + "--quiet", + "--no-gpg-sign", + "-m", + "seed", + ) + return repo + + def test_unusable_head_fails_closed(self): + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + self._git(repo, "init", "--quiet") + # No commit yet: HEAD cannot be resolved, so discovery must not + # report "nothing changed". + with self.assertRaises(policy.ChangeDiscoveryError): + policy.list_changed_paths(repo) + + def test_clean_checkout_reports_no_changes(self): + with tempfile.TemporaryDirectory() as raw: + repo = self._seeded_repo(Path(raw)) + self.assertEqual(policy.list_changed_paths(repo), []) + (repo / "backend" / "server.py").write_text("x = 2\n", encoding="utf-8") + self.assertEqual(policy.list_changed_paths(repo), ["backend/server.py"]) + + def test_invalid_base_ref_fails_closed(self): + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + self._git(repo, "init", "--quiet") + with self.assertRaises(policy.ChangeDiscoveryError): + policy.list_changed_paths(repo, base="origin/definitely-missing") + + def test_revision_range_base_fails_closed(self): + with tempfile.TemporaryDirectory() as raw: + repo = self._seeded_repo(Path(raw)) + self._git(repo, "branch", "other", "HEAD") + # A range resolves for git, but answers a different question: the + # working tree is left out entirely. + (repo / "frontend").mkdir() + (repo / "frontend" / "app.js").write_text("// x\n", encoding="utf-8") + self._git(repo, "add", "frontend/app.js") + self.assertIn("frontend/app.js", policy.list_changed_paths(repo, base="other")) + with self.assertRaises(policy.ChangeDiscoveryError) as ctx: + policy.list_changed_paths(repo, base="other..HEAD") + self.assertIn("not a single revision", str(ctx.exception)) + + def test_base_that_is_a_path_fails_closed(self): + """`--base backend` is a pathspec to git, and would diff nothing at all.""" + with tempfile.TemporaryDirectory() as raw: + repo = self._seeded_repo(Path(raw)) + with self.assertRaises(policy.ChangeDiscoveryError): + policy.list_changed_paths(repo, base="backend") + + +class BenchmarkModeTests(unittest.TestCase): + """Effective benchmark configuration — CLI flags are exported into env first.""" + + def test_default_environment_is_representative(self): + self.assertEqual(policy.benchmark_modes({}), ()) + self.assertEqual(policy.benchmark_modes({"PRKS_E2E_SEED_CACHE": "1"}), ()) + self.assertEqual(policy.benchmark_modes({"PRKS_E2E_PROFILE": "0"}), ()) + + def test_profile_and_disabled_seed_cache_are_benchmark_modes(self): + self.assertEqual(policy.benchmark_modes({"PRKS_E2E_PROFILE": "1"}), ("profile",)) + self.assertEqual( + policy.benchmark_modes({"PRKS_E2E_SEED_CACHE": "off"}), ("no-seed-cache",) + ) + self.assertEqual( + policy.benchmark_modes( + {"PRKS_E2E_PROFILE": "yes", "PRKS_E2E_SEED_CACHE": "false"} + ), + ("profile", "no-seed-cache"), + ) + + def test_env_truthiness_is_shared_with_the_harness(self): + """One definition decides how the harness reads a switch and how the + runner judges the same switch.""" + from tests.e2e import harness + + for raw, enabled in (("1", True), ("true", True), ("0", False), ("off", False)): + self.assertEqual(policy.env_flag_enabled("X", environ={"X": raw}), enabled) + with mock.patch.dict(os.environ, {"X": raw}, clear=False): + self.assertEqual(harness._env_enabled("X"), enabled) + self.assertTrue(policy.env_flag_enabled("X", default=True, environ={})) + self.assertFalse(policy.env_flag_enabled("X", environ={})) + class ReportBannerTests(unittest.TestCase): def test_full_gate_labelled(self): @@ -440,6 +730,43 @@ def test_affected_docs_only_is_success_noop(self): else: os.environ["PRKS_E2E"] = previous + def test_affected_discovery_failure_fails_closed(self): + """A failed Git query must never look like a clean "nothing affected" run.""" + previous = os.environ.get("PRKS_E2E") + os.environ["PRKS_E2E"] = "1" + try: + from tests.e2e import run as runner + import io + from contextlib import redirect_stderr, redirect_stdout + + out = io.StringIO() + err = io.StringIO() + failure = policy.ChangeDiscoveryError( + "change discovery vs origin/nope failed: " + "`git diff --name-only --diff-filter=ACMRD origin/nope` exited 128 " + "— fatal: bad revision 'origin/nope'" + ) + with mock.patch.object(runner, "ensure_chromium_installed") as ensure: + with mock.patch.object(runner, "run_serial") as serial: + with mock.patch.object( + runner, "list_changed_paths", side_effect=failure + ): + with redirect_stdout(out), redirect_stderr(err): + code = runner.main(["--affected", "--base", "origin/nope"]) + self.assertEqual(code, 2) + diagnostic = err.getvalue() + self.assertIn("origin/nope", diagnostic) + self.assertIn("bad revision", diagnostic) + self.assertIn("refusing to report zero affected tests", diagnostic) + self.assertNotIn("success no-op", out.getvalue()) + ensure.assert_not_called() + serial.assert_not_called() + finally: + if previous is None: + os.environ.pop("PRKS_E2E", None) + else: + os.environ["PRKS_E2E"] = previous + def test_fail_fast_last_failed_persists_unexecuted_priors(self): """Runner must merge on observed completions, not the pre-run selection.""" previous_env = os.environ.get("PRKS_E2E") @@ -877,6 +1204,225 @@ def test_last_failed_stale_unlink_failure_skips_cleared_message(self): else: os.environ["PRKS_E2E"] = previous_env + def _invoke_runner_history_case( + self, + repo, + last_path, + test_id, + known_extra=(), + observed=None, + argv_extra=(), + serial_result=None, + ): + """runner.main() over a temp repo with Chromium and the browser run stubbed. + + Returns (exit_code, _print_slowest mock). + """ + from tests.e2e import run as runner + + if observed is None: + observed = {test_id: 99.0} + if serial_result is None: + serial_result = (True, observed, [], {test_id: {"seed_build": 0.01}}) + with contextlib.ExitStack() as stack: + enter = stack.enter_context + enter(mock.patch.object(runner, "REPO", repo)) + enter(mock.patch.object(runner, "LAST_FAILED_PATH", last_path)) + enter(mock.patch.object(runner, "ensure_chromium_installed")) + enter( + mock.patch.object( + runner, "discover_test_ids", return_value=[test_id, *known_extra] + ) + ) + enter(mock.patch.object(runner, "run_serial", return_value=serial_result)) + print_slow = enter(mock.patch.object(runner, "_print_slowest")) + enter(mock.patch.object(runner, "_run_pointer_capture", return_value=0)) + code = runner.main( + [test_id, "--jobs", "1", "--no-pointer-capture", *argv_extra] + ) + return code, print_slow + + def test_env_only_benchmark_modes_do_not_persist_history(self): + """PRKS_E2E_PROFILE / PRKS_E2E_SEED_CACHE=0 are benchmark runs without a flag.""" + from tests.e2e.sharding import load_timings, save_timings + + test_id = "tests.e2e.fake.BenchTests.test_x" + prior_failed = ["tests.e2e.fake.Other.test_keep"] + prior_timings = {test_id: 1.25, prior_failed[0]: 2.0} + observed = {test_id: 99.0} + + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + (repo / ".tests").mkdir() + timings_path = repo / ".tests" / "e2e-timings.json" + last_path = repo / ".tests" / "e2e-last-failed.json" + save_timings(timings_path, prior_timings) + policy.save_last_failed(last_path, prior_failed, meta={"tier": "full"}) + prior_timings_bytes = timings_path.read_bytes() + prior_last_bytes = last_path.read_bytes() + + def _run(env): + timings_path.write_bytes(prior_timings_bytes) + last_path.write_bytes(prior_last_bytes) + with mock.patch.dict(os.environ, {"PRKS_E2E": "1"}, clear=False): + os.environ.pop("PRKS_E2E_PROFILE", None) + os.environ.pop("PRKS_E2E_SEED_CACHE", None) + os.environ.update(env) + return self._invoke_runner_history_case( + repo, + last_path, + test_id, + known_extra=prior_failed, + observed=observed, + ) + + for env in ( + {"PRKS_E2E_PROFILE": "1"}, + {"PRKS_E2E_SEED_CACHE": "0"}, + {"PRKS_E2E_PROFILE": "1", "PRKS_E2E_SEED_CACHE": "0"}, + ): + code, print_slow = _run(env) + self.assertEqual(code, 0) + print_slow.assert_called_once() + self.assertEqual( + timings_path.read_bytes(), + prior_timings_bytes, + "env=%s must leave timing history untouched" % env, + ) + self.assertEqual( + last_path.read_bytes(), + prior_last_bytes, + "env=%s must leave last-failed untouched" % env, + ) + + # An explicitly-default seed cache is a representative run: it persists. + code, _ = _run({"PRKS_E2E_SEED_CACHE": "1"}) + self.assertEqual(code, 0) + self.assertEqual(load_timings(timings_path)[test_id], 99.0) + + def test_benchmark_flags_do_not_leak_into_a_later_in_process_run(self): + """CLI mode exports are scoped to one main(); the next run persists again.""" + from tests.e2e.sharding import load_timings, save_timings + + test_id = "tests.e2e.fake.BenchTests.test_x" + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + (repo / ".tests").mkdir() + timings_path = repo / ".tests" / "e2e-timings.json" + last_path = repo / ".tests" / "e2e-last-failed.json" + save_timings(timings_path, {test_id: 1.25}) + + with mock.patch.dict(os.environ, {"PRKS_E2E": "1"}, clear=False): + os.environ.pop("PRKS_E2E_PROFILE", None) + os.environ.pop("PRKS_E2E_SEED_CACHE", None) + + code, _ = self._invoke_runner_history_case( + repo, last_path, test_id, argv_extra=["--profile"] + ) + self.assertEqual(code, 0) + self.assertEqual(load_timings(timings_path)[test_id], 1.25) + # The benchmark run must not leave the process in benchmark mode. + self.assertEqual(policy.benchmark_modes(os.environ), ()) + + code, _ = self._invoke_runner_history_case(repo, last_path, test_id) + self.assertEqual(code, 0) + self.assertEqual(load_timings(timings_path)[test_id], 99.0) + + def test_env_only_profiling_still_prints_the_infrastructure_profile(self): + """PRKS_E2E_PROFILE=1 pays the profiling cost, so it must show the report.""" + import io + from contextlib import redirect_stdout + + test_id = "tests.e2e.fake.BenchTests.test_x" + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + (repo / ".tests").mkdir() + last_path = repo / ".tests" / "e2e-last-failed.json" + out = io.StringIO() + with mock.patch.dict(os.environ, {"PRKS_E2E": "1"}, clear=False): + os.environ.pop("PRKS_E2E_SEED_CACHE", None) + os.environ["PRKS_E2E_PROFILE"] = "1" + with redirect_stdout(out): + code, _ = self._invoke_runner_history_case(repo, last_path, test_id) + self.assertEqual(code, 0) + printed = out.getvalue() + self.assertIn("E2E infrastructure profile", printed) + self.assertIn("seed_build", printed) + self.assertIn("benchmark mode (profile)", printed) + + def test_benchmark_mode_does_not_clear_stale_last_failed_state(self): + """Pruning the stale file is a history mutation; benchmark runs skip it.""" + import io + from contextlib import redirect_stdout + from tests.e2e import run as runner + + known = ["tests.e2e.live.T.test_ok"] + stale = ["tests.e2e.gone.Old.test_a"] + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + last_path = repo / "e2e-last-failed.json" + policy.save_last_failed(last_path, stale, meta={"tier": "full"}) + before = last_path.read_bytes() + + out = io.StringIO() + with mock.patch.dict(os.environ, {"PRKS_E2E": "1"}, clear=False): + os.environ.pop("PRKS_E2E_PROFILE", None) + os.environ.pop("PRKS_E2E_SEED_CACHE", None) + with contextlib.ExitStack() as stack: + enter = stack.enter_context + enter(mock.patch.object(runner, "REPO", repo)) + enter(mock.patch.object(runner, "LAST_FAILED_PATH", last_path)) + enter( + mock.patch.object( + runner, "discover_test_ids", return_value=known + ) + ) + ensure = enter( + mock.patch.object(runner, "ensure_chromium_installed") + ) + with redirect_stdout(out): + code = runner.main(["--last-failed", "--profile"]) + self.assertEqual(code, 0) + ensure.assert_not_called() + self.assertTrue(last_path.is_file()) + self.assertEqual(last_path.read_bytes(), before) + self.assertIn("leaves the state untouched", out.getvalue()) + + def test_failed_last_failed_write_warns_and_keeps_previous_state(self): + """A last-failed write that never commits is reported, not silently lost.""" + import io + from contextlib import redirect_stderr, redirect_stdout + from tests.e2e import run as runner + + test_id = "tests.e2e.fake.BenchTests.test_x" + with tempfile.TemporaryDirectory() as raw: + repo = Path(raw) + (repo / ".tests").mkdir() + last_path = repo / ".tests" / "e2e-last-failed.json" + policy.save_last_failed(last_path, ["tests.e2e.fake.Other.test_keep"]) + prior_last_bytes = last_path.read_bytes() + + out = io.StringIO() + err = io.StringIO() + with mock.patch.dict(os.environ, {"PRKS_E2E": "1"}, clear=False): + os.environ.pop("PRKS_E2E_PROFILE", None) + os.environ.pop("PRKS_E2E_SEED_CACHE", None) + with mock.patch.object( + runner, "save_last_failed", return_value=False + ) as save: + with redirect_stdout(out), redirect_stderr(err): + code, _ = self._invoke_runner_history_case( + repo, + last_path, + test_id, + serial_result=(False, {test_id: 12.0}, [test_id], {}), + ) + self.assertNotEqual(code, 0) + save.assert_called_once() + self.assertIn("could not write last-failed state", err.getvalue()) + self.assertNotIn("Wrote last-failed", out.getvalue()) + self.assertEqual(last_path.read_bytes(), prior_last_bytes) + def test_benchmark_modes_do_not_persist_timing_or_last_failed_history(self): """--profile / --no-seed-cache must not train LPT timings or last-failed.""" from tests.e2e import run as runner @@ -909,44 +1455,14 @@ def test_benchmark_modes_do_not_persist_timing_or_last_failed_history(self): prior_last_bytes = last_path.read_bytes() def _invoke(extra_flags): - with mock.patch.object(runner, "REPO", repo): - with mock.patch.object(runner, "LAST_FAILED_PATH", last_path): - with mock.patch.object( - runner, "ensure_chromium_installed" - ): - with mock.patch.object( - runner, - "discover_test_ids", - return_value=[test_id] + prior_failed, - ): - with mock.patch.object( - runner, - "run_serial", - return_value=( - True, - observed, - [], - {test_id: {"seed_build": 0.01}}, - ), - ): - with mock.patch.object( - runner, "_print_slowest" - ) as print_slow: - with mock.patch.object( - runner, - "_run_pointer_capture", - return_value=0, - ): - code = runner.main( - [ - test_id, - "--jobs", - "1", - "--no-pointer-capture", - *extra_flags, - ] - ) - return code, print_slow + return self._invoke_runner_history_case( + repo, + last_path, + test_id, + known_extra=prior_failed, + observed=observed, + argv_extra=extra_flags, + ) for flags in ( ["--profile"],