From 7108a0d43b9c0a50a74056b04b2fc40d42f618d1 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Tue, 18 Aug 2026 10:12:30 -0400 Subject: [PATCH 1/2] ci(coverage): key map freshness on the refresh run, not on _meta.git_sha The daily Coverage Map Health cron has failed since 2026-08-16 with Coverage map is STALE: built before the most recent coverage-relevant commit. Refresh workflow may be broken. while the refresh workflow was in fact working. #1683 added two things in one commit that contradict each other: coverage_map_changed.py, which skips the commit when the rebuilt coverage entries are identical, and the git-ancestry freshness rule, which requires the map's _meta.git_sha to descend from the last commit touching src/**/*.fpp or cases.py. git_sha only advances when a commit lands, so the first coverage-relevant commit whose coverage happened to be unchanged pinned it in the past forever. Four such commits landed after #1414 (the map's git_sha), each with a refresh run that printed "Coverage entries unchanged (675 tests) -> nothing to push". Nothing self-heals this; only a future commit that actually moves the entries would. Record the freshness signal where it belongs: coverage-refresh.yml now moves refs/coverage-map/verified to the commit it rebuilt from on every successful refresh, both branches of the commit guard, and the health check asks whether THAT ref descends from the last relevant commit. The ref sits outside refs/heads/ and refs/tags/ so updating it fires no push trigger; a branch or tag would start homebrew.yml, which filters on paths alone. Also raise the refresh timeout from 240 to 480 minutes. Wall clock there is dominated by the phoenix SLURM queue, not the build -- observed waits reach 6h, and the 240-minute cap killed the refresh of #1717 at 4h05m, which is why the map is also behind the current test list. --- .github/scripts/check_coverage_map_health.py | 110 ++++++++++++------- .github/workflows/coverage-health.yml | 9 ++ .github/workflows/coverage-refresh.yml | 25 ++++- toolchain/mfc/test/coverage.py | 17 ++- toolchain/mfc/test/test_coverage_unit.py | 100 ++++++++++++++++- 5 files changed, 214 insertions(+), 47 deletions(-) diff --git a/.github/scripts/check_coverage_map_health.py b/.github/scripts/check_coverage_map_health.py index 739ddc6bae..33e0747399 100644 --- a/.github/scripts/check_coverage_map_health.py +++ b/.github/scripts/check_coverage_map_health.py @@ -11,54 +11,90 @@ MAX_AGE_DAYS = 10 MIN_FRACTION = 0.80 +# coverage-refresh.yml moves this ref to the commit it just rebuilt the map from, on EVERY +# successful refresh -- including the ones whose entries come out identical and therefore +# push no commit at all. +# +# The map's own _meta.git_sha cannot answer the freshness question. It only advances when a +# commit lands, and the refresh deliberately skips the commit when the coverage entries are +# unchanged (a no-op commit on every push is what #1683 removed). So a coverage-relevant +# commit whose coverage happens to be identical pinned git_sha in the past and made this +# check fail every day thereafter, with no way to recover: a refresh that WAS working +# looked broken. The question here is "has a refresh run since the last relevant commit", +# and only the refresh itself can answer it. +# +# It lives outside refs/heads/ and refs/tags/ so that updating it fires no `push` workflow +# trigger. actions/checkout does not fetch it; coverage-health.yml fetches it explicitly. +VERIFIED_REF = "refs/coverage-map/verified" + # 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. +def verified_sha(cwd=None): + """Commit the last successful refresh verified the map against, or None if unknown. + + None covers both "the ref was never pushed" (a fork, or the window before the first + refresh after VERIFIED_REF was introduced) and "the fetch did not bring it down". The + caller must read that as undeterminable and fall back to the wall-clock age rule, not + as a failure -- an absent ref is not evidence of a broken refresh. + """ + rev = subprocess.run(["git", "rev-parse", "--verify", "--quiet", f"{VERIFIED_REF}^{{commit}}"], capture_output=True, text=True, check=False, cwd=cwd) + return rev.stdout.strip() or None + + +def verified_after_last_change(git_sha, cwd=None): + """Did that refresh run 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. + over a genuinely quiet repo (no refresh needed, so no heartbeat 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) + last = subprocess.run(["git", "log", "-1", "--format=%H", "--", *COVERAGE_RELEVANT_PATHS], capture_output=True, text=True, check=False, cwd=cwd) 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) + ancestor = subprocess.run(["git", "merge-base", "--is-ancestor", last.stdout.strip(), git_sha], capture_output=True, check=False, cwd=cwd) 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.") -# Compute each current test's coverage key. Loading a case executes its case -# file; some (e.g. chemistry examples) import optional deps like cantera that are -# not installed in this lightweight job. Skip any case that fails to load instead -# of crashing — map_health measures the fraction of *loadable* current tests that -# are mapped, so a smaller current_keys cannot produce a false "stale" result. -current_keys = set() -unloadable = [] -for b in list_cases(): - try: - current_keys.add(b.to_case().coverage_key()) - except Exception as exc: # noqa: BLE001 — a case file that won't import must not crash the health check - last_line = (str(exc).strip().splitlines() or ["(no message)"])[-1][:140] - unloadable.append((getattr(b, "trace", repr(b)), last_line)) -if unloadable: - print(f"Note: {len(unloadable)} case(s) could not be loaded in this lightweight job (excluded from the check):") - for trace, err in unloadable[:15]: - print(f" - {trace}: {err}") -ok, msg = map_health( - meta=meta, - current_keys=current_keys, - mapped_keys=set(entries), - 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) + +def main(): + entries, meta = load_map(COVERAGE_MAP_PATH) + if entries is None: + sys.exit("Coverage map missing or corrupt.") + # Compute each current test's coverage key. Loading a case executes its case + # file; some (e.g. chemistry examples) import optional deps like cantera that are + # not installed in this lightweight job. Skip any case that fails to load instead + # of crashing -- map_health measures the fraction of *loadable* current tests that + # are mapped, so a smaller current_keys cannot produce a false "stale" result. + current_keys = set() + unloadable = [] + for b in list_cases(): + try: + current_keys.add(b.to_case().coverage_key()) + except Exception as exc: # noqa: BLE001 -- a case file that won't import must not crash the health check + last_line = (str(exc).strip().splitlines() or ["(no message)"])[-1][:140] + unloadable.append((getattr(b, "trace", repr(b)), last_line)) + if unloadable: + print(f"Note: {len(unloadable)} case(s) could not be loaded in this lightweight job (excluded from the check):") + for trace, err in unloadable[:15]: + print(f" - {trace}: {err}") + ok, msg = map_health( + meta=meta, + current_keys=current_keys, + mapped_keys=set(entries), + now=datetime.datetime.now(datetime.timezone.utc).isoformat(), + max_age_days=MAX_AGE_DAYS, + min_fraction=MIN_FRACTION, + verified_after_last_change=verified_after_last_change(verified_sha()), + ) + print(msg) + return 0 if ok else 1 + + +# Guarded so the two git predicates above can be imported and unit-tested against a +# throwaway repository; running the module still performs the full check. +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/coverage-health.yml b/.github/workflows/coverage-health.yml index cd66d57fd3..5835269fbd 100644 --- a/.github/workflows/coverage-health.yml +++ b/.github/workflows/coverage-health.yml @@ -13,6 +13,15 @@ jobs: # commit touching src/**/*.fpp or cases.py, which a shallow clone cannot answer. - uses: actions/checkout@v5 with: { fetch-depth: 0 } + - name: Fetch the coverage-map verification ref + # actions/checkout fetches only refs/heads and refs/tags, and refs/coverage-map/ + # verified deliberately lives outside both so that updating it fires no `push` + # workflow trigger. Absent on a fork and in the window before the first refresh + # after it was introduced, so this must not fail the job: the check treats a + # missing ref as undeterminable and falls back to the wall-clock age rule. + run: | + git fetch --no-tags origin '+refs/coverage-map/verified:refs/coverage-map/verified' \ + || echo "::notice::No coverage-map verification ref yet; using the wall-clock age rule." - 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 1f9b3747fd..e9d1a04aaf 100644 --- a/.github/workflows/coverage-refresh.yml +++ b/.github/workflows/coverage-refresh.yml @@ -17,7 +17,11 @@ concurrency: jobs: refresh: if: github.repository == 'MFlowCode/MFC' - timeout-minutes: 240 + # Wall clock here is dominated by the SLURM queue, not by the build: observed waits on + # the phoenix `embers` QOS have reached 6h, and a 240-minute cap silently killed the + # refresh of #1717 at 4h05m. Cap generously -- a refresh that never finishes leaves the + # map behind, which is exactly what coverage-health.yml then reports. + timeout-minutes: 480 runs-on: group: phoenix labels: gt @@ -71,3 +75,22 @@ jobs: # Discard the rebuilt file so the runner's working tree matches master. git checkout -- tests/coverage_map.json.gz fi + # Record that a refresh verified the map against THIS commit, in BOTH branches + # above. coverage-health.yml cannot use the map's _meta.git_sha for this: git_sha + # advances only when a commit lands, and the guard above deliberately skips the + # commit when the entries are unchanged, so a coverage-relevant commit whose + # coverage is identical left the map looking permanently stale to the health + # check while this workflow was working fine. + # + # refs/coverage-map/ is outside refs/heads/ and refs/tags/ on purpose: a branch or + # tag would fire the `push` trigger of any workflow that filters on paths alone + # (homebrew.yml does), starting unrelated jobs on every refresh. + # + # Non-fatal: the map itself is already committed and pushed by this point, so a + # failed bookkeeping push must not red a refresh that succeeded. It is not silent + # either -- the warning lands in the run summary, and a ref that stops advancing + # drops coverage-health.yml back to its wall-clock rule, which goes red within + # MAX_AGE_DAYS. + git push "https://x-access-token:${CACHE_PUSH_TOKEN}@github.com/MFlowCode/MFC.git" \ + --force HEAD:refs/coverage-map/verified \ + || echo "::warning::Could not update refs/coverage-map/verified; coverage-health.yml falls back to the age rule." diff --git a/toolchain/mfc/test/coverage.py b/toolchain/mfc/test/coverage.py index 1fca077311..26d664affb 100644 --- a/toolchain/mfc/test/coverage.py +++ b/toolchain/mfc/test/coverage.py @@ -269,22 +269,27 @@ 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, built_after_last_change=None): +def map_health(*, meta, current_keys, mapped_keys, now, max_age_days, min_fraction, verified_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 + `verified_after_last_change` is the caller's git verdict on whether a refresh ran 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. + + It asks about the refresh RUN, not about the map's own _meta.git_sha, which advances + only when the rebuilt entries differ from the committed ones. A relevant commit whose + coverage is unchanged leaves a correct map with an old git_sha; reading that as + "stale" made this check fail permanently while the refresh was working fine. """ 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 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: + if verified_after_last_change is False: + return False, "Coverage map is STALE: no successful refresh since the most recent coverage-relevant commit. Refresh workflow may be broken." + if verified_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 c4a0f558ab..24f52ec1aa 100644 --- a/toolchain/mfc/test/test_coverage_unit.py +++ b/toolchain/mfc/test/test_coverage_unit.py @@ -438,12 +438,12 @@ def test_health_quiet_repo_is_not_stale_when_map_is_current(): now="2026-05-29T00:00:00+00:00", max_age_days=10, min_fraction=0.8, - built_after_last_change=True, + verified_after_last_change=True, ) assert ok, msg -def test_health_fails_immediately_when_map_predates_last_source_change(): +def test_health_fails_immediately_when_no_refresh_ran_since_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}, @@ -452,7 +452,7 @@ def test_health_fails_immediately_when_map_predates_last_source_change(): now="2026-05-29T00:00:00+00:00", max_age_days=10, min_fraction=0.8, - built_after_last_change=False, + verified_after_last_change=False, ) assert not ok and "stale" in msg.lower() @@ -535,3 +535,97 @@ def test_guard_errors_when_the_rebuilt_map_is_missing(): rc = _run_guard(repo) assert rc == 2 assert rc != 10 + + +# --- check_coverage_map_health.py: the freshness signal the health workflow branches on --- + +HEALTH_SCRIPT = Path(__file__).resolve().parents[3] / ".github" / "scripts" / "check_coverage_map_health.py" + + +def _health_module(): + """Import the health script by path; it is a script, not an installed module.""" + import importlib.util + + spec = importlib.util.spec_from_file_location("check_coverage_map_health", HEALTH_SCRIPT) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +def _repo_with_history(d): + """A throwaway repo: one commit touching src/**/*.fpp, then one that does not.""" + repo = Path(d) + env = _env_without_git() + git = ["git", "-c", "user.name=t", "-c", "user.email=t@t", "-C", str(repo)] + subprocess.run([*git, "init", "-q", "-b", "master"], check=True, env=env) + (repo / "src" / "simulation").mkdir(parents=True) + (repo / "src" / "simulation" / "m_rhs.fpp").write_text("! v1\n") + subprocess.run([*git, "add", "-A"], check=True, env=env) + subprocess.run([*git, "commit", "-q", "--no-verify", "-m", "relevant"], check=True, env=env) + relevant = subprocess.run([*git, "rev-parse", "HEAD"], capture_output=True, text=True, check=True, env=env).stdout.strip() + (repo / "README.md").write_text("docs\n") + subprocess.run([*git, "add", "-A"], check=True, env=env) + subprocess.run([*git, "commit", "-q", "--no-verify", "-m", "irrelevant"], check=True, env=env) + later = subprocess.run([*git, "rev-parse", "HEAD"], capture_output=True, text=True, check=True, env=env).stdout.strip() + return repo, relevant, later + + +def _set_verified(repo, sha): + subprocess.run(["git", "-C", str(repo), "update-ref", "refs/coverage-map/verified", sha], check=True, env=_env_without_git()) + + +def test_verified_sha_is_none_when_the_ref_was_never_pushed(): + """A fork, or the window before the first refresh: undeterminable, not broken.""" + health = _health_module() + with tempfile.TemporaryDirectory() as d: + repo, _, _ = _repo_with_history(d) + assert health.verified_sha(cwd=repo) is None + # None must reach map_health as None, which falls back to the wall-clock rule. + assert health.verified_after_last_change(health.verified_sha(cwd=repo), cwd=repo) is None + + +def test_verified_after_last_change_true_when_a_refresh_ran_since_the_change(): + health = _health_module() + with tempfile.TemporaryDirectory() as d: + repo, relevant, later = _repo_with_history(d) + _set_verified(repo, later) + assert health.verified_sha(cwd=repo) == later + assert health.verified_after_last_change(later, cwd=repo) is True + # The relevant commit itself counts: a refresh AT the change is current. + assert health.verified_after_last_change(relevant, cwd=repo) is True + + +def test_verified_after_last_change_false_when_the_refresh_predates_the_change(): + """The genuine broken-refresh case this check exists to catch.""" + health = _health_module() + with tempfile.TemporaryDirectory() as d: + repo, relevant, _ = _repo_with_history(d) + env = _env_without_git() + git = ["git", "-c", "user.name=t", "-c", "user.email=t@t", "-C", str(repo)] + before = subprocess.run([*git, "rev-parse", "HEAD~1"], capture_output=True, text=True, check=True, env=env).stdout.strip() + assert before == relevant + (repo / "src" / "simulation" / "m_rhs.fpp").write_text("! v2\n") + subprocess.run([*git, "add", "-A"], check=True, env=env) + subprocess.run([*git, "commit", "-q", "--no-verify", "-m", "relevant again"], check=True, env=env) + _set_verified(repo, relevant) + assert health.verified_after_last_change(relevant, cwd=repo) is False + + +def test_a_no_op_refresh_still_keeps_the_map_healthy(): + """The regression #1683 introduced: unchanged entries push no commit, so the map's + _meta.git_sha stays behind while the refresh is working. The ref, not git_sha, is what + the health check reads -- an old git_sha must not read as STALE.""" + health = _health_module() + with tempfile.TemporaryDirectory() as d: + repo, relevant, later = _repo_with_history(d) + _set_verified(repo, later) + ok, msg = map_health( + meta={"built_at": "2026-05-28T00:00:00+00:00", "git_sha": "ancient", "n_tests": 1}, + current_keys={"a"}, + mapped_keys={"a"}, + now="2026-05-29T00:00:00+00:00", + max_age_days=10, + min_fraction=0.8, + verified_after_last_change=health.verified_after_last_change(health.verified_sha(cwd=repo), cwd=repo), + ) + assert ok, msg From 8a004dd1b0af8e37a8e20188471fb2bc55213ee2 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Tue, 18 Aug 2026 10:22:48 -0400 Subject: [PATCH 2/2] Review: anchor the ref to $GITHUB_SHA, fix the stale comment, run case files under sys.executable Copilot review: - Push $GITHUB_SHA, not HEAD, to refs/coverage-map/verified. When the entries changed, HEAD is the bot's new map commit, one past the commit the refresh actually rebuilt from. Either satisfies the health check's ancestry test, but only $GITHUB_SHA names the same thing in both branches of the guard. - Update the comment above actions/checkout in coverage-health.yml: it still explained the full-history requirement in terms of the map's _meta.git_sha. Separately, the cantera noise in the health job was not a missing install. The job's venv HAS cantera -- `Downloading cantera (18.4MiB)` appears in every run's init step. get_py_program_output ran case files under PATH's python3, and coverage-health.yml invokes build/venv/bin/python3 directly rather than activating the venv, so PATH still pointed at the setup-python interpreter. Every other entry point activates the venv first, which is why only this job saw it. Run case files under sys.executable. Under an activated venv that is the same binary, so no existing flow changes; it is only different where the venv interpreter is invoked by path, which is exactly the broken case. This also closes a real gap in the check: the 16 skipped chemistry cases were dropped from the min-fraction denominator, so coverage rot in the chemistry tests was invisible to the health workflow. Both new tests in test_common.py fail against the previous implementation and pass against this one. --- .github/scripts/check_coverage_map_health.py | 13 +++--- .github/workflows/coverage-health.yml | 5 ++- .github/workflows/coverage-refresh.yml | 6 ++- toolchain/mfc/common.py | 14 +++++- toolchain/mfc/test_common.py | 45 ++++++++++++++++++++ 5 files changed, 74 insertions(+), 9 deletions(-) create mode 100644 toolchain/mfc/test_common.py diff --git a/.github/scripts/check_coverage_map_health.py b/.github/scripts/check_coverage_map_health.py index 33e0747399..f4dddc9838 100644 --- a/.github/scripts/check_coverage_map_health.py +++ b/.github/scripts/check_coverage_map_health.py @@ -64,11 +64,14 @@ def main(): entries, meta = load_map(COVERAGE_MAP_PATH) if entries is None: sys.exit("Coverage map missing or corrupt.") - # Compute each current test's coverage key. Loading a case executes its case - # file; some (e.g. chemistry examples) import optional deps like cantera that are - # not installed in this lightweight job. Skip any case that fails to load instead - # of crashing -- map_health measures the fraction of *loadable* current tests that - # are mapped, so a smaller current_keys cannot produce a false "stale" result. + # Compute each current test's coverage key. Loading a case runs its case file as a + # subprocess, so anything that file imports must resolve. Keep skipping the ones that + # do not rather than crashing -- map_health measures the fraction of *loadable* + # current tests that are mapped, so a smaller current_keys cannot produce a false + # "stale" result -- but the skip is a safety net, not the expected path: every case + # here is meant to load. The 16 chemistry cases that used to land in this list did so + # because get_py_program_output ran them under PATH's python3 while this job invokes + # build/venv/bin/python3 directly, leaving the venv's cantera out of reach. current_keys = set() unloadable = [] for b in list_cases(): diff --git a/.github/workflows/coverage-health.yml b/.github/workflows/coverage-health.yml index 5835269fbd..e50d300272 100644 --- a/.github/workflows/coverage-health.yml +++ b/.github/workflows/coverage-health.yml @@ -9,8 +9,9 @@ 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. + # Full history: the freshness check asks whether refs/coverage-map/verified -- the + # commit the last successful refresh rebuilt from -- contains the last commit + # touching src/**/*.fpp or cases.py, which a shallow clone cannot answer. - uses: actions/checkout@v5 with: { fetch-depth: 0 } - name: Fetch the coverage-map verification ref diff --git a/.github/workflows/coverage-refresh.yml b/.github/workflows/coverage-refresh.yml index e9d1a04aaf..626faf5e3b 100644 --- a/.github/workflows/coverage-refresh.yml +++ b/.github/workflows/coverage-refresh.yml @@ -91,6 +91,10 @@ jobs: # either -- the warning lands in the run summary, and a ref that stops advancing # drops coverage-health.yml back to its wall-clock rule, which goes red within # MAX_AGE_DAYS. + # $GITHUB_SHA, not HEAD: when the entries changed, HEAD is the bot's new map + # commit, one past the commit this refresh actually rebuilt from. Either answers + # the health check's ancestry question, but only $GITHUB_SHA names the same thing + # in both branches -- the source state the map was built against. git push "https://x-access-token:${CACHE_PUSH_TOKEN}@github.com/MFlowCode/MFC.git" \ - --force HEAD:refs/coverage-map/verified \ + --force "$GITHUB_SHA:refs/coverage-map/verified" \ || echo "::warning::Could not update refs/coverage-map/verified; coverage-health.yml falls back to the age rule." diff --git a/toolchain/mfc/common.py b/toolchain/mfc/common.py index ff066c8672..c3985dc966 100644 --- a/toolchain/mfc/common.py +++ b/toolchain/mfc/common.py @@ -2,6 +2,7 @@ import os import shutil import subprocess +import sys import typing from os.path import abspath, dirname, join, normpath, realpath @@ -135,10 +136,21 @@ def get_program_output(arguments: typing.List[str] = None, cwd=None): def get_py_program_output(filepath: str, arguments: typing.List[str] = None): + """Run a case file and capture its stdout. + + sys.executable, not a bare "python3": a case file imports the same optional deps the + toolchain venv provides (cantera, pyrometheus, scipy, ...), so it must run under the + interpreter the toolchain itself is running under. Nearly every entry point activates + the venv first, which makes PATH's python3 the venv's -- but a caller that invokes + build/venv/bin/python3 DIRECTLY does not, and then case files silently ran under the + system interpreter instead. That is what made coverage-health.yml report 16 chemistry + cases as unloadable with ModuleNotFoundError: No module named 'cantera', in a job whose + venv had cantera installed. + """ dirpath = os.path.abspath(os.path.dirname(filepath)) filename = os.path.basename(filepath) - return get_program_output(["python3", filename] + arguments, cwd=dirpath) + return get_program_output([sys.executable, filename] + arguments, cwd=dirpath) def isspace(s: str) -> bool: diff --git a/toolchain/mfc/test_common.py b/toolchain/mfc/test_common.py new file mode 100644 index 0000000000..a5d9e952d4 --- /dev/null +++ b/toolchain/mfc/test_common.py @@ -0,0 +1,45 @@ +import os +import stat +import sys +import tempfile +from pathlib import Path +from unittest.mock import patch + +from mfc.common import get_py_program_output + + +def test_case_files_run_under_the_toolchain_interpreter(): + """A case file must run under sys.executable, not under whatever python3 PATH finds. + + coverage-health.yml invokes build/venv/bin/python3 directly instead of activating the + venv, so PATH's python3 was the system interpreter and every chemistry case failed with + ModuleNotFoundError: No module named 'cantera' -- in a job whose venv had cantera. The + case file reports the interpreter that ran it; it must be this one. + """ + with tempfile.TemporaryDirectory() as d: + case = Path(d) / "case.py" + case.write_text("import sys\nprint(sys.executable)\n") + out, err = get_py_program_output(str(case), []) + assert err == 0 + assert out.strip() == sys.executable + + +def test_a_hostile_python3_on_path_is_not_consulted(): + """Put a python3 on PATH that refuses to run anything; the case file must still load. + + This is the failure the health job hit, made deterministic: PATH's python3 is a + different, wrong interpreter. Asserting on sys.executable alone would still pass if + someone reintroduced a PATH lookup on a machine where the two happen to coincide. + """ + with tempfile.TemporaryDirectory() as d: + bindir = Path(d) / "bin" + bindir.mkdir() + fake = bindir / "python3" + fake.write_text("#!/bin/sh\necho 'wrong interpreter' >&2\nexit 3\n") + fake.chmod(fake.stat().st_mode | stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH) + case = Path(d) / "case.py" + case.write_text("print('loaded')\n") + with patch.dict(os.environ, {"PATH": f"{bindir}{os.pathsep}{os.environ.get('PATH', '')}"}): + out, err = get_py_program_output(str(case), []) + assert err == 0, "case file ran under PATH's python3 instead of sys.executable" + assert out.strip() == "loaded"