From d6aacfb42a66b69ab9abe749dd5e50191e317af5 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Fri, 11 Sep 2026 11:20:10 -0700 Subject: [PATCH 1/2] Stop excluding tests by band, let tier C report instead of gate, and quote $PYTEST MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The bands were never a cost decision.** `test_06*` was disabled as "potentially problematic" and stayed that way; `test_106*`/`test_107*` were held back pending "a triage/deselect decision" that nobody made. Measured 2026-09-12: those three bands hold 243 tests, of which only 26 are level_3 or tier_c. 55 are level_1 AND tier_a — hardened, fast, and exactly what CI exists to run. One slow test (test_1064) was quarantining its neighbours because they shared a number. They run now. The only failure among them was a test-side defect, fixed separately in #728. **This is affordable today.** The comment justifying the 120-minute cap measured the serial phase at ~55 min on 2026-08-15, before xdist. The five successful runs before 2026-09-12 spent 34, 35, 43, 44 and 48 minutes in "Run tests". Sharding across parallel jobs stays the durable fix if this tightens again — referenced in the workflow as "the CI-runtime issue" but never filed, now #730. **Tier C reports rather than gates.** A tier C failure demands an EXPLANATION, not a revert (Charter §8, TESTING-RELIABILITY-SYSTEM.md): these are characterisations that can fail because the code got BETTER. They are excluded from the batches that set $status and run in their own pass at the end, which is read but cannot fail the build. **$PYTEST is now an array.** It was a string expanded UNQUOTED at all 17 call sites, which made adding any argument containing a space unsafe: bash splits `-m 'not tier_c'` into the words `'not` and `tier_c'`, and pytest answers that by silently collecting NOTHING and exiting 0 — a green run that tested nothing: $ pytest -m "'not" "level_2'" tests/test_1070_free_surface_plume.py no tests collected in 0.00s # exit 0 The first version of this change dodged it through PYTEST_ADDOPTS, which pytest shlex-splits. Quoting the variable removes the hazard instead of routing around it, so the next argument someone adds is safe too. Verified by dry run that the marker arrives as a single argument, `ARG[not tier_c]`, in both the xdist and in-process branches. **Also drops a line that now runs twice.** `test_1072` was pulled forward out of test_107* by name because that band was not batched. The band runs as a whole now, so the named line is redundant; it is still covered, once. Verified by dry run with a stub pytest: every gating batch carries the exclusion as one argument, the previously-deferred bands appear, test_1072 appears once, and the tier C pass runs without the exclusion. Underworld development team with AI support from Claude Code --- .github/workflows/build_uw3_and_test.yaml | 17 ++-- scripts/test.sh | 97 ++++++++++++++++------- 2 files changed, 78 insertions(+), 36 deletions(-) diff --git a/.github/workflows/build_uw3_and_test.yaml b/.github/workflows/build_uw3_and_test.yaml index e7286f99a..d049cedba 100644 --- a/.github/workflows/build_uw3_and_test.yaml +++ b/.github/workflows/build_uw3_and_test.yaml @@ -45,13 +45,16 @@ env: jobs: test: runs-on: ubuntu-latest - # The suite has outgrown 60 minutes. Measured 2026-08-15: the SERIAL phase - # alone is ~55 min (three batches at 14-15 min each), so the parallel phase - # - which runs last - was being cut off mid-run and reported as a failure - # that looked like a test failure. Any addition at all pushed a run over. - # This is a stop-gap that unblocks landing work; the durable fix is to split - # the batches across parallel jobs so wall-clock stops tracking total test - # time. See the CI-runtime issue for that. + # Raised past 60 minutes on a 2026-08-15 measurement (serial phase ~55 min). + # xdist has since cut that: the five successful runs before 2026-09-12 spent + # 34, 35, 43, 44 and 48 minutes in "Run tests", well inside this budget. The + # headroom is why the previously-deferred bands could simply be switched on + # (#721 follow-up) rather than traded against something else. + # + # Sharding the batches across parallel jobs remains the durable fix if this + # tightens again; it was referenced here as "the CI-runtime issue" but never + # actually filed, so the numbers above are recorded in the issue that this + # change opens. timeout-minutes: 120 steps: diff --git a/scripts/test.sh b/scripts/test.sh index 953d24b6a..3baceb943 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -78,14 +78,33 @@ export MKL_NUM_THREADS=1 # # Unset (or 1) still runs everything in one process, which is what you want # when a test passes alone and fails in a full run. +# An ARRAY, not a string. Every call site below expands it unquoted, so a string +# would be word-split by bash: `-m 'not tier_c'` becomes the two words `'not` and +# `tier_c'`, and pytest answers that by silently collecting NOTHING and exiting 0 +# — a green run that tested nothing. An array carries its elements intact through +# "${PYTEST[@]}", and globs on the command line still expand normally. +PYTEST=(pytest --config-file=tests/pytest.ini) if [ -n "$WORKERS" ] && [ "$WORKERS" -gt 1 ]; then echo "Serial batches: $WORKERS worker process(es)" - PYTEST="pytest --config-file=tests/pytest.ini --dist loadfile -n $WORKERS" + PYTEST+=(--dist loadfile -n "$WORKERS") else echo "Serial batches: in-process (set WORKERS=N to distribute)" - PYTEST="pytest --config-file=tests/pytest.ini" fi +# Tier C never gates. A tier C failure demands an EXPLANATION, not a revert +# (Charter S8, docs/developer/TESTING-RELIABILITY-SYSTEM.md): these are +# characterisations that can fail because the code got BETTER, and comparisons +# whose result moves with the fixture. They are excluded from the batches that +# set $status and run in their own reporting pass at the end, which is read but +# cannot fail the build. +# Tier C never gates. A tier C failure demands an EXPLANATION, not a revert +# (Charter S8, docs/developer/TESTING-RELIABILITY-SYSTEM.md): these are +# characterisations that can fail because the code got BETTER, and comparisons +# whose result moves with the fixture. They are excluded from the batches that +# set $status and run in their own reporting pass at the end, which is read but +# cannot fail the build. +PYTEST+=(-m "not tier_c") + # Run serial tests (unless --parallel-only specified) if [ $PARALLEL_ONLY -eq 0 ]; then echo "Running serial test suite..." @@ -98,35 +117,37 @@ if [ $PARALLEL_ONLY -eq 0 ]; then python3 "$(dirname "$0")/check_test_coverage.py" || status=1 # Run simple tests (0000-0299: basic functionality, imports, simple operations) - $PYTEST tests/test_00[0-4]*py || status=1 - $PYTEST tests/test_0050*py || status=1 + "${PYTEST[@]}" tests/test_00[0-4]*py || status=1 + "${PYTEST[@]}" tests/test_0050*py || status=1 # test_006[2-9] and test_007x matched NO batch glob and so never ran in CI: # the whole integration-point suite (0064-0067), swarm repopulation, # mid-time velocity and the VE stress history. They are not covered by the # disabled test_06*py line below either - that one is 0600-0699. The band is # taken whole (00[6-7]) rather than enumerated, so a test added next to its # siblings is not dark again. Verified passing (103 tests) before wiring in. - $PYTEST tests/test_005[1-9]*py tests/test_00[6-7]*py || status=1 - $PYTEST tests/test_01*py || status=1 - $PYTEST tests/test_02*py tests/test_03*py || status=1 + "${PYTEST[@]}" tests/test_005[1-9]*py tests/test_00[6-7]*py || status=1 + "${PYTEST[@]}" tests/test_01*py || status=1 + "${PYTEST[@]}" tests/test_02*py tests/test_03*py || status=1 # Intermediate tests (0500-0799: data structures, transformations, enhanced interfaces) - # NOTE: Temporarily disabling test_06*py regression tests (potentially problematic) - $PYTEST tests/test_05*py tests/test_07*py || status=1 - # $PYTEST tests/test_06*py || status=1 # DISABLED - regression tests need validation + # test_06*py was disabled as "potentially problematic" and stayed that way. The + # whole band was measured 2026-09-12 and passes; a band is not a unit of trust, + # and holding one back for a suspicion nobody recorded meant its level_1 files + # never ran at all. + "${PYTEST[@]}" tests/test_05*py tests/test_06*py tests/test_07*py || status=1 # Units system tests (0800-0899: unit-aware functions, arrays, and conversions) - $PYTEST tests/test_08*py || status=1 + "${PYTEST[@]}" tests/test_08*py || status=1 # Poisson solvers (including Darcy flow) - $PYTEST tests/test_100[0-9]*py tests/test_103*py || status=1 + "${PYTEST[@]}" tests/test_100[0-9]*py tests/test_103*py || status=1 # Solver / system tests (advanced solver problems) # test_101* / test_102* include the rotated free-slip suite (test_1018, # issue #504) and the MG / boundary-flux suites, which previously matched # no batch glob and never ran in CI. - $PYTEST tests/test_101*py tests/test_102*py || status=1 - $PYTEST tests/test_105*py || status=1 + "${PYTEST[@]}" tests/test_101*py tests/test_102*py || status=1 + "${PYTEST[@]}" tests/test_105*py || status=1 # The boundary-normal guard lives under tests/parallel/ but carries NO # mpi(min_size=2) mark, because the defect it guards is present in SERIAL in @@ -134,26 +155,27 @@ if [ $PARALLEL_ONLY -eq 0 ]; then # spherical shell at np=1). Run it here so the serial job covers that path — # every other serial test of the default boundary normal is on a box, where # flat walls make the question vacuous. It also runs in the --p N batch below. - $PYTEST tests/parallel/test_1069_boundary_normal_parallel.py || status=1 - # NOT yet batched (issue #504 audit): test_106*py and test_107*py contain - # level_2/level_3 + slow + tier_b/tier_c suites (e.g. test_1064) and need - # a triage/deselect decision before being wired into CI. + "${PYTEST[@]}" tests/parallel/test_1069_boundary_normal_parallel.py || status=1 + # test_106*/test_107* were held back pending "a triage/deselect decision". That + # decision is made: the band is not the unit of exclusion. Of the 243 tests in + # the deferred bands only 26 are level_3 or tier_c; 55 are level_1 AND tier_a — + # hardened, fast, and exactly what CI exists to run. test_1064 is the slow one + # and it is excluded by WHAT IT IS (tier_c) below, not by its number. + "${PYTEST[@]}" tests/test_106*py tests/test_107*py || status=1 # - # test_1072 is pulled forward out of that group, the same way test_1069 is - # above, because leaving it there defeats its purpose: it is the ONLY guard on - # the 3-D free-surface sign and relaxation rate, and #496 exists precisely - # because those regressions were invisible to CI. Landing the test into an - # unbatched file would have closed the issue without closing the gap. - # level_2/tier_b, ~55s serial; passes at np=1 and np=2. - $PYTEST tests/test_1072_free_surface_spherical.py || status=1 + # test_1072 used to be pulled forward out of test_107* by name, because that + # band was not batched and it is the ONLY guard on the 3-D free-surface sign + # and relaxation rate (#496 exists because those regressions were invisible to + # CI). The band runs as a whole now, so the named line is gone rather than + # running it twice. # Diffusion / Advection tests - $PYTEST tests/test_1100*py || status=1 - $PYTEST tests/test_1110*py tests/test_1120*py || status=1 # Annulus + vector SL - $PYTEST tests/test_1450*py || status=1 + "${PYTEST[@]}" tests/test_1100*py || status=1 + "${PYTEST[@]}" tests/test_1110*py tests/test_1120*py || status=1 # Annulus + vector SL + "${PYTEST[@]}" tests/test_1450*py || status=1 # Named (un-numbered) test files - JIT, docstrings, projections - $PYTEST tests/test_docstring_utils.py tests/test_jit_cache.py \ + "${PYTEST[@]}" tests/test_docstring_utils.py tests/test_jit_cache.py \ tests/test_jit_deterministic_ordering.py \ tests/test_multicomponent_projection.py \ tests/test_snes_vector_asymmetric_jacobian.py \ @@ -249,6 +271,23 @@ else echo "⚠️ Skipping parallel tests (use --p N to enable)" fi +# Tier C: run and report, never gate. Failures here are read by a human and +# answered with an explanation or a re-characterisation, never with a revert. +if [ $PARALLEL_ONLY -eq 0 ]; then + echo "" + echo "==========================================" + echo "Tier C characterisations (reported, not gating)" + echo "==========================================" + if pytest --config-file=tests/pytest.ini -m tier_c tests/; then + echo "Tier C: all characterisations still hold." + else + echo "" + echo "⚠️ A tier C characterisation changed. This does NOT fail the build." + echo " Explain what moved and re-characterise the test — do not revert" + echo " code to make it pass. See docs/developer/TESTING-RELIABILITY-SYSTEM.md." + fi +fi + # if [ $status -ne 0 ]; then echo "" From ab9e32a09be74e850d7df58f80b0e29681a70da2 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Fri, 11 Sep 2026 11:49:55 -0700 Subject: [PATCH 2/2] Review (#731): teach the coverage guard the array call form, and retire DEFERRED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- scripts/check_test_coverage.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/scripts/check_test_coverage.py b/scripts/check_test_coverage.py index a384035f0..514f89f39 100644 --- a/scripts/check_test_coverage.py +++ b/scripts/check_test_coverage.py @@ -20,16 +20,11 @@ # Deferred by maintainer decision, not by accident. DEFERRED = { - # No issue: this band was disabled in scripts/test.sh as "potentially - # problematic" without one being filed. Recorded as it stands rather than - # dressed up — #721 follow-up work re-enables it and removes this entry. - "test_06[0-9][0-9]_*": "regression suite disabled in test.sh, no issue filed", - # Narrow: test_1072 is pulled out of this band and run by name, so a broad - # test_107* would list a covered file as deferred. - "test_1070_*": "level_2/level_3 + tier_b/tier_c, awaiting triage (#504)", - "test_1071_*": "level_2/level_3 + tier_b/tier_c, awaiting triage (#504)", - "test_1073_*": "level_2/level_3 + tier_b/tier_c, awaiting triage (#504)", - "test_106*": "level_2/level_3 + tier_b/tier_c, awaiting triage (#504)", + # Empty, and that is the point. Every band that used to sit here — the + # test_06NN regression suite and test_106*/test_107* — is now batched in + # scripts/test.sh. 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 sits in the numbering. } @@ -38,7 +33,10 @@ def globs_run_by(script): text = script.read_text().replace("\\\n", " ") patterns = set() for line in text.splitlines(): - if "$PYTEST" in line and not line.lstrip().startswith("#"): + # Both spellings: the runner is a bash ARRAY, so call sites read + # "${PYTEST[@]}", but a plain $PYTEST is still worth matching in case + # one is left behind or reintroduced. + if re.search(r"\$\{?PYTEST", line) and not line.lstrip().startswith("#"): patterns.update(re.findall(r"tests/[A-Za-z0-9_\[\]*.\-]+", line)) return patterns