Nitsche free-slip tests: a wiring check, a real assert, and the comparison at tier C by lmoresi · Pull Request #728 · underworldcode/underworld3

lmoresi · 2026-09-11T04:56:18Z

Found by turning on tests/test_1060_nitsche_freeslip.py, which matched no glob
in scripts/test.sh and had never run in CI (#721). Two defects, both test-side.

An absolute threshold that tracked the forcing

test_nitsche_normal_velocity_zero asserted max|v_y| < 1e-4 and produced
1.0081e-04. The bound was absolute, so it scaled with the buoyancy forcing
rather than with anything about Nitsche: |v| peaks at 1.8e-2 in this fixture, so
it was ~5.7e-3 relative and sat a fraction of a percent from failing for reasons
unrelated to the method under test.

Nitsche is a weak constraint and does not drive v·n to machine precision —
documented behaviour, and the reason rotated strong free-slip exists
(subsystems/rotated-freeslip.md).
The solver is right, so the test is what changed. It is now a wiring check
with a deliberately loose relative bound and a docstring saying not to tighten
it: a tight number would describe this fixture, not the code.

A comparison test with no assertion

test_nitsche_better_than_penalty_constraint ended on

        # Nitsche at gamma=10 should be comparable or better than penalty at 1e4

with no assert. The file was truncated mid-write at 150 lines, so it computed
both numbers, printed them, and passed unconditionally.

What replaces it is a characterisation at tier C, not a contract. The
ordering it records is what the free-slip rulings rest on: an essential BC holds
v·n to machine precision, while Nitsche and penalty leave a finite leak.

That assertion can fail because the code got better — a weak path coming out
exact would fail it — and tier C is the right home for exactly that: it demands
an explanation, not a revert. The failure message says so outright:

a weak constraint reaching machine precision is GOOD NEWS and a change in
behaviour. Explain it and re-characterise; do not revert to make this pass.

It deliberately does not rank the two weak methods against each other. Which
leaks less is problem-dependent and moves with gamma and the penalty
coefficient; asserting it would be cheering for a method rather than testing the
code (maintainer ruling, 2026-09-10). The measured numbers — essential 0.0,
penalty 1.5e-3, Nitsche 5.7e-3 at res=8 — are recorded in the docstring as a
dated characterisation, not as an assertion.

Scope

No library code changed. Per Charter §8 the tests' own correctness was
established first, and the conclusion is that the tests were wrong. 5 passed
locally; the characterisation is independently selectable with -m tier_c.

Underworld development team with AI support from Claude Code

Copilot

🔵 Needs a closer look

The updated regression test is not wired into CI, so its assertions cannot catch regressions.

Pull request overview

This test-only PR updates Nitsche free-slip checks to validate documented relative leakage and strong-versus-weak constraint behavior.

Changes:

Replaces the absolute threshold with relative bounds.
Adds assertions for essential, penalty, and Nitsche paths.
Keeps scope limited to the test file.

File summaries

File	Summary
`tests/test_1060_nitsche_freeslip.py`	Corrects leakage validation and adds meaningful constraint assertions; the test is not currently exercised by CI.

Review details

Suppressed comments (1)

tests/test_1060_nitsche_freeslip.py:149

This regression guard is still not exercised by the repository's CI entry point: scripts/test.sh explicitly leaves test_106* unbatched (lines 126–128), and no earlier glob includes test_1060_nitsche_freeslip.py. As a result, the new bounds and assertions cannot catch a regression in CI; please wire this test into an approved batch or carry the explicit follow-up from #721 before relying on it as coverage.

Files reviewed: 1/1 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.

…rison at tier C `test_1060_nitsche_freeslip.py` matched no glob in `scripts/test.sh` (#721), so none of this had ever run in CI. **An absolute threshold that tracked the forcing.** `test_nitsche_normal_velocity_zero` asserted `max|v_y| < 1e-4` and produced 1.0081e-04. The bound was absolute, so it scaled with the buoyancy forcing rather than with anything about Nitsche — |v| peaks at 1.8e-2 here, so it was ~5.7e-3 relative and sat a fraction of a percent from failing for unrelated reasons. Nitsche is a weak constraint and does not drive v.n to machine precision; that is documented behaviour and the reason rotated strong free-slip exists. The solver is right, so the test is what changed: it is now a WIRING check with a deliberately loose relative bound and a docstring saying not to tighten it. **A comparison test with no assertion.** `test_nitsche_better_than_penalty_constraint` ended on the comment `# Nitsche at gamma=10 should be comparable or better than penalty at 1e4` with no `assert` — the file was truncated mid-write at 150 lines, so it computed both numbers, printed them, and passed unconditionally. What replaces it is a CHARACTERISATION at tier C, not a contract. The ordering it records is what the free-slip rulings rest on: an essential BC holds v.n to machine precision, while Nitsche and penalty leave a finite leak. That assertion can fail because the code got BETTER — a weak path coming out exact would fail it — and tier C is exactly the right home for a test like that: it demands an explanation, not a revert. The failure messages say so. It deliberately does NOT rank the two weak methods against each other. Which of them leaks less is problem-dependent, moves with gamma and the penalty coefficient, and asserting it would be cheering for a method rather than testing the code (maintainer ruling, 2026-09-10). The measured numbers are recorded in the docstring as a characterisation of this fixture, dated, not as a ranking. Both changes are test-side. No library code changed: per Charter §8 the tests' own correctness was established first. 5 passed locally; the characterisation is independently selectable with `-m tier_c`. Underworld development team with AI support from Claude Code

…quote $PYTEST **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

…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 · 2026-09-11T18:50:25Z

On the review point that this test is not exercised by CI — correct, and it is
the reason the defects survived. scripts/test.sh leaves test_106* unbatched,
so test_1060_nitsche_freeslip.py has never run there.

That is fixed in #731, which batches test_106*/test_107* and the
test_06NN band. This PR is deliberately the first of the chain: #731 enabling
test_106* while the <1e-4 bound is still wrong would simply turn CI red, so
the fix has to land first.

Merge order: #728 → #729 → #723 → #731.

…quote $PYTEST **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

…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

Copilot AI lite review requested due to automatic review settings September 11, 2026 04:56

Copilot started reviewing on behalf of lmoresi September 11, 2026 04:56 View session

lmoresi force-pushed the bugfix/nitsche-freeslip-test-assertions branch from d9f804c to d33b0bb Compare September 11, 2026 04:58

lmoresi changed the title ~~Nitsche free-slip tests: a bound the method actually holds, and an assert that exists~~ Nitsche free-slip tests: a wiring check instead of an accuracy claim, and an assert that exists Sep 11, 2026

Copilot AI reviewed Sep 11, 2026

View reviewed changes

lmoresi force-pushed the bugfix/nitsche-freeslip-test-assertions branch from d33b0bb to b74bd58 Compare September 11, 2026 15:36

lmoresi changed the title ~~Nitsche free-slip tests: a wiring check instead of an accuracy claim, and an assert that exists~~ Nitsche free-slip tests: a wiring check, a real assert, and the comparison at tier C Sep 11, 2026

This was referenced Sep 11, 2026

Tier C is what does not gate: the comparison tests, and the Charter wording for them #729

Merged

Stop excluding tests by band, let tier C report instead of gate, and quote $PYTEST #731

Open

lmoresi merged commit 9c4f27c into development Sep 11, 2026
2 checks passed

Navigation Menu

Uh oh!

Nitsche free-slip tests: a wiring check, a real assert, and the comparison at tier C - #728

Nitsche free-slip tests: a wiring check, a real assert, and the comparison at tier C#728
lmoresi merged 1 commit into
developmentfrom
bugfix/nitsche-freeslip-test-assertions

lmoresi commented Sep 11, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

lmoresi commented Sep 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

lmoresi commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

An absolute threshold that tracked the forcing

A comparison test with no assertion

Scope

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (1)

Uh oh!

lmoresi commented Sep 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lmoresi commented Sep 11, 2026 •

edited

Loading