diff --git a/.github/scripts/check_coverage_map_health.py b/.github/scripts/check_coverage_map_health.py index 978a2b8c84..739ddc6bae 100644 --- a/.github/scripts/check_coverage_map_health.py +++ b/.github/scripts/check_coverage_map_health.py @@ -1,5 +1,6 @@ """Fail loudly if the committed coverage map is stale or under-covers. Used by coverage-health.yml.""" import datetime +import subprocess import sys from pathlib import Path @@ -10,6 +11,26 @@ MAX_AGE_DAYS = 10 MIN_FRACTION = 0.80 +# Must mirror the `paths:` trigger of coverage-refresh.yml. A superset would raise false +# alarms (a change that never triggers a refresh would look like a missed refresh). +COVERAGE_RELEVANT_PATHS = [":(glob)src/**/*.fpp", "toolchain/mfc/test/cases.py"] + + +def built_after_last_change(git_sha): + """Was the map built at or after the last coverage-relevant commit? None if unknowable. + + This is the direct question the wall-clock age check only approximated: it stays quiet + over a genuinely quiet repo (no refresh needed, so no heartbeat commit needed either) + and fires on the next relevant push after a refresh breaks, rather than 10 days later. + """ + if not git_sha: + return None + last = subprocess.run(["git", "log", "-1", "--format=%H", "--", *COVERAGE_RELEVANT_PATHS], capture_output=True, text=True, check=False) + if last.returncode != 0 or not last.stdout.strip(): + return None # shallow clone or no such commit -> fall back to the age rule + ancestor = subprocess.run(["git", "merge-base", "--is-ancestor", last.stdout.strip(), git_sha], capture_output=True, check=False) + return {0: True, 1: False}.get(ancestor.returncode) # anything else -> None (unknown sha, shallow history) + entries, meta = load_map(COVERAGE_MAP_PATH) if entries is None: sys.exit("Coverage map missing or corrupt.") @@ -37,6 +58,7 @@ now=datetime.datetime.now(datetime.timezone.utc).isoformat(), max_age_days=MAX_AGE_DAYS, min_fraction=MIN_FRACTION, + built_after_last_change=built_after_last_change(meta.get("git_sha")), ) print(msg) sys.exit(0 if ok else 1) diff --git a/.github/scripts/coverage_map_changed.py b/.github/scripts/coverage_map_changed.py new file mode 100644 index 0000000000..8f54d7564f --- /dev/null +++ b/.github/scripts/coverage_map_changed.py @@ -0,0 +1,52 @@ +"""Decide whether a rebuilt coverage map is worth committing. Used by coverage-refresh.yml. + +The refresh job rebuilds the map on every master push touching src/**/*.fpp or cases.py, +but the rebuild is usually identical in substance -- only _meta (built_at, git_sha) moves. +A file-level `git diff` therefore always reports a change, so the bot pushed a no-op commit +on nearly every run. Compare the coverage entries instead. + +Exit codes: 0 = entries changed, commit. UNCHANGED (10) = skip. Anything else = error. + +`unchanged` deliberately is NOT 1: an uncaught exception exits 1, and the caller must not +be able to mistake a crashed comparison for "nothing to push" -- that would turn a broken +refresh into a silently green job. Every code outside {0, 10} fails the workflow. +""" + +import subprocess +import sys +import tempfile +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2] / "toolchain")) +from mfc.test.coverage import COVERAGE_MAP_PATH, entries_equal, load_map # noqa: E402 + +UNCHANGED = 10 + +new_entries, _ = load_map(COVERAGE_MAP_PATH) +if new_entries is None: + print(f"Rebuilt {COVERAGE_MAP_PATH} is missing or corrupt.", file=sys.stderr) + sys.exit(2) + +committed = subprocess.run(["git", "show", f"HEAD:{COVERAGE_MAP_PATH}"], capture_output=True, check=False) +if committed.returncode != 0: + print("No coverage map committed at HEAD -> commit the rebuilt one.") + sys.exit(0) + +with tempfile.NamedTemporaryFile(suffix=".json.gz") as tmp: + tmp.write(committed.stdout) + tmp.flush() + old_entries, _ = load_map(Path(tmp.name)) + +if old_entries is None: + print("Committed coverage map is corrupt -> commit the rebuilt one.") + sys.exit(0) + +if entries_equal(old_entries, new_entries): + print(f"Coverage entries unchanged ({len(new_entries)} tests) -> nothing to push.") + sys.exit(UNCHANGED) + +added = len(set(new_entries) - set(old_entries)) +removed = len(set(old_entries) - set(new_entries)) +recovered = sum(1 for k in set(old_entries) & set(new_entries) if sorted(old_entries[k]) != sorted(new_entries[k])) +print(f"Coverage entries changed: +{added} -{removed}, {recovered} with different coverage -> committing.") +sys.exit(0) diff --git a/.github/workflows/coverage-health.yml b/.github/workflows/coverage-health.yml index 301d87dd10..cd66d57fd3 100644 --- a/.github/workflows/coverage-health.yml +++ b/.github/workflows/coverage-health.yml @@ -9,7 +9,10 @@ jobs: if: github.repository == 'MFlowCode/MFC' runs-on: ubuntu-latest steps: + # Full history: the freshness check asks whether the map's git_sha contains the last + # commit touching src/**/*.fpp or cases.py, which a shallow clone cannot answer. - uses: actions/checkout@v5 + with: { fetch-depth: 0 } - uses: actions/setup-python@v6 with: { python-version: '3.12' } - name: Initialize MFC diff --git a/.github/workflows/coverage-refresh.yml b/.github/workflows/coverage-refresh.yml index 72241c2f3b..1f9b3747fd 100644 --- a/.github/workflows/coverage-refresh.yml +++ b/.github/workflows/coverage-refresh.yml @@ -35,7 +35,25 @@ jobs: env: CACHE_PUSH_TOKEN: ${{ secrets.CACHE_PUSH_TOKEN }} run: | - if ! git diff --quiet tests/coverage_map.json.gz; then + # Compare coverage ENTRIES, not file bytes: _meta (built_at, git_sha) moves on + # every rebuild, so `git diff` on the .gz always reported a change and the bot + # pushed a no-op commit nearly every run. rc 0 = changed, 10 = unchanged. + # The comparison script is stdlib-only, so the system interpreter is a fine + # fallback if the SLURM job never got far enough to create build/venv. + PY=build/venv/bin/python3 + [ -x "$PY" ] || PY=python3 + set +e + "$PY" .github/scripts/coverage_map_changed.py + changed=$? + set -e + # Every other code is a hard error, never a silent skip: an uncaught Python + # exception exits 1 and a missing interpreter exits 127, and treating either as + # "unchanged" would let a permanently broken comparison run permanently green. + case "$changed" in + 0|10) ;; + *) echo "::error::Coverage map comparison failed (rc=$changed)."; exit 1 ;; + esac + if [ "$changed" -eq 0 ]; then git config user.name "mfc-bot" git config user.email "mfc-bot@users.noreply.github.com" git add tests/coverage_map.json.gz @@ -50,5 +68,6 @@ jobs: # ensures this token is actually used for the push. git push "https://x-access-token:${CACHE_PUSH_TOKEN}@github.com/MFlowCode/MFC.git" HEAD:master else - echo "Coverage map unchanged." + # Discard the rebuilt file so the runner's working tree matches master. + git checkout -- tests/coverage_map.json.gz fi diff --git a/toolchain/mfc/test/case.py b/toolchain/mfc/test/case.py index e12705316c..837749791f 100644 --- a/toolchain/mfc/test/case.py +++ b/toolchain/mfc/test/case.py @@ -251,9 +251,9 @@ def get_uuid(self) -> str: return trace_to_uuid(self.trace) def coverage_key(self) -> str: - from .coverage import param_hash + from .coverage import canonicalize_param_paths, param_hash - return param_hash(self.params) + return param_hash(canonicalize_param_paths(self.params, common.MFC_ROOT_DIR)) def get_dirpath(self): return os.path.join(common.MFC_TEST_DIR, self.get_uuid()) diff --git a/toolchain/mfc/test/coverage.py b/toolchain/mfc/test/coverage.py index 496064e695..1fca077311 100644 --- a/toolchain/mfc/test/coverage.py +++ b/toolchain/mfc/test/coverage.py @@ -8,11 +8,51 @@ import gzip import hashlib import json +import os import subprocess from pathlib import Path from typing import Optional, Tuple +def canonicalize_param_paths(params: dict, root: str) -> dict: + """Rewrite absolute in-repo paths to repo-relative ones, for hashing only. + + TestCaseBuilder.to_case() absolutizes file-valued params (e.g. the STL cases' + stl_models(1)%model_filepath) so a test runs from any cwd. Those absolutes embed the + checkout location, which differs between the map-building runner and PR runners. + Hashing them gave the same test a different key on every machine, which churned ~8 + entries out of the map on every refresh and left those tests permanently unmapped + (rung 5 -> always run). Hash the repo-relative form so the key names the test, not + the machine. Paths outside the repo are left alone -- they are genuinely part of the + test's identity. + """ + root = os.path.abspath(root) + out = {} + for key, value in params.items(): + canonical = value + if isinstance(value, str) and os.path.isabs(value): + try: + rel = os.path.relpath(value, root) + except ValueError: # different drive on Windows -> not in-repo + rel = os.pardir + # Only a leading `..` COMPONENT means "outside the repo". Testing the string + # prefix alone would also reject an in-repo name that merely starts with dots + # (`..foo`), leaving it absolute -- the churn this function exists to remove. + if rel != os.pardir and not rel.startswith(os.pardir + os.sep): + canonical = rel.replace(os.sep, "/") + out[key] = canonical + return out + + +def entries_equal(a: Optional[dict], b: Optional[dict]) -> bool: + """True if two maps describe the same coverage, ignoring per-test list order.""" + if a is None or b is None: + return False + if set(a) != set(b): + return False + return all(sorted(a[k]) == sorted(b[k]) for k in a) + + def param_hash(params: dict) -> str: """Stable 16-hex key identifying a test by its defining params. @@ -35,8 +75,14 @@ def save_map(path: Path, entries: dict, *, n_tests: int, git_sha: str, gfortran_ "n_tests": n_tests, } path.parent.mkdir(parents=True, exist_ok=True) - with gzip.open(path, "wt", encoding="utf-8") as f: - json.dump(payload, f, indent=2, sort_keys=True) + body = json.dumps(payload, indent=2, sort_keys=True).encode("utf-8") + # mtime=0 and an empty filename keep the gzip header byte-identical across builds. + # gzip.open() stamps the current time and the source filename into the header, so two + # runs producing the same map still wrote different bytes -- which defeated the + # "only commit if the file changed" guard in coverage-refresh.yml. + with open(path, "wb") as raw: + with gzip.GzipFile(filename="", mode="wb", fileobj=raw, mtime=0) as f: + f.write(body) # Test-definition file: changing it adds/modifies tests, but only the tests it touches @@ -223,12 +269,22 @@ def format_summary(*, ran, total, reason, meta, now) -> str: return f"Coverage selection: ran {ran}/{total} tests · {age} · {reason}" -def map_health(*, meta, current_keys, mapped_keys, now, max_age_days, min_fraction): - """Return (ok, message). Loud anti-rot check used by the health workflow.""" +def map_health(*, meta, current_keys, mapped_keys, now, max_age_days, min_fraction, built_after_last_change=None): + """Return (ok, message). Loud anti-rot check used by the health workflow. + + `built_after_last_change` is the caller's git verdict on whether the map was built at + or after the most recent coverage-relevant commit: True (provably current), False + (provably behind), or None (undeterminable -> fall back to the wall-clock rule). + A map only decays when the sources or test list it was built from move, so wall-clock + age is a poor proxy: it cries STALE over a quiet weekend and stays silent for 10 days + after a refresh actually breaks. + """ if not meta or not meta.get("built_at"): return False, "Coverage map has no build metadata." age = (datetime.datetime.fromisoformat(now) - datetime.datetime.fromisoformat(meta["built_at"])).days - if age > max_age_days: + if built_after_last_change is False: + return False, "Coverage map is STALE: built before the most recent coverage-relevant commit. Refresh workflow may be broken." + if built_after_last_change is None and age > max_age_days: return False, f"Coverage map is STALE: {age}d old (max {max_age_days}d). Refresh workflow may be broken." if current_keys: frac = len(current_keys & mapped_keys) / len(current_keys) diff --git a/toolchain/mfc/test/test_coverage_unit.py b/toolchain/mfc/test/test_coverage_unit.py index b7b233a0d9..4b83fbb9ed 100644 --- a/toolchain/mfc/test/test_coverage_unit.py +++ b/toolchain/mfc/test/test_coverage_unit.py @@ -1,9 +1,11 @@ +import subprocess +import sys import tempfile import types as _types from pathlib import Path from unittest.mock import patch -from mfc.test.coverage import format_summary, get_changed_files, is_always_run_all, load_map, map_health, param_hash, save_map, select_tests +from mfc.test.coverage import canonicalize_param_paths, entries_equal, format_summary, get_changed_files, is_always_run_all, load_map, map_health, param_hash, save_map, select_tests def test_param_hash_is_order_independent(): @@ -359,3 +361,148 @@ def test_empty_map_with_fpp_change_runs_all_rung4(): cases = _cases("a", "b") run, skip, reason = select_tests(cases, {}, {"src/simulation/m_rhs.fpp"}) assert len(run) == 2 and reason.startswith("rung4") + + +# --- Map churn: a rebuilt map must be byte-comparable when nothing really changed --- + + +def test_coverage_key_is_independent_of_checkout_location(): + """The same test built from two checkouts must hash identically. + + to_case() absolutizes file-valued params, so the STL cases carry the runner's + checkout path. Hashing that gave every runner a different key for the same test. + """ + runner_a = "/home/runner/work/MFC/MFC" + runner_b = "/storage/scratch/job1234/MFC" + a = param_hash(canonicalize_param_paths({"m": 100, "stl_models(1)%model_filepath": f"{runner_a}/examples/2D_ibm_stl/model.stl"}, runner_a)) + b = param_hash(canonicalize_param_paths({"m": 100, "stl_models(1)%model_filepath": f"{runner_b}/examples/2D_ibm_stl/model.stl"}, runner_b)) + assert a == b + + +def test_canonicalize_preserves_paths_outside_the_repo(): + params = {"f": "/opt/shared/meshes/mesh.stl"} + assert canonicalize_param_paths(params, "/home/runner/MFC") == params + + +def test_canonicalize_leaves_non_path_values_untouched(): + params = {"m": 100, "weno_order": 5, "bubbles_euler": "T"} + assert canonicalize_param_paths(params, "/home/runner/MFC") == params + + +def test_canonicalize_relativizes_in_repo_name_that_starts_with_dots(): + """`..foo` is a leading-dots filename, not a parent-directory escape.""" + root = "/home/runner/MFC" + assert canonicalize_param_paths({"f": f"{root}/..cache/model.stl"}, root) == {"f": "..cache/model.stl"} + + +def test_canonicalize_preserves_sibling_directory_sharing_a_prefix(): + """The near miss: relpath yields a genuine leading `..`, so the path stays absolute.""" + params = {"f": "/home/runner/MFC-scratch/model.stl"} + assert canonicalize_param_paths(params, "/home/runner/MFC") == params + + +def test_canonicalized_key_still_distinguishes_different_files(): + root = "/home/runner/MFC" + a = param_hash(canonicalize_param_paths({"f": f"{root}/examples/a/model.stl"}, root)) + b = param_hash(canonicalize_param_paths({"f": f"{root}/examples/b/model.stl"}, root)) + assert a != b + + +def test_save_map_writes_a_deterministic_gzip_header(): + """gzip stamps mtime + source filename by default, so identical maps differed on disk.""" + with tempfile.TemporaryDirectory() as d: + p = Path(d) / "m.json.gz" + save_map(p, {"abc": ["src/simulation/m_rhs.fpp"]}, n_tests=1, git_sha="deadbee", gfortran_version="13") + raw = p.read_bytes() + assert raw[4:8] == b"\x00\x00\x00\x00", "gzip MTIME field must be zeroed" + assert not raw[3] & 0x08, "gzip FNAME flag must be clear" + + +def test_entries_equal_ignores_coverage_list_order(): + assert entries_equal({"a": ["y.fpp", "x.fpp"]}, {"a": ["x.fpp", "y.fpp"]}) + + +def test_entries_equal_detects_real_differences(): + assert not entries_equal({"a": ["x.fpp"]}, {"a": ["x.fpp", "y.fpp"]}) + assert not entries_equal({"a": ["x.fpp"]}, {"b": ["x.fpp"]}) + assert not entries_equal(None, {"a": ["x.fpp"]}) + + +def test_health_quiet_repo_is_not_stale_when_map_is_current(): + """An old map is fine if nothing coverage-relevant landed since it was built.""" + ok, msg = map_health( + meta={"built_at": "2026-05-01T00:00:00+00:00", "n_tests": 600}, + current_keys={"a"}, + mapped_keys={"a"}, + now="2026-05-29T00:00:00+00:00", + max_age_days=10, + min_fraction=0.8, + built_after_last_change=True, + ) + assert ok, msg + + +def test_health_fails_immediately_when_map_predates_last_source_change(): + """Detects a dead refresh on the next relevant push, not 10 days later.""" + ok, msg = map_health( + meta={"built_at": "2026-05-28T00:00:00+00:00", "n_tests": 600}, + current_keys={"a"}, + mapped_keys={"a"}, + now="2026-05-29T00:00:00+00:00", + max_age_days=10, + min_fraction=0.8, + built_after_last_change=False, + ) + assert not ok and "stale" in msg.lower() + + +# --- coverage_map_changed.py: the commit guard the refresh workflow branches on --- + +CHANGED_SCRIPT = Path(__file__).resolve().parents[3] / ".github" / "scripts" / "coverage_map_changed.py" + + +def _repo_with_committed_map(d, entries): + """A throwaway git repo whose HEAD holds `entries` as the coverage map.""" + repo = Path(d) + git = ["git", "-c", "user.name=t", "-c", "user.email=t@t", "-C", str(repo)] + subprocess.run([*git, "init", "-q"], check=True) + save_map(repo / "tests" / "coverage_map.json.gz", entries, n_tests=len(entries), git_sha="aaa", gfortran_version="13") + subprocess.run([*git, "add", "-A"], check=True) + subprocess.run([*git, "commit", "-q", "--no-verify", "-m", "map"], check=True) + return repo + + +def _run_guard(repo): + return subprocess.run([sys.executable, str(CHANGED_SCRIPT)], cwd=repo, capture_output=True, text=True, check=False).returncode + + +def test_guard_reports_unchanged_with_a_code_that_is_not_the_crash_code(): + """A rebuild differing only in _meta must skip -- but never via exit 1. + + An uncaught exception exits 1, so if "unchanged" were 1 the workflow could not tell a + no-op refresh from a broken comparison, and a dead guard would run permanently green. + """ + with tempfile.TemporaryDirectory() as d: + repo = _repo_with_committed_map(d, {"k1": ["src/simulation/m_rhs.fpp"]}) + # Same entries, fresh _meta -- exactly what every no-op refresh produces. + save_map(repo / "tests" / "coverage_map.json.gz", {"k1": ["src/simulation/m_rhs.fpp"]}, n_tests=1, git_sha="bbb", gfortran_version="13") + rc = _run_guard(repo) + assert rc == 10 + assert rc not in (0, 1) + + +def test_guard_reports_changed_when_coverage_actually_moves(): + with tempfile.TemporaryDirectory() as d: + repo = _repo_with_committed_map(d, {"k1": ["src/simulation/m_rhs.fpp"]}) + save_map(repo / "tests" / "coverage_map.json.gz", {"k1": ["src/simulation/m_rhs.fpp"], "k2": ["src/common/m_eos.fpp"]}, n_tests=2, git_sha="bbb", gfortran_version="13") + assert _run_guard(repo) == 0 + + +def test_guard_errors_when_the_rebuilt_map_is_missing(): + """The SLURM job died before writing a map: fail the job, do not read it as unchanged.""" + with tempfile.TemporaryDirectory() as d: + repo = _repo_with_committed_map(d, {"k1": ["src/simulation/m_rhs.fpp"]}) + (repo / "tests" / "coverage_map.json.gz").unlink() + rc = _run_guard(repo) + assert rc == 2 + assert rc != 10