A guard for the test globs, and 22 session scripts out of the test tree (#721) - #723
Open
lmoresi wants to merge 2 commits into
Open
A guard for the test globs, and 22 session scripts out of the test tree (#721)#723lmoresi wants to merge 2 commits into
lmoresi wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved coverage-guard behavior and stale usage documentation remain.
Pull request overview
This PR closes CI test-discovery gaps and moves development session scripts out of tests/.
Changes:
- Expands serial test globs and adds coverage validation.
- Tracks intentionally deferred test suites.
- Relocates session scripts and updates selected output paths and documentation.
File summaries
| File | Summary |
|---|---|
scripts/test.sh |
Updated test globs and coverage-guard invocation; guard-location documentation needs correction. |
scripts/sessions/vep_timedep_yield.py |
Relocated session script. |
scripts/sessions/vep_strain_weakening.py |
Relocated session script. |
scripts/sessions/vep_fault_weakening.py |
Relocated session script. |
scripts/sessions/run_vp_shear_box.py |
Relocated session script. |
scripts/sessions/run_vep_shear_box.py |
Relocated session script. |
scripts/sessions/run_vep_oscillatory.py |
Relocated session script. |
scripts/sessions/run_ve_vep_oscillatory_plot.py |
Relocated session script. |
scripts/sessions/run_ve_vep_oscillatory_checkpoint.py |
Relocated session script. |
scripts/sessions/run_ve_shear_validation.py |
Relocated script; usage example still references the old path. |
scripts/sessions/run_ve_shear_quick.py |
Relocated session script. |
scripts/sessions/run_ve_shear_order2_quick.py |
Relocated session script. |
scripts/sessions/run_ve_oscillatory.py |
Relocated session script. |
scripts/sessions/run_ve_order2_debug.py |
Relocated session script. |
scripts/sessions/run_snapshot_backstepping_spatial.py |
Relocated script; usage path and orchestration-model naming need updates. |
scripts/sessions/run_snapshot_backstepping_demo.py |
Relocated script; usage path and orchestration-model naming need updates. |
scripts/sessions/README.md |
Documents the relocated session scripts. |
scripts/sessions/profile_jit_phases.py |
Relocated script; usage example still references the old path. |
scripts/sessions/plot_ve_vep_oscillatory.py |
Relocated plotting script. |
scripts/sessions/plot_ve_oscillatory_validation.py |
Relocated script; usage examples still reference the old path. |
scripts/sessions/plot_ve_combined.py |
Relocated plotting script; producer and loader documentation is stale. |
scripts/sessions/minimal_vep_timing.py |
Relocated script; usage line still references the old path. |
scripts/sessions/benchmark_projection_evaluate.py |
Relocated benchmark script. |
scripts/sessions/benchmark_index_swarm_vectorized.py |
Relocated benchmark; documented invocation still references the old path. |
scripts/check_test_coverage.py |
Adds coverage validation; deferred-pattern traceability, stale-entry handling, and explicit coverage exclusions need correction. |
Review details
Suppressed comments (4)
scripts/check_test_coverage.py:25
- The first deferred pattern records only “pending validation (see test.sh)”, while the PR description says every deferred entry carries both a reason and an issue. The nearby
test_106*/test_107*entries have#504, but thistest_06NNentry has no issue to make the deferral traceable; add the governing issue (or correct the stated contract).
"test_06[0-9][0-9]_*": "regression suite disabled pending validation (see test.sh)",
scripts/check_test_coverage.py:52
DEFERREDis only subtracted fromevery; there is no stale-entry check when a deferred file later becomes covered by a live glob (or when a deferred pattern becomes empty). Obsolete entries can therefore remain indefinitely while the output still reports them as deferred, so the documented shrink-only invariant is not enforced. Reject stale deferred entries, analogous to the allowlist check incheck_deprecated_patterns.py.
deferred = set()
for pattern in DEFERRED:
deferred.update(Path(p).name for p in glob.glob(str(REPO / "tests" / pattern)))
every = {p.name for p in (REPO / "tests").glob("test_*.py")}
dark = sorted(every - covered - deferred)
scripts/check_test_coverage.py:27
- This pattern also matches
tests/test_1072_free_surface_spherical.py, butscripts/test.shexplicitly runs that file at line 147 after pulling it out of the deferred group. The coverage check therefore reports an actively covered test as deferred, and the broad pattern can mask future explicitly re-enabled 107x tests. Narrow this entry to the still-deferred 1070/1071 files (or otherwise exclude covered files).
"test_107*": "level_2/level_3 + tier_b/tier_c, awaiting triage (#504)",
scripts/test.sh:107
- The guard is invoked before this batch at lines 94–96, not at the end of this block, so this explanation points maintainers to the wrong location when they update the globs. Say “the guard above” (or refer to
check_test_coverage.py) instead.
# #570 class of defect; the guard at the end of this block now fails
# the run if any test file matches no glob at all.
- Files reviewed: 6/28 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ee (#721) 586224f already closed the 006x-007x gap on development, and took the band whole rather than enumerating it — the better fix, and it makes that particular range safe from recurrence. This is what is left of #721 after it. **The other ranges are still hand-maintained.** `scripts/check_test_coverage.py` fails the run when any `tests/test_*.py` matches no glob in `scripts/test.sh`. Run against development as it stands, it finds two files the band fix could not reach, because they are nowhere near that band: tests/test_0301_meshvariable_clone.py no test_03* glob existed tests/test_1030_analytic_zhong2008.py no test_103* glob existed Both globs are added here. Files deferred by decision are listed in DEFERRED with their reason and issue, on the same terms as the allowlist in `check_deprecated_patterns.py`: the list may only shrink. The deferred pattern is written `test_06[0-9][0-9]_*` rather than `test_06*`, because the latter also swallows `test_0062..test_0069` — a different suite, which must run. **The test tree.** 22 session scripts — drivers, plotting, benchmarks, profilers — were sitting in `tests/` among the real test files. They are not tests, nothing imports them, and they made the directory hard to read. They move to `scripts/sessions/` with a README. Three wrote their figures to an absolute path inside a worktree that no longer exists, so they had been writing nowhere since that worktree was removed; they now write beside themselves, and the three stale figures those runs left in the repository root are removed. `_mg_ladder.py` stays in `tests/` — it is a test helper and two tests import it. Guard passes: all 302 test files reachable, 25 deferred by decision. Closes #721 Underworld development team with AI support from Claude Code
lmoresi
force-pushed
the
docs/audit-2026-09
branch
from
September 11, 2026 18:13
fac3b9c to
fb4ccb3
Compare
lmoresi
changed the title
… noticed a stale entry Three defects in `check_test_coverage.py`, all found by Copilot review. **The deferred pattern claimed a file that runs.** `test_107*` also matches `test_1072_free_surface_spherical.py`, which `scripts/test.sh` pulls out of that band and runs by name. The guard therefore reported an actively covered test as deferred, and the broad pattern would have masked any further 107x file that got re-enabled. Narrowed to the files actually deferred: 1070, 1071, 1073. **The shrink-only invariant was not enforced.** DEFERRED was only subtracted from the file list, so an entry that stopped being true — its files now covered, or no longer present — sat there indefinitely while the output still counted it as a deliberate deferral. That is how a shrink-only list quietly becomes a fiction. The guard now rejects both cases, and says which entry and why. Verified against a probe carrying one of each: it reports "matches only files that already run" and "matches no test file", and exits 1. **A deferral with no issue behind it.** The PR said every deferred entry carries a reason and an issue; the `test_06NN` entry carried only "pending validation". There is no issue — the band was disabled in `test.sh` as "potentially problematic" and nothing was filed. Recorded as it stands rather than dressed up, with a note that the #721 follow-up re-enables the band and removes the entry. **Stale usage paths.** Eleven of the moved scripts still told the reader to run them from `tests/`, which my own move invalidated. Repointed to `scripts/sessions/`. Guard passes: all 302 test files reachable, 24 deferred by decision. Underworld development team with AI support from Claude Code
lmoresi
added a commit
that referenced
this pull request
Sep 11, 2026
…re DEFERRED Rebased onto #723 so the two changes land consistently, which immediately surfaced an integration defect the guard itself caught: `globs_run_by` matched the literal `$PYTEST`, and this PR rewrites every call site to the array form `"${PYTEST[@]}"`. The guard therefore found NO globs and declared all 302 test files dark. It now matches both spellings. **DEFERRED is empty, and that is the point.** Every band that sat there — the test_06NN regression suite and test_106*/test_107* — is batched now. Nothing is excluded from CI by its number any more; a test that should not gate says so with `@pytest.mark.tier_c`, which is a property of the test rather than of where it falls in the numbering. **Dependencies, which the PR did not declare.** Copilot was right that this cannot land alone: - #728 fixes `test_1060_nitsche_freeslip`, whose bound is still `<1e-4` against an observed 1.0081e-4. Enabling `test_106*` without it makes this batch fail. - #729 puts the `tier_c` mark on the `test_1070` comparison and updates the tier definitions in `tests/pytest.ini` and the docs. Without it this PR enables `test_107*` while that comparison still gates, and the reporting pass contradicts a marker contract that still reads "development only, not for automation". Merge order is #728, #729, #723, then this. The PYTEST_ADDOPTS point in the same review was already addressed: the array landed in an amended commit the review had not seen. It also removes the objection behind it — an inherited PYTEST_ADDOPTS from the caller is no longer discarded, because the script no longer sets it. Verified: dry run shows the marker arriving as a single `ARG[not tier_c]`, the tier C pass running without it, and the guard passing with 302 reachable and 0 deferred. Underworld development team with AI support from Claude Code
Member
Author
|
Review points addressed in The deferred pattern claimed a file that runs. Correct — No stale-entry check. Also correct, and the sharper point: A deferral with no issue behind it. The Stale usage paths. Eleven moved scripts still told the reader to run them The comment about the guard's location was already corrected in the rebase — it |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased onto current
development.586224f6already closed the 006x-007x gapand took the band whole rather than enumerating it — the better fix, and it
makes that range safe from recurrence. My widening of it is dropped. This is
what is left of #721 after that.
The other ranges are still hand-maintained
scripts/check_test_coverage.pyfails the run when anytests/test_*.pymatches no glob in
scripts/test.sh. Run againstdevelopmentas it stands, itfinds two files the band fix could not reach, because they are nowhere near that
band:
Both globs are added here.
Files deferred by decision go in a
DEFERREDlist with their reason and issue,on the same terms as the allowlist in
check_deprecated_patterns.py— it mayonly shrink. The deferred pattern is written
test_06[0-9][0-9]_*rather thantest_06*, because the latter also swallowstest_0062..test_0069, a differentsuite which must run.
The test tree
22 session scripts — drivers, plotting, benchmarks, profilers — were sitting in
tests/among the real test files. They are not tests, nothing imports them,and they made the directory hard to read. They move to
scripts/sessions/witha README.
Three of them wrote their figures to an absolute path inside a worktree that no
longer exists, so they had been writing nowhere since that worktree was removed.
They now write beside themselves, and the three stale figures those runs left in
the repository root are removed.
_mg_ladder.pystays intests/— it is a test helper and two tests import it.Verified
Guard passes: all 302 test files reachable from
scripts/test.sh, 25 deferredby decision.
Closes #721
Underworld development team with AI support from Claude Code