A test file with no tests was switching the units system on at collection (#567) — and CI can now run in parallel

[lmoresi]

Fixes#567.

It was never the locator

The three failing tests are point-locator tests, the assertion message says
"answered in a cell that does not contain it", and the failure moves with the
xdist worker count. Every one of those signals points at the locator. None of
them is right. The locator, the DMInterpolationCache, mesh._index, the #560
rejection reach and the per-cell face control points are all exonerated — the
cache key is per-mesh and no key collides. evaluate returned exactly the
right value for the data it was given. The data was wrong, and it was
written by the test's own fixture.

The minimal reproducing pair

tests/test_0741_expression_arithmetic_units.py + tests/test_0761_point_locator.py

and it does not need xdist at all:

pytest --config-file=tests/pytest.ini \
tests/test_0741_expression_arithmetic_units.py \
tests/test_0761_point_locator.py \
-q -k test_p1_field_on_a_shared_edge
# 3 failed, 3 passed, 19 deselected

Every test in test_0741 is deselected there. In fact test_0741 contained no
tests at all — it was a converted script whose whole body ran at module level.
Merely collecting it is enough to break the locator suite.

The leaking state

test_0741 did this at import:

model=uw.Model()
model.set_reference_quantities(length=uw.quantity(2900, "km"),
time=uw.quantity(1, "Myr"))

That runs during pytest collection, and it switches the units system on for the
whole process. Importing every test module in turn says it is the only file
in the suite that mutates the global default model at import time.

conftest.isolate_test_state resets the default model before every test, so
this should be harmless. It is not, and the reason is fixture ordering: pytest
builds higher-scoped fixtures first, so test_0761's module-scoped
located_box fixture — which builds the mesh and the P1 variable the whole file
shares — is set up before the first test's function-scoped reset ever runs. A
function-scoped reset cannot protect a module-scoped fixture. Over thirty test
files have module- or session-scoped fixtures; the locator suite is simply the
one whose fixture reads coordinates.

With the units system active, var.coords returns dimensional coordinates.
Measured, on the unit box with a 2900 km length scale:

 field.coords min=0 max=2.9e+06 <- what the fixture read
dm coords min=0 max=1 <- what the mesh actually is

So field.data[:, 0] = _nodal_signal(field.coords) sampled a
sin(97x)cos(89y) signal at points 2.9 million times too far apart and wrote
the result as nodal values. The oracle then computed the closed form from the
DM's vertex coordinates. Two different fields, compared: O(1) disagreement at
every point, which is exactly the reported signature.

Isolating the step with a standalone probe:

configuration	worst error
clean	2.9143e-15
units active while the fixture builds, reset before evaluating (the pytest case)	1.6747e+00
units active throughout	1.6747e+00
reset before the fixture builds	2.9143e-15

1.6747e+00 is the number CI reported for [2d-0.5], to every digit.

Why the worker count decided it

The pollution only reaches a module-scoped fixture if no earlier test in that
process has already run — one function-scoped reset cleans it up for good. So
the failure needs test_0761 to be the first file in its worker. Serially it
never is (test_0500 runs first). Under --dist loadfile the worker count
decides the file grouping, and at 16 workers — and at 4, in CI — it lands first.
Confirmed directly: in the failing run test_0761 was the first file on gw0
and its first three tests failed.

That also disposes of the ruled-out list in the issue. It was never about a
preceding file, which is why running the obvious neighbours before it changed
nothing; the polluter acts at collection, before any file runs.

User-facing, or test-only?

Test-only.var.coords returning metres when the units system is on is the
documented contract, not a defect. Two consecutive evaluate calls on different
meshes cannot return each other's answers — no cache key collides. What was
wrong was a test fixture reading coordinates in one unit system and an oracle
reading them in another, and a test file that switched unit systems globally at
import.

The fix

Two commits.

tests/conftest.py — reset at module scope as well as function scope. This
is the durable half. A scope="module" autouse fixture is ordered before a
module's own fixtures, so every module-scoped fixture in the suite is now built
with a pristine default model regardless of what the process did earlier,
including at collection. The per-test reset stays as it was.

tests/test_0741_expression_arithmetic_units.py — stop running at import.
The four products it was checking (cm/year * Myr, spelled as quantities or as
expressions, in either order) are now five real assertions, and the reference
quantities live in a fixture that tears them down again. A test file with no
tests that mutates global state at import time is a hazard whatever else is
true.

Regression test

tests/test_0742_module_fixture_units_isolation.py runs a real pytest in a
subprocess against a copy of the live conftest.py. The generated module
pollutes at import exactly as test_0741 did, and its module-scoped fixture
asserts that a unit-box variable's coordinates reach 1.0.

Negative control, with the module-scoped reset deleted from conftest.py:

Obtained: 2900000.0
Expected: 1.0 ± 1.0e-06
FAILED test_leaking_module.py::test_the_fixture_saw_the_undimensionalised_unit_box

It fails for the right reason and passes with the guard in place. Deleting the
guard is the only way to make it fail, which is what pins the fix rather than
the symptom.

Verification

The original reproduction, CI's own batch:

run	before	after
tests/test_05*py tests/test_07*py -n 4 --dist loadfile	3 failed, 529 passed	538 passed, 5 skipped, 9 xfailed, 1 xpassed (61s)

Level 1, the whole selection from the issue:

workers	result	time
16	1491 passed, 31 skipped, 2 xfailed	2:25
8	1491 passed, 31 skipped, 2 xfailed	2:14
4	1491 passed, 31 skipped, 2 xfailed	3:25

The three named tests are green serially and at 4, 8 and 16 workers; the full
test_0761 file is green serially (4.4s) and at 4 (6.0s), 8 (6.1s) and 16
(7.2s) workers.

scripts/test_levels.sh 1 (the developer loop #566 added): 1491 passed, 31
skipped, 2 xfailed in 2:22, exit 0.

scripts/test.sh --p 2 end to end, both configurations green with zero
failures:

	serial batches	MPI batches	end to end
in-process (today's CI)	25.1 min	300s	28:04
WORKERS=8	9.6 min	294s	12:21

2.6x on the serial batches, 2.3x end to end. The MPI batches are untouched —
they run their own mpirun.

CI parallelism is now safe to enable, and the third commit enables it

scripts/test.sh and the workflow both carried a comment saying CI stays
in-process until #567 lands and that setting WORKERS in the workflow is the
whole remaining change. It has landed, so the third commit is that change:
WORKERS: 4 in build_uw3_and_test.yaml (the runner's vCPU count, a literal —
an earlier attempt to detect it inside the job got 1 and ran xdist with a single
worker, paying a spawn and a fresh underworld3 import per batch to parallelise
nothing), plus the stale comment blocks rewritten.

It is a separate commit deliberately. Drop it if you would rather watch a green
CI run on the fix alone first; the other two commits stand on their own.

Related: #572 raised the job cap to 120 min as a stop-gap, #573 tracks the
durable fix, #551 is the locator umbrella and #556 added the tests that caught
this.

Underworld development team with AI support from Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.