ci(coverage): key map freshness on the refresh run, not on _meta.git_sha - #1732
ci(coverage): key map freshness on the refresh run, not on _meta.git_sha#1732sbryngelson wants to merge 3 commits into
Conversation
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.
There was a problem hiding this comment.
Pull request overview
This PR fixes the “Coverage Map Health” CI false-positive by decoupling map content from map freshness: freshness is now keyed off a dedicated git ref that advances on every successful refresh run (including no-op refreshes), rather than _meta.git_sha (which only advances when a new map commit is pushed). It also increases the refresh workflow timeout to account for long SLURM queue delays, and adds unit tests around the new git predicates.
Changes:
- Update
map_healthto accept averified_after_last_changefreshness verdict (run-based) instead ofbuilt_after_last_change(map-content-based). - Teach the health workflow to fetch and use
refs/coverage-map/verified, and refactor the health script so its git predicates are importable/testable. - Increase
coverage-refresh.ymltimeout and add tests covering missing refs, true/false freshness, and the no-op refresh regression.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
toolchain/mfc/test/test_coverage_unit.py |
Updates existing health tests for the new API and adds new unit tests for the verified-ref predicates. |
toolchain/mfc/test/coverage.py |
Renames/clarifies the health API to accept run-based freshness (verified_after_last_change) and updates messaging. |
.github/workflows/coverage-refresh.yml |
Raises timeout and records refresh freshness via refs/coverage-map/verified after each successful refresh. |
.github/workflows/coverage-health.yml |
Fetches the verification ref explicitly (since it’s outside heads/tags) and tolerates it being absent. |
.github/scripts/check_coverage_map_health.py |
Implements verified_sha / verified_after_last_change, adds a main() entrypoint guard for unit testing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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." |
| @@ -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 | |||
…e 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.
|
Both review comments applied in 8a004dd, plus the root cause of the 1. 2. Stale comment above 3. The
Fixed at the source: case files now run under This closes a real gap rather than just quieting logs — those 16 chemistry cases were dropped from the Both new tests in |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1732 +/- ##
=======================================
Coverage 61.59% 61.59%
=======================================
Files 84 84
Lines 21493 21493
Branches 3176 3176
=======================================
Hits 13239 13239
Misses 6078 6078
Partials 2176 2176 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Description
The daily Coverage Map Health cron has failed every day since 2026-08-16 (latest run) with:
The refresh workflow is not broken. #1683 added two things in one commit that contradict each other:
coverage_map_changed.py— the refresh pushes a commit only when the rebuilt coverage entries differ;_meta(built_at,git_sha) is deliberately excluded from the comparison, so a no-op refresh pushes nothing.built_after_last_change()— health requires the map's_meta.git_shato descend from the last commit touchingsrc/**/*.fpportoolchain/mfc/test/cases.py._meta.git_shaonly advances when a commit lands. So the first coverage-relevant commit whose coverage happens to be unchanged pinsgit_shain the past permanently, and the health check reports a working refresh as broken. Nothing self-heals it; only a later commit that actually moves the entries would.The committed map is at
git_sha: ae4b4c40(#1414, Aug 11). Four coverage-relevant commits landed after it, each with a refresh run that printedCoverage entries unchanged (675 tests) -> nothing to push:bb29773b7(#1718)3db0bd175(#1722)b7d788384(#1686)ca26c4cd4(#1721)(The
ModuleNotFoundError: No module named 'cantera'tracebacks in the health log are not the cause of the STALE failure — those cases are printed underNote: 16 case(s) could not be loadedand are excluded from the check. They are a separate real bug, fixed below.)The fix
Record the freshness signal where it can actually be observed — on the refresh run, not in the map's content:
coverage-refresh.ymlmovesrefs/coverage-map/verifiedto the commit it rebuilt from on every successful refresh, in both branches of the commit guard. The push is non-fatal: the map is already pushed by that point, so failing this bookkeeping step must not red a refresh that worked — and it is not silent, because a ref that stops advancing drops the health check back to its wall-clock rule, which goes red withinMAX_AGE_DAYS.check_coverage_map_health.pyasks whether that ref descends from the last coverage-relevant commit. A missing ref (fork, or the window before the first refresh after this lands) reads as undeterminable and falls back to the wall-clock age rule, exactly as a shallow clone already does.refs/heads/andrefs/tags/on purpose: updating it fires nopushworkflow trigger. A branch or tag would starthomebrew.yml, which filters onpaths:alone with no ref filter.Also: the
canterafailures are an interpreter bug, not a missing installThe existing code comment says those optional deps are "not installed in this lightweight job". That is wrong — every health run's init step logs
Downloading cantera (18.4MiB)/Installed 96 packages.common.get_py_program_outputran case files with a hardcoded"python3"taken from PATH.coverage-health.ymlinvokesbuild/venv/bin/python3directly instead of activating the venv, so PATH still resolved to the setup-python interpreter, which has no cantera. Every other entry point goes through./mfc.sh, which activates the venv and makes PATH'spython3the venv's — which is why only this job ever saw it.Case files now run under
sys.executable. Under an activated venv that is the same binary, so no existing flow changes; it differs only where the venv interpreter is invoked by path.This closes a gap rather than just quieting logs: those 16 chemistry cases were dropped from the
MIN_FRACTIONdenominator, so coverage rot in the chemistry tests was invisible to the health check.Also: the refresh timeout
Refresh run 32095131563 (push of #1717, which changed
cases.py) was killed at 4h05m bytimeout-minutes: 240. Wall clock there is dominated by the phoenix SLURM queue, not the build — observed waits reach 6h03m. Raised to 480. That timeout is why the map is now also behind the current test list, independent of the bug above.Type of change
Testing
ruff format --check,ruff check,typos, and the full toolchain unit suite (422 passed) pass locally.New unit tests in
test_coverage_unit.pydrive the two git predicates against a throwaway repository:test_verified_sha_is_none_when_the_ref_was_never_pushed— absent ref is undeterminable, not brokentest_verified_after_last_change_true_when_a_refresh_ran_since_the_change— including a refresh at the changetest_verified_after_last_change_false_when_the_refresh_predates_the_change— the genuine broken-refresh casetest_a_no_op_refresh_still_keeps_the_map_healthy— the regression this PR fixes: an ancient_meta.git_shawith a current ref must read healthycheck_coverage_map_health.pygained anif __name__ == "__main__":guard so its predicates are importable; it was previously untestable top-level script code.Two tests in the new
toolchain/mfc/test_common.pycover the interpreter fix, both verified to fail against the previous implementation:test_case_files_run_under_the_toolchain_interpreter— the case file reports the interpreter that ran ittest_a_hostile_python3_on_path_is_not_consulted— PATH is poisoned with apython3that exits 3, so the test cannot pass by coincidence on a machine where the two interpreters happen to matchAfter merging: dispatch
Coverage Map Refreshonce by hand to createrefs/coverage-map/verified. Until it exists the health check falls back to the 10-day age rule, and the current map (built 2026-08-11) crosses that on 2026-08-21 — before the Monday cron. That run also brings the map up to date with #1717's test-list changes, which the timeout above prevented.Checklist