diff --git a/.coverage b/.coverage index 6c3c1e3..944624e 100644 Binary files a/.coverage and b/.coverage differ diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 53b8e97..7d52f7c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -114,6 +114,13 @@ jobs: .venv-tests/bin/python -m coverage run --branch --source=scripts.mutation_report -m unittest tests.test_mutation_report .venv-tests/bin/python -m coverage report --show-missing --fail-under=100 + # A quarantine check that passes when it should fail lets a skipped test stay skipped forever. + - name: Run quarantine check tests with branch coverage + if: ${{ !cancelled() }} + run: | + .venv-tests/bin/python -m coverage run --branch --source=scripts.quarantine_check -m unittest tests.test_quarantine_check + .venv-tests/bin/python -m coverage report --show-missing --fail-under=100 + # --------------------------------------------------------------------------- # Terminal aggregator. This is the single required check in the ruleset. # It runs even when an upstream job fails/cancels (if: always()) and then diff --git a/.github/workflows/quarantine-check.yml b/.github/workflows/quarantine-check.yml new file mode 100644 index 0000000..171050a --- /dev/null +++ b/.github/workflows/quarantine-check.yml @@ -0,0 +1,33 @@ +name: Quarantine Check + +# Fails the build on a quarantined test whose quarantine has run out, names no issue, or lasts +# more than 14 days (workspace docs/testing-standards.md, section 7). A flaky test is fixed, or +# skipped for at most 14 days with the issue that tracks its fix; never skipped silently. + +on: + workflow_call: {} + +permissions: + contents: read + +jobs: + quarantine: + name: Quarantine check + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + persist-credentials: false + + # Checkout github-workflows at this workflow file's own released version; release-please + # keeps the literal in step with the tag this file ships in. + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + repository: JorisJonkers-dev/github-workflows + ref: v0.25.0 # x-release-please-version + path: .github-workflows + persist-credentials: false + + - name: Check every quarantine is live + run: python3 .github-workflows/scripts/quarantine_check.py diff --git a/README.md b/README.md index ebc53af..5ca9c7f 100644 --- a/README.md +++ b/README.md @@ -34,6 +34,7 @@ should call released tags instead of branches. | `python-ci.yml` | Python Ruff, typecheck, and test workflow with overrideable commands. | | `nix-ci.yml` | Nix formatter, Statix, Deadnix, flake check, and optional validation. | | `jvm-ci.yml` | Gradle static lint/test workflow that fails Gradle warnings and deprecations. | +| `quarantine-check.yml` | Fails the build on a quarantined test whose quarantine has expired, names no issue, or lasts more than 14 days. The marker is `quarantine: until YYYY-MM-DD`, in the reason of whatever skips the test. | | `mutation-report.yml` | Mutates the lines a pull request changes (gremlins for Go, Stryker for TypeScript and Vue) and keeps one comment listing the mutants that survived. Never fails the build. | | `docker-image-ci.yml` | Build a Docker image without publishing. | | `container-publish.yml` | Publish a GHCR image with release and sha tags. | diff --git a/release-please-config.json b/release-please-config.json index 5bb683e..e1673a7 100644 --- a/release-please-config.json +++ b/release-please-config.json @@ -42,6 +42,10 @@ "type": "generic", "path": ".github/workflows/path-buckets.yml" }, + { + "type": "generic", + "path": ".github/workflows/quarantine-check.yml" + }, { "type": "generic", "path": ".github/workflows/jvm-ci.yml" diff --git a/scripts/quarantine_check.py b/scripts/quarantine_check.py new file mode 100644 index 0000000..169af00 --- /dev/null +++ b/scripts/quarantine_check.py @@ -0,0 +1,99 @@ +#!/usr/bin/env python3 +"""Fail the build on a quarantined test whose quarantine has run out. + +A flaky test is fixed, or quarantined for at most 14 days with the issue that +tracks its fix (workspace docs/testing-standards.md, section 7). The marker is one +string, put in the reason of whatever skips the test in its language: + + t.Skip("quarantine: https://github.com///issues/ until 2026-10-30") + test.fixme(true, 'quarantine: https://github.com/... until 2026-10-30') + @Disabled("quarantine: https://github.com/... until 2026-10-30") + +Every tracked test file is read. A marker fails when its date has passed, when it +is more than 14 days away, when it names no issue, or when it has no date. So a +quarantine cannot become permanent: once its date passes, the build fails again. + + quarantine_check.py [--root DIR] [--today YYYY-MM-DD] +""" + +from __future__ import annotations + +import argparse +import re +import subprocess +from dataclasses import dataclass +from datetime import date, timedelta +from pathlib import Path + +LONGEST = timedelta(days=14) +MARKER = re.compile(r"quarantine:(?P[^\"'`)]*)") +ISSUE = re.compile(r"https://github\.com/[\w.-]+/[\w.-]+/issues/\d+") +UNTIL = re.compile(r"\buntil\s+(\d{4}-\d{2}-\d{2})\b") +TEST_FILE = re.compile(r"(_test\.go|\.(spec|test)\.[cm]?[jt]sx?|Test\.(kt|java))$") + + +@dataclass(frozen=True) +class Finding: + path: str + line: int + problem: str + + def annotation(self) -> str: + return f"::error file={self.path},line={self.line}::quarantine {self.problem}" + + +def is_test_file(path: str) -> bool: + return bool(TEST_FILE.search(path)) + + +def check_line(line: str, today: date) -> str | None: + """Why the quarantine on this line is not allowed, or None when it is, or there is none.""" + marker = MARKER.search(line) + if not marker: + return None + rest = marker.group("rest") + if not ISSUE.search(rest): + return "names no issue: give the GitHub issue that tracks the fix" + until = UNTIL.search(rest) + try: + expires = date.fromisoformat(until.group(1)) if until else None + except ValueError: + expires = None + if expires is None: + return "has no date: end it with `until YYYY-MM-DD`" + if expires < today: + return f"expired on {expires}: fix the test or remove it" + if expires > today + LONGEST: + return f"expires {expires}, more than 14 days away" + return None + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description="Fail on a quarantined test whose quarantine has run out.") + parser.add_argument("--root", default=".", help="the repository to read") + parser.add_argument("--today", type=date.fromisoformat, default=date.today(), help="the date to judge against") + args = parser.parse_args(argv) + + tracked = subprocess.run(["git", "ls-files"], cwd=args.root, capture_output=True, text=True, check=True).stdout.splitlines() + findings: list[Finding] = [] + quarantined = 0 + for path in filter(is_test_file, tracked): + for number, line in enumerate(Path(args.root, path).read_text(errors="replace").splitlines(), start=1): + if MARKER.search(line): + quarantined += 1 + problem = check_line(line, args.today) + if problem: + findings.append(Finding(path, number, problem)) + + for finding in findings: + print(finding.annotation()) + if not quarantined: + print("No quarantined tests.") + elif not findings: + noun = "test" if quarantined == 1 else "tests" + print(f"{quarantined} quarantined {noun}, every one live.") + return 1 if findings else 0 + + +if __name__ == "__main__": # pragma: no cover + raise SystemExit(main()) diff --git a/tests/test_quarantine_check.py b/tests/test_quarantine_check.py new file mode 100644 index 0000000..f50cd5d --- /dev/null +++ b/tests/test_quarantine_check.py @@ -0,0 +1,98 @@ +from __future__ import annotations + +import io +import subprocess +import tempfile +import unittest +from contextlib import redirect_stdout +from datetime import date +from pathlib import Path + +from scripts.quarantine_check import Finding, check_line, is_test_file, main + +TODAY = date(2026, 10, 16) +ISSUE = "https://github.com/JorisJonkers-dev/identity/issues/60" + + +class ReadingAMarker(unittest.TestCase): + def test_a_marker_with_an_issue_and_a_date_within_fourteen_days_is_accepted(self) -> None: + self.assertIsNone(check_line(f't.Skip("quarantine: {ISSUE} until 2026-10-30")', TODAY)) + + def test_a_marker_expiring_today_is_still_accepted(self) -> None: + self.assertIsNone(check_line(f"// quarantine: {ISSUE} until 2026-10-16", TODAY)) + + def test_a_marker_past_its_date_fails_the_build_again(self) -> None: + self.assertEqual(check_line(f"quarantine: {ISSUE} until 2026-10-15", TODAY), "expired on 2026-10-15: fix the test or remove it") + + def test_a_marker_more_than_fourteen_days_away_is_refused(self) -> None: + self.assertEqual(check_line(f"quarantine: {ISSUE} until 2026-10-31", TODAY), "expires 2026-10-31, more than 14 days away") + + def test_a_marker_without_an_issue_link_is_refused(self) -> None: + self.assertEqual(check_line("quarantine: flaky on CI until 2026-10-20", TODAY), "names no issue: give the GitHub issue that tracks the fix") + + def test_a_marker_without_a_date_is_refused(self) -> None: + self.assertEqual(check_line(f"quarantine: {ISSUE}", TODAY), "has no date: end it with `until YYYY-MM-DD`") + + def test_a_marker_with_an_impossible_date_is_refused(self) -> None: + self.assertEqual(check_line(f"quarantine: {ISSUE} until 2026-02-30", TODAY), "has no date: end it with `until YYYY-MM-DD`") + + def test_a_line_without_a_marker_is_ignored(self) -> None: + self.assertIsNone(check_line("func TestAnAdminAddsAPersonToAGroup(t *testing.T) {", TODAY)) + + +class WhichFilesAreTests(unittest.TestCase): + def test_go_typescript_kotlin_and_java_tests_are_scanned(self) -> None: + for path in ["internal/a/b_test.go", "web/src/x.spec.ts", "web/src/y.test.ts", "web/tests/e2e/gate.spec.ts", + "api/src/test/kotlin/FooTest.kt", "lib/src/integrationTest/java/BarTest.java"]: + with self.subTest(path=path): + self.assertTrue(is_test_file(path)) + + def test_documents_and_sources_are_not_scanned(self) -> None: + for path in ["docs/testing-standards.md", "internal/a/b.go", "web/src/x.ts", "README.md"]: + with self.subTest(path=path): + self.assertFalse(is_test_file(path)) + + +def repo(files: dict[str, str]) -> tempfile.TemporaryDirectory: + directory = tempfile.TemporaryDirectory() + root = Path(directory.name) + for path, text in files.items(): + (root / path).parent.mkdir(parents=True, exist_ok=True) + (root / path).write_text(text) + subprocess.run(["git", "init", "-q"], cwd=root, check=True) + subprocess.run(["git", "add", "-A"], cwd=root, check=True) + return directory + + +class CommandLine(unittest.TestCase): + def test_a_repository_with_live_quarantines_passes(self) -> None: + with repo({"a_test.go": f't.Skip("quarantine: {ISSUE} until 2026-10-20")\n'}) as root, redirect_stdout(io.StringIO()) as out: + code = main(["--root", root, "--today", "2026-10-16"]) + self.assertEqual(code, 0) + self.assertIn("1 quarantined test, every one live", out.getvalue()) + + def test_an_expired_quarantine_fails_and_names_where_it_is(self) -> None: + with repo({"web/x.spec.ts": "it('a', () => {})\n" + f"it.skip('b quarantine: {ISSUE} until 2026-10-01', () => {{}})\n"}) as root, redirect_stdout(io.StringIO()) as out: + code = main(["--root", root, "--today", "2026-10-16"]) + self.assertEqual(code, 1) + self.assertIn("::error file=web/x.spec.ts,line=2::quarantine expired on 2026-10-01", out.getvalue()) + + def test_a_marker_outside_test_files_is_not_checked(self) -> None: + with repo({"docs/standards.md": "quarantine: flaky until never\n"}) as root, redirect_stdout(io.StringIO()): + self.assertEqual(main(["--root", root, "--today", "2026-10-16"]), 0) + + def test_a_repository_without_quarantines_passes(self) -> None: + with repo({"a_test.go": "package a\n"}) as root, redirect_stdout(io.StringIO()) as out: + self.assertEqual(main(["--root", root, "--today", "2026-10-16"]), 0) + self.assertIn("No quarantined tests.", out.getvalue()) + + def test_today_defaults_to_the_current_date(self) -> None: + with repo({"a_test.go": "package a\n"}) as root, redirect_stdout(io.StringIO()): + self.assertEqual(main(["--root", root]), 0) + + def test_a_finding_reads_as_a_github_annotation(self) -> None: + self.assertEqual(Finding("a_test.go", 3, "names no issue").annotation(), "::error file=a_test.go,line=3::quarantine names no issue") + + +if __name__ == "__main__": + unittest.main()