Tier C is what does not gate: the comparison tests, and the Charter wording for them - #729
Merged
Merged
Conversation
…ording for them Three tests asserted that one method is more accurate than another. That shape of assertion encodes a preference rather than a contract — the result moves with the fixture, the mesh, the forcing and every default the two methods carry — and it can fail because the code got BETTER, which is not a regression: test_0773_surface_smoother.py taubin_low_kept > laplacian_low_kept + 0.03 test_0066_integration_point_slcn.py l2_ip <= l2_nodal, peak_ip >= peak_nodal test_1070_free_surface_plume.py errors["strong"] < 0.5 * errors["penalty"] Each also carried a real contract — an absolute bound against a known answer with no rival method in it. Those stay where they are and keep gating: the Taubin passband bounds, the integration-point L2 error, and the net volume flux through the free surface. `test_0066` gains `test_rotating_gaussian_ip_accuracy` so its contract stands alone rather than riding on the comparison. The comparisons move to tier C, which the maintainer ruling (2026-09-12) defines by what it does rather than by how immature it is: a tier C failure NEVER blocks a change, it demands an explanation. Each now says so in its failure message — "if the nodal path improved, explain it and re-characterise; do not revert to make this pass" — and records its measured numbers and configuration, dated, in the docstring. Charter §8 gains the rule that produced this: assert against a known answer, not against a rival method; convergence ORDER and mathematical exactness are contracts and may be asserted freely, "method A scored better than method B here" may not. Plus the tell that decides the tier — if an assertion would break when the code gets better, it belongs at tier C — and a note that the number in a test filename sets a broad sequence, nothing more, so a shared number is not a conflict. TESTING-RELIABILITY-SYSTEM.md, which governs the tiers, had tier C as "Experimental ... NEVER for automated CI". That is one population, not the definition, and it conflicts with a characterisation that SHOULD run and be reported. Tier C is now defined by not gating, with the two populations named: C1 characterisation, which never promotes, and C2 experimental, which promotes to B once the feature lands. Both remain unusable as a basis for coding, which was the original point of the tiers. Also records the precedence a per-test mark needs: the narrower tier wins, so a tier_c test inside a tier_a module is tier C and is excluded by `-m "not tier_c"`. Verified: 37 passed (0773, 0066), 6 passed 1 skipped (1070); `-m tier_c` selects exactly the three characterisations, `-m "not tier_c"` the other 41. Underworld development team with AI support from Claude Code
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Tier C tests can still gate or be skipped, while the documentation and CI configuration remain inconsistent with the intended policy.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR moves method-comparison assertions to non-gating Tier C characterization tests, adds an independent accuracy contract, and updates testing guidance.
Changes:
- Reclassifies comparisons in tests 0773, 0066, and 1070.
- Adds standalone integration-point accuracy coverage.
- Documents Tier C behavior and assertion guidance.
File summaries
| File | Review summary |
|---|---|
tests/test_1070_free_surface_plume.py |
Tier C remains additive with tier_b; the comparison can still gate, the flux contract is also made non-gating, and the test is skipped by default CI. Required baselines and configuration details are incomplete. |
tests/test_0773_surface_smoother.py |
Tier C remains additive with tier_a; the comparison can still gate. The 0.97 contract is no longer gating, and the characterization omits the measured Laplacian baseline. |
tests/test_0066_integration_point_slcn.py |
Tier C remains additive with tier_a, so the comparison can still gate. The characterization lacks dated measured L2 and peak values. |
docs/developer/UW3_STYLE_CHARTER.md |
The documented tier precedence does not match pytest mark merging or the current release-gate selector. |
docs/developer/TESTING-RELIABILITY-SYSTEM.md |
CI and release-gate behavior do not implement the stated non-gating policy, and later marker documentation and promotion rules remain inconsistent. |
Review details
Suppressed comments (3)
docs/developer/TESTING-RELIABILITY-SYSTEM.md:117
- The C1/C2 rewrite is contradicted by older rules later in this governing document: the pytest.ini snippet still labels
tier_cexperimental and not for automation, the run commands calltier_a or tier_bfull validation, and the old C→B promotion checklist remains. Maintainers therefore get incompatible definitions of Tier C; update the remaining sections and marker registration to implement the new 'runs but never gates' policy.
**Promotion path**: C2 → B once the feature is implemented, the test passes
consistently and a developer confirms the test itself is correct. C1 does not
promote — a characterisation is Tier C permanently, by its nature.
tests/test_0773_surface_smoother.py:73
- The new characterization requirement calls for measured numbers for the compared paths. This records Taubin as approximately 0.99 but only says the plain-Laplacian result was 'markedly less', so the baseline is not reproducible from the docstring. Include the observed Laplacian value as well as the existing fixture settings.
Measured 2026-09-12 at n_iters=40, alpha=0.6: Taubin keeps ~0.99 of the low
mode, plain Laplacian markedly less. The 0.03 margin is a characterisation
of this fixture, not a specification.
tests/test_1070_free_surface_plume.py:265
- This C1 docstring gives the qualitative comparison but omits the measured values, fixture configuration, and date required by the new Tier C documentation rule. Record the strong/penalty error and leak baselines plus the settings that produced them, so a failure can be re-characterized from the documented run.
r"""With a flux-free datum the STRONG rotated constraint holds the surface as a
material boundary far better than the weak penalty, and does not leak volume.
This is the reason ``consistent_constraint`` defaults to ``"strong"``: the penalty
both misses the prescribed rate and passes a net volume flux through the surface,
- Files reviewed: 5/5 changed files
- Comments generated: 11
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+69
to
+71
| **The defining property**: a Tier C failure NEVER blocks a change. It demands an | ||
| explanation. Tier C tests still run in CI and are still read — they are excluded | ||
| from what gates a merge, not from what is executed. |
Comment on lines
+110
to
+113
| **Where a per-test mark disagrees with the module-level one**, the narrower tier | ||
| wins: a `tier_c` test inside a `tier_a` module is Tier C, and is excluded by | ||
| `-m "not tier_c"`. Say so in a comment next to the mark, so it does not read as | ||
| an oversight. |
Comment on lines
+144
to
+146
| Where a per-test mark disagrees with the module-level one, the narrower tier wins: | ||
| a `tier_c` test inside a `tier_a` module is tier C, and is excluded from anything | ||
| gating on `not tier_c`. |
Comment on lines
+119
to
+122
| # tier_c overrides the module-level tier_a for this test alone: it compares two | ||
| # transport managers, so it can fail because one of them got better. | ||
| @pytest.mark.level_2 | ||
| @pytest.mark.tier_c |
Comment on lines
+57
to
+59
| # tier_c overrides the module-level tier_a for this test alone: it compares two | ||
| # smoothers, so it can fail because one of them got better. | ||
| @pytest.mark.tier_c |
Comment on lines
+105
to
+108
| **Pytest markers**: | ||
| - `@pytest.mark.tier_c` | ||
| - `@pytest.mark.xfail(reason="Feature not yet implemented")` | ||
| - `@pytest.mark.skip(reason="Waiting for X to be fixed")` | ||
| - plus `@pytest.mark.xfail(reason=...)` / `@pytest.mark.skip(reason=...)` for C2 | ||
| where relevant |
Comment on lines
+84
to
+87
| print(f"low mode kept: taubin={taubin_low_kept:.3f} " | ||
| f"laplacian={laplacian_low_kept:.3f}") | ||
| assert taubin_low_kept > 0.97, ( | ||
| f"Taubin kept only {taubin_low_kept:.3f} of the low mode") |
Comment on lines
+256
to
+259
| # tier_c overrides the module-level tier_b: the second assertion compares two | ||
| # constraint methods, so it can fail because one of them got better. | ||
| @pytest.mark.level_2 | ||
| def test_freesurface_strong_constraint_beats_penalty(): | ||
| @pytest.mark.tier_c |
Comment on lines
+259
to
+260
| @pytest.mark.tier_c | ||
| def test_freesurface_strong_constraint_against_penalty(): |
Comment on lines
+132
to
+136
| The relationship is sensitive to the Courant number, the quadrature degree | ||
| and the element size, so it is a characterisation of this fixture | ||
| (cellSize=0.08, dt=0.1, 16 steps) rather than a general claim. Compare | ||
| `project_integration_point_proxy_pic_lip`, where the bulk diagnostics were | ||
| identical while the interface answer was not. |
…ntracts come back out Copilot review found the load-bearing mistake: pytest MERGES a module-level `pytestmark` with a function's own marks rather than overriding them. So a `tier_c` test inside a `tier_a` module carried BOTH, and `tier_a or tier_b` — which is exactly what `scripts/release_gate.py` asks for by default — still selected it. The precedence rule I documented was not implemented by anything, and the characterisations could still have gated a release. Fixed at the source rather than in the selector: the three files now carry the LEVEL on the module and the TIER on each test, so the tiers are mutually exclusive by construction. Verified on all 45 items in those files — `tier_c and (tier_a or tier_b)` selects nothing, `tier_a or tier_b or tier_c` selects all 45, and nothing is untiered. `test_freesurface_ring_quadrature_is_exact_in_parallel` had been relying on the module tier and gets `tier_b` explicitly. `release_gate.py` spells the exclusion out anyway. The convention above is a convention; if a module-level tier ever reappears next to a per-test `tier_c`, the failure mode is a characterisation silently gating a release. **Two contracts were stranded inside tier C**, which contradicted this PR's own description of what stays gating: - `test_1070` held `leaks["strong"] < 1e-3` — the no-net-flux contract — inside the comparison. Split into `test_freesurface_strong_constraint_passes_no_net_flux` (tier B), with a module-scoped fixture so both tests share one measurement rather than paying the four solve/advance steps twice. - `test_0773` held `taubin_low_kept > 0.97` at 40 iterations. That was an incidental extra assert, not a separate contract: the absolute preservation bound already exists in the tier A test at 80 iterations. Dropped, with the docstring saying where the contract lives. **The measured numbers are now measured.** The rule this PR adds requires dated values in the docstring, and I had written estimates. `test_0773` said Taubin keeps 0.992 against 0.836; it is 0.996 against 0.917. `test_1070` said datum error 2.4e-3 against 1.5e-1; it is 1.06e-2 against 3.01e-2. `test_0066` recorded no numbers at all and now carries L2 1.70e-3 against 3.96e-3, peak 0.9909 against 0.9696. **Stale tier C descriptions swept.** `tests/pytest.ini`, the marker table in TESTING-RELIABILITY-SYSTEM.md and the example in HOW-TO-WRITE-UW3-SCRIPTS.md all still called tier C "development only, not for automation", which contradicts a characterisation that runs and is reported. 44 passed, 1 skipped (needs --with-mpi) across the three files. 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
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
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.
Follow-on from #728. Three more tests asserted that one method is more accurate
than another:
test_0773_surface_smoother.pytaubin_low_kept > laplacian_low_kept + 0.03test_0066_integration_point_slcn.pyl2_ip <= l2_nodal,peak_ip >= peak_nodaltest_1070_free_surface_plume.pyerrors["strong"] < 0.5 * errors["penalty"]That shape encodes a preference rather than a contract — the result moves with
the fixture, the mesh, the forcing and every default the two methods carry — and
it can fail because the code got better, which is not a regression.
The contracts stay, and keep gating
Each test also carried a real absolute bound against a known answer, with no
rival method in it: the Taubin passband bounds, the integration-point L2 error,
the net volume flux through the free surface. Those are untouched.
test_0066gainstest_rotating_gaussian_ip_accuracyso its contract stands onits own rather than riding on the comparison.
The comparisons move to tier C
Per the maintainer ruling (2026-09-12), tier C is defined by what it does, not
by how immature it is: a tier C failure never blocks a change, it demands an
explanation. Each comparison now says so in its own failure message —
— and records its measured numbers and configuration, dated, in the docstring.
Charter §8
Adds the rule that produced this: assert against a known answer, not against a
rival method. Convergence order and mathematical exactness are contracts and
may be asserted freely; "method A scored better than method B here" may not.
Plus the tell that decides the tier — if an assertion would break when the code
gets better, it belongs at tier C — and a note that the number in a filename
sets a broad sequence, nothing more, so a shared number is not a conflict.
The governing doc
TESTING-RELIABILITY-SYSTEM.mdhad tier C as "Experimental … NEVER forautomated CI". That is one population, not the definition, and it contradicts a
characterisation that should run and be reported. Tier C is now defined by
not gating, with both populations named: C1 characterisation, which never
promotes, and C2 experimental, which promotes to B once the feature lands.
Both stay unusable as a basis for coding — the original point of the tiers.
Also records the precedence a per-test mark needs: the narrower tier wins, so a
tier_ctest inside atier_amodule is tier C and is excluded by-m "not tier_c".Verified
37 passed (0773, 0066); 6 passed, 1 skipped (1070).
-m tier_cselects exactlythe three characterisations;
-m "not tier_c"selects the other 41.Underworld development team with AI support from Claude Code