test(tools): add a pytest suite for check_patches.py

.github/workflows/ci.yml

@@ -26,6 +26,10 @@ jobs:
           python-version: "3.12"
       - name: Lint the patch series
         run: python tools/check_patches.py
+      - name: Install pytest
+        run: python -m pip install --upgrade pip pytest
+      - name: Test the linter
+        run: python -m pytest tools/tests -q
 
   sdk-python:
     name: python sdk tests

tools/check_patches.py

@@ -57,8 +57,8 @@ def check(self, name: str, ok: bool, detail: str = "") -> None:
             self.failures.append(name)
 
 
-def _patch_files() -> list[Path]:
-    return sorted(p for p in PATCHES.glob("*.patch"))
+def _patch_files(patches_dir: Path) -> list[Path]:
+    return sorted(p for p in patches_dir.glob("*.patch"))
 
 
 def _strip_comment(added: str) -> str:
@@ -67,14 +67,15 @@ def _strip_comment(added: str) -> str:
     return added[:i] if i != -1 else added
 
 
-def check_series_sync(rep: Report, verbose: bool) -> None:
-    if not SERIES.exists():
+def check_series_sync(rep: Report, patches_dir: Path, verbose: bool) -> None:
+    series = patches_dir / "series"
+    if not series.exists():
         rep.check("series-sync", False, "patches/series is missing")
         return
-    listed = [ln.strip() for ln in SERIES.read_text().splitlines()
+    listed = [ln.strip() for ln in series.read_text().splitlines()
               if ln.strip() and not ln.strip().startswith("#")]
     listed_names = [Path(x).name for x in listed]
-    actual = [p.name for p in _patch_files()]
+    actual = [p.name for p in _patch_files(patches_dir)]
 
     dupes = sorted({n for n in listed_names if listed_names.count(n) > 1})
     missing_file = [n for n in listed_names if n not in actual]        # in series, no file
@@ -94,10 +95,10 @@ def check_series_sync(rep: Report, verbose: bool) -> None:
     rep.check("series-sync", ok, detail)
 
 
-def check_numbering(rep: Report, verbose: bool) -> None:
+def check_numbering(rep: Report, patches_dir: Path, verbose: bool) -> None:
     nums: list[int] = []
     bad_names: list[str] = []
-    for p in _patch_files():
+    for p in _patch_files(patches_dir):
         m = PATCH_NAME_RE.match(p.name)
         if not m:
             bad_names.append(p.name)
@@ -118,14 +119,14 @@ def check_numbering(rep: Report, verbose: bool) -> None:
     rep.check("numbering", ok, detail)
 
 
-def check_bodies(rep: Report, verbose: bool) -> None:
+def check_bodies(rep: Report, patches_dir: Path, verbose: bool) -> None:
     """single-surface + well-formed + uxr-only + no-brand, per patch."""
     multi_file: list[str] = []
     malformed: list[str] = []
     bad_switch: list[str] = []
     brand_hits: list[str] = []
 
-    for p in _patch_files():
+    for p in _patch_files(patches_dir):
         text = p.read_text(encoding="utf-8", errors="replace")
         lines = text.splitlines()
         diff_headers = [ln for ln in lines if ln.startswith("diff --git ")]
@@ -159,20 +160,37 @@ def check_bodies(rep: Report, verbose: bool) -> None:
               else f"brand literal would ship in binary: {brand_hits}")
 
 
+def run_checks(patches_dir: Path, verbose: bool = False) -> Report:
+    """Run every check against `patches_dir` and return the populated Report.
+
+    The one seam the tests use: point this at a fixture directory (holding
+    `*.patch` files and a `series`) to exercise each check in isolation.
+    """
+    rep = Report()
+    check_series_sync(rep, patches_dir, verbose)
+    check_numbering(rep, patches_dir, verbose)
+    check_bodies(rep, patches_dir, verbose)
+    return rep
+
+
 def main() -> int:
     ap = argparse.ArgumentParser(description="Integrity linter for the Fortress patch set.")
     ap.add_argument("-v", "--verbose", action="store_true")
+    ap.add_argument("--patches-dir", type=Path, default=PATCHES,
+                    help="directory holding the *.patch files and series (default: patches/)")
     args = ap.parse_args()
 
-    if not PATCHES.is_dir():
-        print(f"error: {PATCHES} not found (run from the repo root)", file=sys.stderr)
+    patches_dir = args.patches_dir
+    if not patches_dir.is_dir():
+        print(f"error: {patches_dir} not found (run from the repo root)", file=sys.stderr)
         return 1
 
-    print(f"Fortress patch-set linter - {len(_patch_files())} patches in {PATCHES.relative_to(REPO)}/")
-    rep = Report()
-    check_series_sync(rep, args.verbose)
-    check_numbering(rep, args.verbose)
-    check_bodies(rep, args.verbose)
+    try:
+        where = patches_dir.resolve().relative_to(REPO)
+    except ValueError:
+        where = patches_dir
+    print(f"Fortress patch-set linter - {len(_patch_files(patches_dir))} patches in {where}/")
+    rep = run_checks(patches_dir, args.verbose)
 
     print("-" * 60)
     if rep.failures:

tools/tests/test_check_patches.py

@@ -0,0 +1,148 @@
+"""
+Tests for tools/check_patches.py — the patch-set integrity linter.
+
+The linter gates every PR, so a regression in it could silently start passing bad patches:
+the one thing it exists to prevent. Each test builds a tiny fixture patch-set in a temp
+directory and asserts that a given check fails on exactly the violation it targets — and
+that a clean set passes every check.
+
+Run:  pytest tools/tests -q
+"""
+from __future__ import annotations
+import sys
+from pathlib import Path
+
+# Make `import check_patches` work when tests run from the repo root or from tools/.
+sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
+import check_patches as cp  # noqa: E402
+
+
+# --------------------------------------------------------------------------- fixture helpers
+def make_patch(path="src/foo.cc", added=("  int x = 0;",), *,
+               hunk=True, headers=True, second_file=None) -> str:
+    """Build a unified-diff patch. Defaults are clean and well-formed; the keyword args let a
+    test break exactly one property (drop the hunk / the file headers, add a second file)."""
+    lines = [f"diff --git a/{path} b/{path}", "index 1234567..89abcde 100644"]
+    if headers:
+        lines += [f"--- a/{path}", f"+++ b/{path}"]
+    if hunk:
+        lines.append(f"@@ -1,2 +1,{2 + len(added)} @@")
+        lines.append(" context above")
+        lines += [f"+{a}" for a in added]
+        lines.append(" context below")
+    if second_file:  # a second `diff --git` -> two surfaces in one patch
+        lines += [f"diff --git a/{second_file} b/{second_file}",
+                  f"--- a/{second_file}", f"+++ b/{second_file}",
+                  "@@ -1 +1,2 @@", " ctx", "+  added"]
+    return "\n".join(lines) + "\n"
+
+
+def write_set(root: Path, patches: dict[str, str], series: list[str] | None = None) -> Path:
+    """Write patch files + a series into `root`. Series defaults to the files in sorted order."""
+    for name, body in patches.items():
+        (root / name).write_text(body, encoding="utf-8")
+    if series is None:
+        series = sorted(patches)
+    (root / "series").write_text("\n".join(series) + "\n", encoding="utf-8")
+    return root
+
+
+# A clean two-patch set: contiguous numbering, in-sync series, one file each, well-formed,
+# a uxr- switch (exercises the uxr-only pass path), no brand literals.
+def clean_patches() -> dict[str, str]:
+    return {
+        "0001-alpha.patch": make_patch("core/alpha.cc",
+                                        added=('  if (cmd.HasSwitch("uxr-alpha")) return;',)),
+        "0002-beta.patch": make_patch("core/beta.cc", added=("  int y = 1;",)),
+    }
+
+
+# --------------------------------------------------------------------------- the clean baseline
+def test_clean_set_passes_every_check(tmp_path):
+    rep = cp.run_checks(write_set(tmp_path, clean_patches()))
+    assert rep.failures == []
+
+
+# --------------------------------------------------------------------------- series-sync
+def test_series_sync_missing_entry(tmp_path):
+    # A patch file exists but is not listed -> apply-patches.sh would silently skip it.
+    write_set(tmp_path, clean_patches(), series=["0001-alpha.patch"])
+    assert cp.run_checks(tmp_path).failures == ["series-sync"]
+
+
+def test_series_sync_missing_file(tmp_path):
+    # Series lists a patch with no backing file -> the build breaks.
+    write_set(tmp_path, clean_patches(),
+              series=["0001-alpha.patch", "0002-beta.patch", "0003-ghost.patch"])
+    assert cp.run_checks(tmp_path).failures == ["series-sync"]
+
+
+def test_series_sync_duplicate_entry(tmp_path):
+    write_set(tmp_path, clean_patches(),
+              series=["0001-alpha.patch", "0001-alpha.patch", "0002-beta.patch"])
+    assert cp.run_checks(tmp_path).failures == ["series-sync"]
+
+
+def test_series_sync_wrong_order(tmp_path):
+    write_set(tmp_path, clean_patches(), series=["0002-beta.patch", "0001-alpha.patch"])
+    assert cp.run_checks(tmp_path).failures == ["series-sync"]
+
+
+# --------------------------------------------------------------------------- numbering
+def test_numbering_gap(tmp_path):
+    patches = {"0001-alpha.patch": make_patch("core/alpha.cc"),
+               "0003-gamma.patch": make_patch("core/gamma.cc")}
+    write_set(tmp_path, patches)  # series auto-syncs, so only numbering should trip
+    assert cp.run_checks(tmp_path).failures == ["numbering"]
+
+
+def test_numbering_duplicate_number(tmp_path):
+    patches = {"0001-alpha.patch": make_patch("core/alpha.cc"),
+               "0002-beta.patch": make_patch("core/beta.cc"),
+               "0002-clone.patch": make_patch("core/clone.cc")}
+    rep = cp.run_checks(write_set(tmp_path, patches))
+    assert rep.failures == ["numbering"]
+
+
+def test_numbering_non_conforming_name(tmp_path):
+    patches = {"0001-alpha.patch": make_patch("core/alpha.cc"),
+               "not-a-numbered.patch": make_patch("core/other.cc")}
+    assert cp.run_checks(write_set(tmp_path, patches)).failures == ["numbering"]
+
+
+# --------------------------------------------------------------------------- single-surface
+def test_single_surface_two_files(tmp_path):
+    patches = {"0001-alpha.patch": make_patch("core/one.cc", second_file="core/two.cc")}
+    assert cp.run_checks(write_set(tmp_path, patches)).failures == ["single-surface"]
+
+
+# --------------------------------------------------------------------------- well-formed
+def test_well_formed_missing_hunk(tmp_path):
+    patches = {"0001-alpha.patch": make_patch("core/alpha.cc", hunk=False)}
+    assert cp.run_checks(write_set(tmp_path, patches)).failures == ["well-formed"]
+
+
+def test_well_formed_missing_headers(tmp_path):
+    patches = {"0001-alpha.patch": make_patch("core/alpha.cc", headers=False)}
+    assert cp.run_checks(write_set(tmp_path, patches)).failures == ["well-formed"]
+
+
+# --------------------------------------------------------------------------- uxr-only-switches
+def test_uxr_only_rejects_non_uxr_switch(tmp_path):
+    patches = {"0001-alpha.patch": make_patch(
+        "core/alpha.cc", added=('  if (cmd.HasSwitch("legacy-mode")) return;',))}
+    assert cp.run_checks(write_set(tmp_path, patches)).failures == ["uxr-only-switches"]
+
+
+# --------------------------------------------------------------------------- no-brand-literals
+def test_no_brand_literal_in_added_code(tmp_path):
+    patches = {"0001-alpha.patch": make_patch(
+        "core/alpha.cc", added=('  const char* k = "fortress-build";',))}
+    assert cp.run_checks(write_set(tmp_path, patches)).failures == ["no-brand-literals"]
+
+
+def test_brand_token_in_comment_is_allowed(tmp_path):
+    # A `//` comment mentioning a brand does not ship as a string literal, so it must pass.
+    patches = {"0001-alpha.patch": make_patch(
+        "core/alpha.cc", added=("  int z = 0;  // fortress tweak",))}
+    assert cp.run_checks(write_set(tmp_path, patches)).failures == []