From 6b7e76f6d2f9a2a9d9dcdd00f679347ff1a999b8 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Fri, 24 Jul 2026 14:11:42 +0000 Subject: [PATCH] Add Playwright coverage-delta CI gate Closes #415. Incident #389 skipped ~190 Playwright tests in one commit with green CI - nothing checked test inventory across a diff, only whether tests that still ran still passed. coverage_delta.py statically parses frontend/tests/**/*.spec.ts titles + skip state at head vs. the PR's merge-base and fails on a removed or newly-skipped title unless .github/coverage-acks.txt carries a matching ack line. Co-Authored-By: Claude Fable 5 --- .github/coverage-acks.txt | 23 + .github/scripts/coverage_delta.py | 559 +++++++++++++++++++ .github/scripts/tests/test_coverage_delta.py | 321 +++++++++++ .github/workflows/coverage-delta.yml | 60 ++ docs/infrastructure.md | 11 + 5 files changed, 974 insertions(+) create mode 100644 .github/coverage-acks.txt create mode 100644 .github/scripts/coverage_delta.py create mode 100644 .github/scripts/tests/test_coverage_delta.py create mode 100644 .github/workflows/coverage-delta.yml diff --git a/.github/coverage-acks.txt b/.github/coverage-acks.txt new file mode 100644 index 000000000..9bbcb4c66 --- /dev/null +++ b/.github/coverage-acks.txt @@ -0,0 +1,23 @@ +# Coverage-delta gate ack file (issue #415, incident #389). +# +# .github/scripts/coverage_delta.py fails a PR when a Playwright test title +# present at the merge-base is either absent at head or went from active to +# skipped, UNLESS this file carries a matching line. Same tether-discipline +# shape as docs_lint.py's in-file ALLOWLIST: mechanical check, human-written +# escape hatch, one line per exception, reason required. +# +# Format (append, never edit/remove someone else's line without checking +# with them first - these are a permanent record of a deliberate call, not +# a scratchpad): +# +# coverage-ack: — +# +# is `::` (nested +# test.describe() titles joined with " > ", matching coverage_delta.py's +# own manifest key). A glob (`*`, `?`, `[...]`) is fine for "this whole +# file" or "this whole describe block" - e.g.: +# +# coverage-ack: frontend/tests/Foo.spec.ts::* — reason +# coverage-ack: frontend/tests/Foo.spec.ts::some describe > * — reason +# +# Lines starting with `#` and blank lines are ignored. diff --git a/.github/scripts/coverage_delta.py b/.github/scripts/coverage_delta.py new file mode 100644 index 000000000..6f233caf5 --- /dev/null +++ b/.github/scripts/coverage_delta.py @@ -0,0 +1,559 @@ +#!/usr/bin/env python3 +""" +Playwright coverage-delta gate. + +Issue #415, incident #389: the /editor route swap (PR #389, 2026-07-23) +skipped ~190 Playwright tests in one commit and CI stayed green, because +nothing checks *test inventory* across a diff - only whether the tests +that still run still pass. This script is a static parser (no browser, +no `npm ci`, no Playwright runtime) over frontend/tests/**/*.spec.ts that: + + 1. inventories every test() title (full describe-chain-qualified) and + its skip state (test.skip, test.describe.skip, or a `testInfo.skip(...)` + call reachable from a beforeEach/the test body itself), + 2. does the same parse against the base ref's merge-base file contents + via `git show` (no checkout juggling - see build_manifest_from_ref), + 3. fails when a title present at base is absent at head, or was active + at base and is skipped at head - UNLESS the PR's diff carries an ack + token: a "coverage-ack: - " line appended to + .github/coverage-acks.txt (same tether discipline as docs_lint.py's + ALLOWLIST: mechanical check, human-written escape hatch, one line per + exception, reason required). + +New tests and un-skipping are always fine and never flagged. + +KNOWN LIMITATIONS (docs_lint.py convention: state them plainly rather than +let them get discovered as a false-positive surprise later): + - This is a static-source parser, not a JS/TS AST. It masks string/ + template-literal/comment contents (same length, so line numbers stay + correct - see mask_source()) and then does bracket-matching over what's + left. Regex literals (`/foo/`) are NOT masked; if one is ever + introduced containing an unbalanced `(`/`)`/`{`/`}` inside the pattern + itself, the scanner's bracket matching for that scope would get + confused. None exist in frontend/tests/ today (checked at write time); + if this ever misparses a file for that reason, mask regex literals too + rather than special-casing the one file. + - `testInfo.skip(, "reason")` is treated as an unconditional skip + of its enclosing scope regardless of what is (this repo's own + convention is always `testInfo.skip(true, "...")` in a top-level + beforeEach - see the #389 file-level skip pattern). A parser can't + evaluate an arbitrary runtime condition anyway; erring toward "treat + it as skipped" is the safe direction for a gate whose whole purpose is + catching skips, not toward silently trusting a condition might be false. + - Dynamic test titles built from a template literal + (`` test(`...${x}...`, ...) ``) are identified by their literal SOURCE + text (placeholders kept as `${x}`, not evaluated) - stable across a + diff, which is what the gate needs, but not the same string Playwright + itself reports at runtime for each loop iteration. + +Run standalone: python3 .github/scripts/coverage_delta.py --base +""" +from __future__ import annotations + +import argparse +import fnmatch +import os +import re +import subprocess +from dataclasses import dataclass +from pathlib import Path +from typing import Optional + +REPO_ROOT = Path(__file__).resolve().parents[2] +TESTS_DIR_REL = "frontend/tests" +ACKS_FILE_REL = ".github/coverage-acks.txt" + +# frontend/tests/perf/ is excluded from CI-gated correctness runs by +# playwright.config.ts's own testIgnore ("**/tests/perf/**") - mirror that +# exclusion here so a perf benchmark's title changes never trip this gate. +EXCLUDED_DIR_PARTS = ("perf",) + + +# --------------------------------------------------------------------------- +# Source masking (docs_lint.py's `_blank` trick: same length, same newline +# positions, so every later offset/line-number stays valid against the +# ORIGINAL text even though string/comment contents are blanked out here). +# --------------------------------------------------------------------------- + + +def _blank_range(chars: list, start: int, end: int) -> None: + for k in range(start, end): + if chars[k] != "\n": + chars[k] = " " + + +def mask_source(text: str) -> str: + """ + Returns a same-length copy of `text` with line-comment, block-comment, + string-literal, and template-literal CONTENTS blanked out (quotes/ + delimiters included) so that bracket-matching over the result never + trips on a `{`/`}`/`(`/`)` that only appears inside a string or a + comment. Positions in the result line up 1:1 with the original. + """ + chars = list(text) + n = len(text) + i = 0 + while i < n: + c = text[i] + if c == "/" and i + 1 < n and text[i + 1] == "/": + j = text.find("\n", i) + if j == -1: + j = n + _blank_range(chars, i, j) + i = j + continue + if c == "/" and i + 1 < n and text[i + 1] == "*": + j = text.find("*/", i + 2) + end = n if j == -1 else j + 2 + _blank_range(chars, i, end) + i = end + continue + if c in ("'", '"'): + j = i + 1 + while j < n: + if text[j] == "\\": + j += 2 + continue + if text[j] == c: + j += 1 + break + j += 1 + _blank_range(chars, i, min(j, n)) + i = j + continue + if c == "`": + j = i + 1 + while j < n: + if text[j] == "\\": + j += 2 + continue + if text[j] == "`": + j += 1 + break + j += 1 + _blank_range(chars, i, min(j, n)) + i = j + continue + i += 1 + return "".join(chars) + + +def line_of(text: str, offset: int) -> int: + return text.count("\n", 0, offset) + 1 + + +def find_matching(text: str, open_idx: int, open_ch: str, close_ch: str) -> int: + """Index of the bracket matching text[open_idx] (which must be open_ch), + scanning on `text` (expected to be mask_source()'d already so brackets + inside strings/comments can't throw the count off). Falls back to + len(text) - 1 if unbalanced (malformed/edge-case source - never crash + the gate over a parse wobble, just under-scope that one call site).""" + depth = 0 + i = open_idx + n = len(text) + while i < n: + c = text[i] + if c == open_ch: + depth += 1 + elif c == close_ch: + depth -= 1 + if depth == 0: + return i + i += 1 + return n - 1 + + +def extract_literal(original: str, pos: int) -> Optional[str]: + """ + Given `pos` pointing at (or just before, modulo whitespace) a string/ + template literal in the ORIGINAL (unmasked) text, return its content + (quotes stripped, common escapes unescaped). Returns None if `pos` + isn't a string/template literal at all (e.g. a dynamic/variable title - + none exist in frontend/tests/ today, checked at write time). + """ + n = len(original) + while pos < n and original[pos] in " \t\r\n": + pos += 1 + if pos >= n or original[pos] not in ("'", '"', "`"): + return None + quote = original[pos] + j = pos + 1 + while j < n: + if original[j] == "\\": + j += 2 + continue + if original[j] == quote: + break + j += 1 + content = original[pos + 1 : min(j, n)] + if quote != "`": + content = content.replace(f"\\{quote}", quote).replace("\\\\", "\\") + return content + + +HEAD_RE = re.compile(r"\btest(\.describe\.skip|\.describe\.only|\.describe|\.skip|\.beforeEach|\.only)?\s*\(") + + +def _classify(group1: Optional[str]) -> str: + return { + None: "test", + ".describe.skip": "describe_skip", + ".describe.only": "describe", + ".describe": "describe", + ".skip": "test_skip", + ".beforeEach": "before_each", + ".only": "test", + }[group1] + + +@dataclass +class _Child: + kind: str + head_start: int + paren_start: int + call_end: int + body_start: Optional[int] + body_end: Optional[int] + + +def _scan_children(masked: str, pos: int, end: int) -> list: + children = [] + i = pos + while i < end: + m = HEAD_RE.search(masked, i, end) + if not m: + break + kind = _classify(m.group(1)) + paren_start = m.end() - 1 + call_end = find_matching(masked, paren_start, "(", ")") + if call_end >= end: + call_end = end - 1 + arrow_idx = masked.find("=>", paren_start, call_end) + body_start = body_end = None + if arrow_idx != -1: + j = arrow_idx + 2 + while j < call_end and masked[j] in " \t\r\n": + j += 1 + if j < call_end and masked[j] == "{": + body_start, body_end = j + 1, find_matching(masked, j, "{", "}") + elif j < call_end: + body_start, body_end = j, call_end + children.append(_Child(kind, m.start(), paren_start, call_end, body_start, body_end)) + i = call_end + 1 + return children + + +SKIP_CALL_RE = re.compile(r"\btestInfo\.skip\s*\(") + + +def _skip_reason(original: str, masked: str, body_start: int, body_end: int) -> Optional[str]: + m = SKIP_CALL_RE.search(masked, body_start, body_end) + if not m: + return None + paren_start = m.end() - 1 + call_end = find_matching(masked, paren_start, "(", ")") + comma = masked.find(",", paren_start, call_end) + if comma == -1: + return None + return extract_literal(original, comma + 1) + + +@dataclass +class TestEntry: + file: str + title: str + skip: bool + reason: Optional[str] + line: int + + @property + def test_id(self) -> str: + return f"{self.file}::{self.title}" + + +def _parse_scope( + original: str, + masked: str, + pos: int, + end: int, + file_rel: str, + describe_stack: list, + inherited_skip: bool, + inherited_reason: Optional[str], +) -> list: + results: list = [] + children = _scan_children(masked, pos, end) + + scope_skip = False + scope_reason = None + for c in children: + if c.kind == "before_each" and c.body_start is not None: + reason = _skip_reason(original, masked, c.body_start, c.body_end) + if reason is not None or SKIP_CALL_RE.search(masked, c.body_start, c.body_end): + scope_skip = True + scope_reason = scope_reason or reason + + for c in children: + if c.kind in ("describe", "describe_skip"): + title = extract_literal(original, c.paren_start + 1) + if title is None: + title = f"" + sub_skip = inherited_skip or scope_skip or (c.kind == "describe_skip") + sub_reason = ( + inherited_reason or scope_reason or ("test.describe.skip" if c.kind == "describe_skip" else None) + ) + if c.body_start is not None: + results.extend( + _parse_scope( + original, + masked, + c.body_start, + c.body_end, + file_rel, + describe_stack + [title], + sub_skip, + sub_reason, + ) + ) + elif c.kind in ("test", "test_skip"): + title = extract_literal(original, c.paren_start + 1) + if title is None: + title = f"" + own_reason = None + own_skip = c.kind == "test_skip" + if c.body_start is not None: + own_reason = _skip_reason(original, masked, c.body_start, c.body_end) + if own_reason is not None or SKIP_CALL_RE.search(masked, c.body_start, c.body_end): + own_skip = True + skip = inherited_skip or scope_skip or own_skip + reason = own_reason or scope_reason or inherited_reason + if skip and reason is None and c.kind == "test_skip": + reason = "test.skip" + results.append( + TestEntry( + file=file_rel, + title=" > ".join(describe_stack + [title]), + skip=skip, + reason=reason, + line=line_of(original, c.head_start), + ) + ) + # kind == "before_each": already folded into scope_skip above. + return results + + +def parse_file(source: str, file_rel: str) -> list: + masked = mask_source(source) + return _parse_scope(source, masked, 0, len(masked), file_rel, [], False, None) + + +# --------------------------------------------------------------------------- +# Manifest building +# --------------------------------------------------------------------------- + + +def _is_excluded(rel_path: str) -> bool: + parts = Path(rel_path).parts + return any(p in EXCLUDED_DIR_PARTS for p in parts) + + +def build_manifest_from_worktree(repo_root: Path, tests_dir_rel: str = TESTS_DIR_REL) -> dict: + manifest: dict = {} + tests_dir = repo_root / tests_dir_rel + if not tests_dir.is_dir(): + return manifest + for path in sorted(tests_dir.rglob("*.spec.ts")): + rel = path.relative_to(repo_root).as_posix() + if _is_excluded(path.relative_to(tests_dir).as_posix()): + continue + source = path.read_text() + for entry in parse_file(source, rel): + manifest[entry.test_id] = entry + return manifest + + +def _git(repo_root: Path, *args: str) -> str: + result = subprocess.run( + ["git", *args], + cwd=repo_root, + capture_output=True, + text=True, + ) + if result.returncode != 0: + raise RuntimeError(f"git {' '.join(args)} failed: {result.stderr.strip()}") + return result.stdout + + +def build_manifest_from_ref(repo_root: Path, ref: str, tests_dir_rel: str = TESTS_DIR_REL) -> dict: + """ + Parses frontend/tests/**/*.spec.ts as they existed at `ref` (a commit- + ish, normally a merge-base sha) using `git show`/`git ls-tree` only - + no `git checkout`, so the caller's actual working tree is never + touched. + """ + manifest: dict = {} + try: + listing = _git(repo_root, "ls-tree", "-r", "--name-only", ref, "--", tests_dir_rel) + except RuntimeError: + return manifest + for rel in listing.splitlines(): + rel = rel.strip() + if not rel or not rel.endswith(".spec.ts"): + continue + if _is_excluded(Path(rel).relative_to(tests_dir_rel).as_posix()): + continue + try: + source = _git(repo_root, "show", f"{ref}:{rel}") + except RuntimeError: + continue + for entry in parse_file(source, rel): + manifest[entry.test_id] = entry + return manifest + + +def merge_base(repo_root: Path, base_ref: str) -> str: + return _git(repo_root, "merge-base", "HEAD", base_ref).strip() + + +# --------------------------------------------------------------------------- +# Ack tokens +# --------------------------------------------------------------------------- + +ACK_LINE_RE = re.compile(r"^coverage-ack:\s*(.+?)\s*—\s*(.+?)\s*$") + + +def load_acks(repo_root: Path, acks_file_rel: str = ACKS_FILE_REL) -> list: + """Returns a list of (pattern, reason, line_no) tuples. Non-matching / + blank / `#`-comment lines are ignored. `pattern` is matched against a + test_id (`::`) with fnmatch - a plain string with no + wildcard is an exact match, `*`/`?`/`[...]` work as globs.""" + path = repo_root / acks_file_rel + if not path.is_file(): + return [] + acks = [] + for i, line in enumerate(path.read_text().splitlines(), start=1): + stripped = line.strip() + if not stripped or stripped.startswith("#"): + continue + m = ACK_LINE_RE.match(stripped) + if m: + acks.append((m.group(1), m.group(2), i)) + return acks + + +def is_acked(test_id: str, acks: list) -> Optional[tuple]: + for pattern, reason, line_no in acks: + if fnmatch.fnmatchcase(test_id, pattern): + return (pattern, reason, line_no) + return None + + +# --------------------------------------------------------------------------- +# Diff +# --------------------------------------------------------------------------- + + +@dataclass +class Violation: + test_id: str + kind: str # "removed" | "newly_skipped" + detail: str + + +def diff_manifests(base_manifest: dict, head_manifest: dict) -> list: + violations = [] + for test_id, base_entry in sorted(base_manifest.items()): + head_entry = head_manifest.get(test_id) + if head_entry is None: + violations.append( + Violation( + test_id, + "removed", + f"present at base (file={base_entry.file}, line={base_entry.line}), " f"absent at head", + ) + ) + continue + if (not base_entry.skip) and head_entry.skip: + violations.append( + Violation( + test_id, + "newly_skipped", + f"active at base, skipped at head (file={head_entry.file}, " + f"line={head_entry.line}, reason={head_entry.reason or 'none given'})", + ) + ) + return violations + + +# --------------------------------------------------------------------------- +# CLI +# --------------------------------------------------------------------------- + + +def run(repo_root: Path, base_ref: str, acks_file_rel: str = ACKS_FILE_REL) -> tuple: + """Returns (unacked_violations, acked_violations, base_sha).""" + base_sha = merge_base(repo_root, base_ref) + base_manifest = build_manifest_from_ref(repo_root, base_sha) + head_manifest = build_manifest_from_worktree(repo_root) + violations = diff_manifests(base_manifest, head_manifest) + acks = load_acks(repo_root, acks_file_rel) + + unacked, acked = [], [] + for v in violations: + hit = is_acked(v.test_id, acks) + if hit: + acked.append((v, hit)) + else: + unacked.append(v) + return unacked, acked, base_sha + + +def main(argv=None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--base", + default=os.environ.get("COVERAGE_DELTA_BASE", "origin/master"), + help="Base ref/sha to diff against (default: $COVERAGE_DELTA_BASE or origin/master)", + ) + parser.add_argument( + "--repo-root", + default=str(REPO_ROOT), + help="Repo root (default: this script's repo)", + ) + args = parser.parse_args(argv) + repo_root = Path(args.repo_root).resolve() + + try: + unacked, acked, base_sha = run(repo_root, args.base) + except RuntimeError as exc: + print(f"::error::coverage_delta.py could not compute a diff: {exc}") + return 1 + + print(f"coverage-delta: base={args.base} (merge-base {base_sha[:12]})") + + for v, (pattern, reason, line_no) in acked: + print( + f"coverage-delta: ACKED [{v.kind}] {v.test_id} - {v.detail} " + f"(.github/coverage-acks.txt:{line_no} pattern=`{pattern}` reason: {reason})" + ) + + for v in unacked: + print( + f"::error::coverage-delta [{v.kind}] {v.test_id} - {v.detail}. " + f"If this is intentional, add a line to .github/coverage-acks.txt: " + f'"coverage-ack: {v.test_id} — " (a glob over file/title also works).' + ) + + if unacked: + print( + f"\n{len(unacked)} unacked coverage regression(s) " f"({len(acked)} acked). See issue #415 / incident #389." + ) + else: + extra = f" ({len(acked)} acked)" if acked else "" + print(f"\ncoverage-delta: clean{extra}.") + + return len(unacked) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/scripts/tests/test_coverage_delta.py b/.github/scripts/tests/test_coverage_delta.py new file mode 100644 index 000000000..5a348904c --- /dev/null +++ b/.github/scripts/tests/test_coverage_delta.py @@ -0,0 +1,321 @@ +""" +Unit + integration tests for coverage_delta.py (issue #415). + +Fixture tests exercise the static parser directly against snippet strings +(no filesystem/git needed). Integration tests build a real scratch git repo +with two commits and drive coverage_delta.run() end to end - this is also +where the two "prove it" synthetic cases from the issue (a removed title, +a new skip) live as permanent regression coverage, not just a one-off local +demo. + +Run: python3 .github/scripts/tests/test_coverage_delta.py +""" + +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(SCRIPTS_DIR)) + +import coverage_delta as cd # noqa: E402 + +REPO_ROOT = SCRIPTS_DIR.parents[1] + + +def entries(source: str, file_rel: str = "frontend/tests/Fixture.spec.ts"): + return cd.parse_file(source, file_rel) + + +def by_title(source: str, title: str, file_rel: str = "frontend/tests/Fixture.spec.ts"): + for e in entries(source, file_rel): + if e.title == title: + return e + raise AssertionError( + f"no entry titled {title!r} in parsed source; got {[e.title for e in entries(source, file_rel)]}" + ) + + +class TestMaskSource(unittest.TestCase): + def test_same_length_and_newline_positions(self): + text = 'const a = "x{y}";\n// comment {\nconst b = `t${1}` /* block {}\nspans lines */;\n' + masked = cd.mask_source(text) + self.assertEqual(len(masked), len(text)) + self.assertEqual(text.count("\n"), masked.count("\n")) + self.assertEqual([i for i, c in enumerate(text) if c == "\n"], [i for i, c in enumerate(masked) if c == "\n"]) + + def test_string_content_is_blanked(self): + masked = cd.mask_source('test("has { and ( inside", () => {});') + self.assertNotIn("{", masked.split("(", 1)[1].split(")", 1)[0]) + + +class TestBasicParsing(unittest.TestCase): + def test_flat_test_is_active(self): + e = by_title('test("a plain test", async ({ page }) => {\n await page.click("x");\n});', "a plain test") + self.assertFalse(e.skip) + self.assertIsNone(e.reason) + + def test_describe_nesting_builds_full_title(self): + src = """ +test.describe("outer", () => { + test.describe("inner", () => { + test("leaf", async () => {}); + }); +}); +""" + e = by_title(src, "outer > inner > leaf") + self.assertFalse(e.skip) + + def test_dynamic_template_title_is_source_stable(self): + src = "for (const x of [1,2]) {\n test(`case ${x}`, async () => {});\n}" + e = by_title(src, "case ${x}") + self.assertFalse(e.skip) + + +class TestSkipDetection(unittest.TestCase): + def test_file_level_before_each_skips_every_test(self): + # The #389 incident pattern: a top-level test.beforeEach(testInfo.skip(true, "...")), + # applied before any test.describe - every test in the file is skipped. + src = """ +test.beforeEach(async ({}, testInfo) => { + testInfo.skip(true, "route swap - see issue #272"); +}); + +test.describe("group", () => { + test("a", async () => {}); + test("b", async () => {}); +}); +""" + for title in ("group > a", "group > b"): + e = by_title(src, title) + self.assertTrue(e.skip, title) + self.assertIn("issue #272", e.reason) + + def test_describe_scoped_before_each_skips_only_that_describe(self): + src = """ +test.describe("skipped group", () => { + test.beforeEach(async ({}, testInfo) => { + testInfo.skip(true, "scoped reason"); + }); + test("a", async () => {}); +}); +test.describe("active group", () => { + test("b", async () => {}); +}); +""" + self.assertTrue(by_title(src, "skipped group > a").skip) + self.assertFalse(by_title(src, "active group > b").skip) + + def test_describe_skip_propagates(self): + src = 'test.describe.skip("group", () => {\n test("a", async () => {});\n});' + e = by_title(src, "group > a") + self.assertTrue(e.skip) + + def test_test_skip_is_individually_skipped(self): + src = 'test.describe("group", () => {\n test.skip("a", async () => {});\n test("b", async () => {});\n});' + self.assertTrue(by_title(src, "group > a").skip) + self.assertFalse(by_title(src, "group > b").skip) + + def test_inline_testinfo_skip_in_test_body(self): + src = """ +test("conditional", async ({}, testInfo) => { + testInfo.skip(true, "inline reason"); +}); +""" + e = by_title(src, "conditional") + self.assertTrue(e.skip) + self.assertEqual(e.reason, "inline reason") + + def test_unrelated_sibling_test_not_skipped_by_inline_skip(self): + src = """ +test("a", async ({}, testInfo) => { + testInfo.skip(true, "only a"); +}); +test("b", async () => {}); +""" + self.assertTrue(by_title(src, "a").skip) + self.assertFalse(by_title(src, "b").skip) + + +class TestAcks(unittest.TestCase): + def test_load_and_exact_match(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / ".github").mkdir() + (root / ".github" / "coverage-acks.txt").write_text( + "# comment line, ignored\n" "coverage-ack: frontend/tests/Foo.spec.ts::a title — reason text\n" + ) + acks = cd.load_acks(root) + self.assertEqual(len(acks), 1) + self.assertIsNotNone(cd.is_acked("frontend/tests/Foo.spec.ts::a title", acks)) + self.assertIsNone(cd.is_acked("frontend/tests/Foo.spec.ts::other title", acks)) + + def test_glob_match(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / ".github").mkdir() + (root / ".github" / "coverage-acks.txt").write_text( + "coverage-ack: frontend/tests/Foo.spec.ts::* — whole file swept\n" + ) + acks = cd.load_acks(root) + self.assertIsNotNone(cd.is_acked("frontend/tests/Foo.spec.ts::group > leaf", acks)) + self.assertIsNone(cd.is_acked("frontend/tests/Bar.spec.ts::group > leaf", acks)) + + def test_malformed_line_is_ignored_not_crashed(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / ".github").mkdir() + (root / ".github" / "coverage-acks.txt").write_text("coverage-ack: missing the dash reason\n") + self.assertEqual(cd.load_acks(root), []) + + +class TestDiffManifests(unittest.TestCase): + def _entry(self, file, title, skip): + return cd.TestEntry(file=file, title=title, skip=skip, reason=None, line=1) + + def test_removed_test_is_a_violation(self): + base = {"f::a": self._entry("f", "a", False)} + head = {} + v = cd.diff_manifests(base, head) + self.assertEqual(len(v), 1) + self.assertEqual(v[0].kind, "removed") + + def test_newly_skipped_is_a_violation(self): + base = {"f::a": self._entry("f", "a", False)} + head = {"f::a": self._entry("f", "a", True)} + v = cd.diff_manifests(base, head) + self.assertEqual(len(v), 1) + self.assertEqual(v[0].kind, "newly_skipped") + + def test_new_test_is_fine(self): + base = {} + head = {"f::a": self._entry("f", "a", False)} + self.assertEqual(cd.diff_manifests(base, head), []) + + def test_unskip_is_fine(self): + base = {"f::a": self._entry("f", "a", True)} + head = {"f::a": self._entry("f", "a", False)} + self.assertEqual(cd.diff_manifests(base, head), []) + + def test_unchanged_skip_state_is_fine(self): + base = {"f::a": self._entry("f", "a", True)} + head = {"f::a": self._entry("f", "a", True)} + self.assertEqual(cd.diff_manifests(base, head), []) + + +def _git(cwd, *args): + subprocess.run(["git", *args], cwd=cwd, check=True, capture_output=True, text=True) + + +def _write(path: Path, text: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + + +class TestEndToEndGitIntegration(unittest.TestCase): + """ + Builds a real scratch git repo (two commits: base, head) and runs + coverage_delta.run() against it exactly as CI would. This is the + permanent regression home for the issue's two synthetic proof cases. + """ + + def _init_repo(self, tmp: Path) -> None: + _git(tmp, "init", "-q") + _git(tmp, "config", "user.email", "test@example.com") + _git(tmp, "config", "user.name", "Test") + _write( + tmp / "frontend" / "tests" / "Sample.spec.ts", + 'test.describe("group", () => {\n' + ' test("stays active", async () => {});\n' + ' test("will be removed", async () => {});\n' + ' test("will be skipped", async () => {});\n' + "});\n", + ) + _git(tmp, "add", "-A") + _git(tmp, "commit", "-q", "-m", "base") + _git(tmp, "branch", "-q", "base-branch") + + def test_clean_when_nothing_changes(self): + with tempfile.TemporaryDirectory() as tmpdir: + tmp = Path(tmpdir) + self._init_repo(tmp) + unacked, acked, base_sha = cd.run(tmp, "base-branch") + self.assertEqual(unacked, []) + self.assertEqual(acked, []) + + def test_synthetic_case_removed_title_fails(self): + with tempfile.TemporaryDirectory() as tmpdir: + tmp = Path(tmpdir) + self._init_repo(tmp) + _write( + tmp / "frontend" / "tests" / "Sample.spec.ts", + 'test.describe("group", () => {\n' + ' test("stays active", async () => {});\n' + ' test("will be skipped", async () => {});\n' + "});\n", + ) + unacked, acked, _ = cd.run(tmp, "base-branch") + kinds = {v.test_id: v.kind for v in unacked} + self.assertIn("frontend/tests/Sample.spec.ts::group > will be removed", kinds) + self.assertEqual(kinds["frontend/tests/Sample.spec.ts::group > will be removed"], "removed") + + def test_synthetic_case_new_skip_fails(self): + with tempfile.TemporaryDirectory() as tmpdir: + tmp = Path(tmpdir) + self._init_repo(tmp) + _write( + tmp / "frontend" / "tests" / "Sample.spec.ts", + 'test.describe("group", () => {\n' + ' test("stays active", async () => {});\n' + ' test("will be removed", async () => {});\n' + ' test.skip("will be skipped", async () => {});\n' + "});\n", + ) + unacked, acked, _ = cd.run(tmp, "base-branch") + kinds = {v.test_id: v.kind for v in unacked} + self.assertEqual( + kinds["frontend/tests/Sample.spec.ts::group > will be skipped"], + "newly_skipped", + ) + + def test_ack_token_excuses_both_synthetic_violations(self): + with tempfile.TemporaryDirectory() as tmpdir: + tmp = Path(tmpdir) + self._init_repo(tmp) + _write( + tmp / "frontend" / "tests" / "Sample.spec.ts", + 'test.describe("group", () => {\n' + ' test("stays active", async () => {});\n' + ' test.skip("will be skipped", async () => {});\n' + "});\n", + ) + _write( + tmp / ".github" / "coverage-acks.txt", + "coverage-ack: frontend/tests/Sample.spec.ts::* — synthetic test-file cleanup, see PR #999\n", + ) + unacked, acked, _ = cd.run(tmp, "base-branch") + self.assertEqual(unacked, []) + self.assertEqual(len(acked), 2) # removed title + newly-skipped title, both acked + + +class TestRealRepoSmoke(unittest.TestCase): + """Sanity checks against this repo's own frontend/tests/ - not a diff + (no base ref assumed reachable in every CI checkout), just confirms the + parser produces a sane, non-empty manifest with the #389/#272 skip + pattern actually detected.""" + + def test_real_manifest_is_non_empty_and_has_known_skips(self): + manifest = cd.build_manifest_from_worktree(REPO_ROOT) + self.assertGreater(len(manifest), 100) + skipped_files = {e.file for e in manifest.values() if e.skip} + self.assertIn("frontend/tests/PDFGenerator.spec.ts", skipped_files) + + def test_perf_dir_is_excluded(self): + manifest = cd.build_manifest_from_worktree(REPO_ROOT) + self.assertFalse(any("/perf/" in e.file for e in manifest.values())) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/coverage-delta.yml b/.github/workflows/coverage-delta.yml new file mode 100644 index 000000000..79325a410 --- /dev/null +++ b/.github/workflows/coverage-delta.yml @@ -0,0 +1,60 @@ +name: Playwright coverage delta + +# Issue #415, incident #389: the /editor route swap (PR #389, 2026-07-23) +# skipped ~190 Playwright tests in one commit and CI stayed green - nothing +# checked test INVENTORY across a diff, only whether the tests that still +# ran still passed. This is a pure static-source check (no browser, no +# `npm ci` - stdlib Python 3 only): parses frontend/tests/**/*.spec.ts test +# titles + skip state at head and at the PR's merge-base +# (.github/scripts/coverage_delta.py), and fails when a title present at +# base is gone or newly skipped at head - UNLESS .github/coverage-acks.txt +# carries a matching "coverage-ack: — " line (same +# tether-discipline shape as docs_lint.py's ALLOWLIST). New tests and +# un-skipping are always fine and never flagged. +# +# Runs on every pull_request (no path filter): the gate's whole reason for +# existing is to catch a test-inventory change that's easy to miss amid an +# otherwise-unrelated-looking diff, so gating its own trigger behind a path +# filter would reintroduce the exact class of blind spot it's meant to +# close. It's pure stdlib Python with no install step, so the always-run +# cost is low. + +on: + pull_request: + workflow_dispatch: + +permissions: + contents: read + +jobs: + coverage-delta-rules: + name: coverage_delta.py parser + diff unit tests + # Fixture + real-repo tests for the parser and diff/ack logic - proves + # the rules themselves behave, independent of any particular PR's diff. + # Mirrors docs-lint.yml's "lint" / "docs-lint-rules" job split. + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: Run coverage_delta.py unit tests + run: python3 .github/scripts/tests/test_coverage_delta.py + + coverage-delta: + name: Coverage delta vs. base ref + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + with: + # Full history so `git merge-base` can find the PR's actual base + # commit regardless of default shallow-clone depth. + fetch-depth: 0 + - name: Run coverage-delta gate + env: + PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + if [ -n "$PR_BASE_SHA" ]; then + python3 .github/scripts/coverage_delta.py --base "$PR_BASE_SHA" + else + # workflow_dispatch or any non-PR trigger - fall back to the + # script's own default (origin/master). + python3 .github/scripts/coverage_delta.py + fi diff --git a/docs/infrastructure.md b/docs/infrastructure.md index 033663d67..856b9905c 100644 --- a/docs/infrastructure.md +++ b/docs/infrastructure.md @@ -223,6 +223,17 @@ directly. own Node (v24.11.1 as of `v1.57.0`) isn't the version this repo's other CI jobs use. Bumping the pinned Playwright version means bumping this image tag in lockstep in both workflow files. +- `coverage-delta.yml` (issue #415, incident #389: the `/editor` route swap + skipped ~190 Playwright tests in one commit and CI stayed green) runs on + every `pull_request` with no path filter, pure stdlib Python, no + container/`npm ci` needed — `.github/scripts/coverage_delta.py` statically + parses `frontend/tests/**/*.spec.ts` test titles + skip state at head and + at the PR's merge-base (`git show`, never a `git checkout`), and fails + when a title present at base is gone or newly skipped at head unless + `.github/coverage-acks.txt` carries a matching + `coverage-ack: — ` line (same tether discipline + as `docs_lint.py`'s in-file `ALLOWLIST`). New tests and un-skipping are + always fine. ## Push policy