From fa2a5d484e5e8c885fa5ce7b6c068f0da205e048 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Sun, 16 Aug 2026 21:24:04 +1000 Subject: [PATCH 1/3] Test modules must not build state while being collected pytest imports a module in order to collect it, so module-level code runs before any test, any fixture, and the isolation in tests/conftest.py. That is how #567 (units switched on at import) and #505 (two Stokes solves at import) reached development. test_0050_utils.py, test_0120_data_property_access.py and test_0130_field_creation.py were converted scripts with no test function in them: 0120 and 0130 printed their exceptions instead of raising, so they could not fail, and 0050's only assertion was disabled by its name. Each is now fixtures and tests over its original subject. The auditor check is a delta rather than uw_object_count == 7, which is only true in a fresh process running that file alone. conftest.py fingerprints the uw_object count, the active model's reference quantities and the strict-units flag around each module's collection, and fails the run naming the module and what moved. Eighteen modules already do this and are exempted by name (#587); the list is a ratchet, so nothing new lands. test_0051_collection_state_guard.py runs pytest on an offending module and on a clean one. scripts/test.sh no longer comments out test_0050. Closes #505, #575. Underworld development team with AI support from Claude Code --- scripts/test.sh | 2 +- tests/conftest.py | 123 +++++++++++++++ tests/test_0050_utils.py | 181 ++++++++++------------ tests/test_0051_collection_state_guard.py | 64 ++++++++ tests/test_0120_data_property_access.py | 73 +++++---- tests/test_0130_field_creation.py | 79 +++++----- 6 files changed, 357 insertions(+), 165 deletions(-) create mode 100644 tests/test_0051_collection_state_guard.py diff --git a/scripts/test.sh b/scripts/test.sh index c60bd39dd..93a69edfd 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -83,7 +83,7 @@ if [ $PARALLEL_ONLY -eq 0 ]; then # 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 # disable auditor test for now + $PYTEST tests/test_0050*py || status=1 $PYTEST tests/test_005[1-9]*py tests/test_006[0-1]*py || status=1 $PYTEST tests/test_01*py || status=1 $PYTEST tests/test_02*py || status=1 diff --git a/tests/conftest.py b/tests/conftest.py index 1485a36a5..e9a0928e7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -47,6 +47,129 @@ os.environ.setdefault("UW_MESH_CACHE_DIR", f".meshes/{_xdist_worker}") +# ============================================================================== +# COLLECTION-TIME GLOBAL STATE GUARD (#575) +# ============================================================================== +# pytest imports a test module in order to collect it, so anything a module does +# at import time runs BEFORE any test, any fixture and any isolation the +# fixtures below provide. Two defects have reached `development` that way: +# +# #567 a module switched the units system on process-wide at import, and the +# first module-scoped fixture in that worker built its mesh under +# dimensional coordinates; +# #505 a module ran two Stokes solves at import, so `--collect-only` sat +# inside SNESSolve for 20+ minutes looking like a silent death. +# +# The fixtures below fix the consequence. This guard fixes the practice: it +# fingerprints the process state around each module's import and fails the run, +# naming the module and what moved, when a module writes to it. +# +# The check is skipped when underworld3 is not importable — conftest.py is +# loaded before the package is necessarily installed in CI. + + +def _global_state_fingerprint(): + """Process-global state that importing a test module must leave alone.""" + + try: + import underworld3 as uw + from underworld3 import model as _model + from underworld3.utilities._api_tools import uw_object + except ImportError: + return None + + active_model = _model._default_model + reference_quantities = () + if active_model is not None: + reference_quantities = tuple( + sorted(getattr(active_model, "_reference_quantities", None) or {}) + ) + + return { + "uw objects created": uw_object.uw_object_counter(), + "units reference quantities": reference_quantities, + "strict units": uw.is_strict_units_active(), + } + + +# Modules that already do this, measured on `development` at the time the guard +# was written. They are exempted so the guard can be turned on today; the list +# is a ratchet, not an approval — nothing may be added to it, and each entry is +# a module whose module-level work belongs in a fixture (#587). +# +# `test_0601_mesh_vector_calc.py` is the one to fix first: alone in this list it +# moves the units state (`use_strict_units(False)` at import), which is the #567 +# mechanism itself and reaches every module collected after it. +_KNOWN_COLLECTION_TIME_WORK = ( + "parallel/test_0765_internal_boundary_integral_mpi.py", + "test_0004_pointwise_fns.py", + "test_0005_IndexSwarmVariable.py", + "test_0501_integrals.py", + "test_0502_boundary_integrals.py", + "test_0504_projections.py", + "test_0601_mesh_vector_calc.py", + "test_0810_amr_swarm_migration_regression.py", + "test_0830_mesh_adapt_variable_transfer.py", + "test_1000_poissonCart.py", + "test_1000_poissonNaturalBC.py", + "test_1001_poissonSph.py", + "test_1004_DarcyCartesian.py", + "test_1010_stokesCart.py", + "test_1011_stokesSph.py", + "test_1014_stokes_multigrid.py", + "test_1014_stokes_shell_nullspace.py", + "test_1050_VEstokesCart.py", +) + + +def _is_known_offender(nodeid): + path = nodeid.replace(os.sep, "/") + return any(path.endswith(known) for known in _KNOWN_COLLECTION_TIME_WORK) + + +_state_before_collect = {} +_collection_offenders = [] + + +def pytest_collectstart(collector): + if isinstance(collector, pytest.Module): + _state_before_collect[collector.nodeid] = _global_state_fingerprint() + + +def pytest_collectreport(report): + before = _state_before_collect.pop(report.nodeid, None) + if before is None: + return + + after = _global_state_fingerprint() + if after is None: + return + + moved = {k: (before[k], after[k]) for k in before if before[k] != after[k]} + if moved and not _is_known_offender(report.nodeid): + _collection_offenders.append((report.nodeid, moved)) + + +def pytest_collection_finish(session): + if not _collection_offenders: + return + + lines = [ + "Test modules changed global state while being COLLECTED.", + "", + "Work belongs inside a test function or a fixture. Code at module level", + "runs at import, before the isolation fixtures in tests/conftest.py can", + "act, and it runs even under --collect-only (issues #567, #505).", + "", + ] + for nodeid, moved in _collection_offenders: + lines.append(f" {nodeid}") + for key, (before, after) in moved.items(): + lines.append(f" {key}: {before} -> {after}") + + raise pytest.UsageError("\n".join(lines)) + + @pytest.fixture(scope="module", autouse=True) def isolate_module_state(): """Reset the global model BEFORE a module's own fixtures are built. diff --git a/tests/test_0050_utils.py b/tests/test_0050_utils.py index 15693943b..69a17ded1 100644 --- a/tests/test_0050_utils.py +++ b/tests/test_0050_utils.py @@ -1,126 +1,103 @@ -import pytest - -# All tests in this module are quick core tests -pytestmark = pytest.mark.level_1 -# --- -# jupyter: -# jupytext: -# formats: ipynb,py:percent -# text_representation: -# extension: .py -# format_name: percent -# format_version: '1.3' -# jupytext_version: 1.16.2 -# kernelspec: -# display_name: Python 3 (ipykernel) -# language: python -# name: python3 -# --- - -## %% - -import underworld3 as uw -import sympy - -mesh = uw.meshing.StructuredQuadBox(elementRes=(5,) * 2) -x, y = mesh.X - -# %% -v = uw.discretisation.MeshVariable(r"mathbf{u}", mesh, mesh.dim, vtype=uw.VarType.VECTOR, degree=2) -p = uw.discretisation.MeshVariable(r"mathbf{p}", mesh, 1, vtype=uw.VarType.SCALAR, degree=1) - +"""The installation auditor, and a second Stokes solver over fields the first already owns. -def bc_1(solver): - s1 = solver - s1.add_dirichlet_bc((0.0, 0.0), "Bottom") - s1.add_dirichlet_bc((y, 0.0), "Top") +Both subjects used to sit at module level, so importing this file built a mesh, +two solvers and solved both — during pytest COLLECTION, before any test ran. +A `--collect-only` run was measured 20+ minutes inside `SNESSolve`, which to the +caller is indistinguishable from pytest dying silently (#505). The auditor +assertion was disabled at the same time by the name `dont_test_auditor`, so the +file did all of that work and checked nothing. - s1.add_dirichlet_bc((sympy.oo, 0.0), "Left") - s1.add_dirichlet_bc((sympy.oo, 0.0), "Right") +The auditor check is now written as a delta rather than the absolute +`uw_object_count == 7` it used to assert. The counter is process-wide and +monotonic, so an absolute count is only true in a fresh process running this +file alone — which is why the assertion could not survive being enabled. +""" +import pytest +import sympy +import numpy as np -def bc_2(solver): - s1 = solver - s1.add_dirichlet_bc((0.0, sympy.oo), "Bottom") - s1.add_dirichlet_bc((0.0, sympy.oo), "Top") +import underworld3 as uw - s1.add_dirichlet_bc((0.0, 0.0), "Left") - s1.add_dirichlet_bc((0.0, x), "Right") +pytestmark = pytest.mark.level_1 -# %% -def vis_model(mesh): - import pyvista as pv - import underworld3.visualisation as vis +@pytest.fixture(scope="module") +def mesh(): + return uw.meshing.StructuredQuadBox(elementRes=(5,) * 2) - v = mesh.vars["mathbfu"] - pl = pv.Plotter(window_size=(1000, 750)) - pvmesh = vis.mesh_to_pv_mesh(mesh) - pvmesh.point_data["V"] = vis.vector_fn_to_pv_points(pvmesh, v.sym) - pvmesh.point_data["Vmag"] = vis.scalar_fn_to_pv_points(pvmesh, sympy.sqrt(v.sym.dot(v.sym))) - pvmesh.point_data["V1"] = vis.scalar_fn_to_pv_points(pvmesh, v.sym[1]) +def test_auditor_reads_the_whole_installation(mesh): + """Every installation field the auditor advertises is populated. - pl.add_mesh( - pvmesh, - cmap="coolwarm", - edge_color="Black", - show_edges=True, - scalars="Vmag", - use_transparency=False, - opacity=1.0, - ) + The auditor sets a field to None and warns when it cannot import the + package behind it, so a None here is a real gap in what we can report. + """ - velocity_points = vis.meshVariable_to_pv_cloud(v) - velocity_points.point_data["V"] = vis.vector_fn_to_pv_points(velocity_points, v.sym) - arrows = pl.add_arrows( - velocity_points.points, - velocity_points.point_data["V"], - mag=3e-1, - opacity=0.5, - show_scalar_bar=False, - cmap="coolwarm", - ) - - pl.show(cpos="xy") + unreadable = [k for k, v in uw.auditor.get_installation_data.items() if v is None] + assert not unreadable, f"auditor could not read: {unreadable}" -# %% -stokes = uw.systems.Stokes(mesh, velocityField=v, pressureField=p) -stokes.constitutive_model = uw.constitutive_models.ViscousFlowModel -stokes.constitutive_model.Parameters.shear_viscosity_0 = 1 -# %% -bc_1(stokes) +def test_auditor_counts_objects_as_they_are_created(mesh): + """The runtime count advances when objects are built, and only then.""" -# %% -stokes.solve() + before = uw.auditor.get_runtime_data["uw_object_count"] -# %% -# vis_model(mesh) + scratch = uw.meshing.StructuredQuadBox(elementRes=(2, 2)) + uw.discretisation.MeshVariable("audited", scratch, 1, degree=1) -# %% -s1 = uw.systems.Stokes(mesh, velocityField=v, pressureField=p) -s1.constitutive_model = uw.constitutive_models.ViscousFlowModel -s1.constitutive_model.Parameters.shear_viscosity_0 = 1 -# stokes._rebuild_after_mesh_update() -bc_2(s1) + after = uw.auditor.get_runtime_data["uw_object_count"] -# %% -# stokes.solve() -s1.solve() + # At least the two objects named above; the constructors may build more. + assert after - before >= 2 -# %% -# vis_model(mesh) + # The control: reading the auditor is not itself what moves the count. + assert uw.auditor.get_runtime_data["uw_object_count"] == after -def dont_test_auditor(): - # assert not values are in install data are None - for v in uw.auditor.get_installation_data.values(): - assert v is not None +def test_second_solver_over_the_same_fields(mesh): + """Two Stokes solvers share one velocity/pressure pair and give different flows. - # assert 7 uw_objects are created - assert uw.auditor.get_runtime_data.get("uw_object_count") == 7 + `sympy.oo` in a component slot leaves that component unconstrained, so the + two boundary condition sets below constrain different components on + different walls. The assertion is that the second solve reaches its own + answer rather than returning the first solver's field: the two are set up + over the same `MeshVariable`s, which is the situation where a stale + setup would go unnoticed. + """ + x, y = mesh.X + v = uw.discretisation.MeshVariable( + r"mathbf{u}", mesh, mesh.dim, vtype=uw.VarType.VECTOR, degree=2 + ) + p = uw.discretisation.MeshVariable( + r"mathbf{p}", mesh, 1, vtype=uw.VarType.SCALAR, degree=1 + ) -# %% + first = uw.systems.Stokes(mesh, velocityField=v, pressureField=p) + first.constitutive_model = uw.constitutive_models.ViscousFlowModel + first.constitutive_model.Parameters.shear_viscosity_0 = 1 + first.add_dirichlet_bc((0.0, 0.0), "Bottom") + first.add_dirichlet_bc((y, 0.0), "Top") + first.add_dirichlet_bc((sympy.oo, 0.0), "Left") + first.add_dirichlet_bc((sympy.oo, 0.0), "Right") + first.solve() + + driven_from_the_top = v.data.copy() + + second = uw.systems.Stokes(mesh, velocityField=v, pressureField=p) + second.constitutive_model = uw.constitutive_models.ViscousFlowModel + second.constitutive_model.Parameters.shear_viscosity_0 = 1 + second.add_dirichlet_bc((0.0, sympy.oo), "Bottom") + second.add_dirichlet_bc((0.0, sympy.oo), "Top") + second.add_dirichlet_bc((0.0, 0.0), "Left") + second.add_dirichlet_bc((0.0, x), "Right") + second.solve() + + driven_from_the_side = v.data.copy() + + assert np.isfinite(driven_from_the_top).all() + assert np.isfinite(driven_from_the_side).all() + assert abs(driven_from_the_top).max() > 0.0 + assert not np.allclose(driven_from_the_top, driven_from_the_side) diff --git a/tests/test_0051_collection_state_guard.py b/tests/test_0051_collection_state_guard.py new file mode 100644 index 000000000..c76361496 --- /dev/null +++ b/tests/test_0051_collection_state_guard.py @@ -0,0 +1,64 @@ +"""The collection-time global-state guard in `tests/conftest.py` (#575). + +The guard's whole value is that it fires, so it is tested by running pytest on a +module that offends and on one that does not. Without the second run the first +proves only that something failed. + +The sub-run loads the guard's hooks out of the real `tests/conftest.py` by path, +so this tests the shipped hooks rather than a copy of them. +""" + +import pathlib + +import pytest + +pytest_plugins = ["pytester"] + +pytestmark = pytest.mark.level_1 + +_CONFTEST = pathlib.Path(__file__).parent / "conftest.py" + +# The sub-run gets its own rootdir, so it does not inherit our conftest. Load it +# by path under a name of its own: `from conftest import *` would find the +# sub-run's own half-initialised `conftest` module in sys.modules and import +# nothing, which reads as the guard staying silent. +_SUB_CONFTEST = f""" +import importlib.util + +spec = importlib.util.spec_from_file_location("uw_collection_guard", {str(_CONFTEST)!r}) +guard = importlib.util.module_from_spec(spec) +spec.loader.exec_module(guard) + +pytest_collectstart = guard.pytest_collectstart +pytest_collectreport = guard.pytest_collectreport +pytest_collection_finish = guard.pytest_collection_finish +""" + + +def test_guard_fails_the_run_on_module_level_work(pytester): + """A mesh built at module level is reported, with the module named.""" + + pytester.makeconftest(_SUB_CONFTEST) + pytester.makepyfile( + test_offender="import underworld3 as uw\n" + "mesh = uw.meshing.StructuredQuadBox(elementRes=(2, 2))\n" + "def test_x():\n assert True\n" + ) + + result = pytester.runpytest_subprocess("--collect-only") + + assert result.ret != 0 + result.stderr.fnmatch_lines( + ["*changed global state while being COLLECTED*", "*test_offender.py*"] + ) + + +def test_guard_is_silent_when_nothing_is_built(pytester): + """The control: a module that only defines a test collects cleanly.""" + + pytester.makeconftest(_SUB_CONFTEST) + pytester.makepyfile(test_clean="def test_x():\n assert True\n") + + result = pytester.runpytest_subprocess("--collect-only") + + assert result.ret == 0 diff --git a/tests/test_0120_data_property_access.py b/tests/test_0120_data_property_access.py index 9fe81ce51..2fdb3d2b8 100644 --- a/tests/test_0120_data_property_access.py +++ b/tests/test_0120_data_property_access.py @@ -1,38 +1,59 @@ -import pytest +"""Writing a swarm variable through `.data`, twice, and reading back what was written. -# All tests in this module are quick core tests -pytestmark = pytest.mark.level_1 -import underworld3 as uw +This file used to be a converted debug script: the mesh, swarm and writes ran +at module level during pytest COLLECTION, and every statement was wrapped in a +`try/except` that printed the exception. It could not fail — a broken `.data` +property printed a cross and the run stayed green. +""" + +import pytest import numpy as np -# Quick test to see if field access is working +import underworld3 as uw from underworld3.meshing import UnstructuredSimplexBox -mesh = UnstructuredSimplexBox( - minCoords=(0.0, 0.0), - maxCoords=(1.0, 1.0), - cellSize=1.0 / 8.0, -) +pytestmark = pytest.mark.level_1 + + +@pytest.fixture(scope="module") +def populated_swarm(): + mesh = UnstructuredSimplexBox( + minCoords=(0.0, 0.0), + maxCoords=(1.0, 1.0), + cellSize=1.0 / 8.0, + ) + swarm = uw.swarm.Swarm(mesh=mesh) + values = uw.swarm.SwarmVariable("test", swarm, 1, proxy_degree=1) + swarm.populate(fill_param=2) + + return swarm, values + + +def test_data_property_writes_and_reads_back(populated_swarm): + """A write through `.data` is visible on the next read, and is the value written.""" + + swarm, values = populated_swarm + coords = swarm._particle_coordinates.data + + expected = np.cos(np.pi * coords[:, 0]) + values.data[:, 0] = expected -swarm = uw.swarm.Swarm(mesh=mesh) -s_values = uw.swarm.SwarmVariable("test", swarm, 1, proxy_degree=1) + assert np.allclose(values.data[:, 0], expected) -swarm.populate(fill_param=2) -print("Testing data property access...") -try: - # This should trigger the data property - s_values.data[:, 0] = np.cos(np.pi * swarm._particle_coordinates.data[:, 0]) - print("✓ Data property access successful") +def test_second_write_replaces_the_first(populated_swarm): + """The second write is not served a cached copy of the first. - # Try accessing again to test caching - s_values.data[:, 0] = np.sin(np.pi * swarm._particle_coordinates.data[:, 1]) - print("✓ Second data property access successful") + The cache is the point of the test: `.data` hands out a view whose validity + is tracked, and a stale view would return the cosine below after the sine + has been written. + """ - print("Field access working correctly!") + swarm, values = populated_swarm + coords = swarm._particle_coordinates.data -except Exception as e: - print(f"✗ Error with data property: {e}") - import traceback + values.data[:, 0] = np.cos(np.pi * coords[:, 0]) + replacement = np.sin(np.pi * coords[:, 1]) + values.data[:, 0] = replacement - traceback.print_exc() + assert np.allclose(values.data[:, 0], replacement) diff --git a/tests/test_0130_field_creation.py b/tests/test_0130_field_creation.py index cad1850b1..a31ca0afe 100644 --- a/tests/test_0130_field_creation.py +++ b/tests/test_0130_field_creation.py @@ -1,45 +1,52 @@ -import pytest +"""Successive mesh variables on one mesh get distinct field ids, and `.array` is reachable. -# All tests in this module are quick core tests -pytestmark = pytest.mark.level_1 -#!/usr/bin/env python3 +This file used to be a converted debug script: the mesh and the three variables +were created at module level during pytest COLLECTION, and each step sat inside +a `try/except` that printed the exception. A duplicate field id or an +unreachable `.array` printed a cross and the run stayed green. +""" + +import pytest import underworld3 as uw -import numpy as np from underworld3.meshing import UnstructuredSimplexBox -# Create a simple test case to debug the field ID issue -print("Creating mesh...") -mesh = UnstructuredSimplexBox(minCoords=(0.0, 0.0), maxCoords=(1.0, 1.0), cellSize=0.2) +pytestmark = pytest.mark.level_1 -print("Creating first variable...") -try: - u = uw.discretisation.MeshVariable("u", mesh, 2, vtype=uw.VarType.VECTOR, degree=2) - print(f"✓ Variable u created successfully with field_id={u.field_id}") -except Exception as e: - print(f"✗ Failed to create variable u: {e}") -print("Creating second variable...") -try: - p = uw.discretisation.MeshVariable("p", mesh, 1, vtype=uw.VarType.SCALAR, degree=1) - print(f"✓ Variable p created successfully with field_id={p.field_id}") -except Exception as e: - print(f"✗ Failed to create variable p: {e}") +@pytest.fixture(scope="module") +def mesh(): + return UnstructuredSimplexBox( + minCoords=(0.0, 0.0), maxCoords=(1.0, 1.0), cellSize=0.2 + ) + + +def test_successive_variables_get_distinct_field_ids(mesh): + """Three variables of mixed rank and degree, three different field ids. -print("Creating third variable...") -try: + A repeated id is the failure this file was written to catch: the second + variable would then address the first one's DOFs. + """ + + u = uw.discretisation.MeshVariable("u", mesh, 2, vtype=uw.VarType.VECTOR, degree=2) + p = uw.discretisation.MeshVariable("p", mesh, 1, vtype=uw.VarType.SCALAR, degree=1) s = uw.discretisation.MeshVariable("s", mesh, 1, vtype=uw.VarType.SCALAR, degree=1) - print(f"✓ Variable s created successfully with field_id={s.field_id}") -except Exception as e: - print(f"✗ Failed to create variable s: {e}") - -print("Testing array access...") -try: - print("Accessing s.array...") - s_array = s.array - print("✓ s.array access successful") -except Exception as e: - print(f"✗ Failed to access s.array: {e}") - import traceback - - traceback.print_exc() + + ids = [u.field_id, p.field_id, s.field_id] + + assert len(set(ids)) == 3, f"field ids collide: {ids}" + + +def test_array_is_reachable_and_shaped_by_the_variable(mesh): + """`.array` returns storage matching the variable's own component count.""" + + scalar = uw.discretisation.MeshVariable( + "s_array", mesh, 1, vtype=uw.VarType.SCALAR, degree=1 + ) + vector = uw.discretisation.MeshVariable( + "v_array", mesh, 2, vtype=uw.VarType.VECTOR, degree=2 + ) + + assert scalar.array.shape[-1] == 1 + assert vector.array.shape[-1] == mesh.dim + assert scalar.array.shape[0] == scalar.coords.shape[0] From 30e414bbc67c83abd727dcc3175798250fc32eb1 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Sun, 16 Aug 2026 21:29:19 +1000 Subject: [PATCH 2/3] The collection guard has an off switch for harnesses that generate an offender test_0742 copies the live conftest into a pytester sub-run together with a module that leaks units at import, because what it pins is that the module-scoped reset survives exactly that. The guard refused that module and ended the sub-run before it reached its assertion. UW_TEST_COLLECTION_GUARD=off turns the guard off for such a run. test_0742 sets it for its sub-run only, and test_0051 covers both settings, so the off switch cannot silently become the default. Underworld development team with AI support from Claude Code --- tests/conftest.py | 16 ++++++++++++++- tests/test_0051_collection_state_guard.py | 20 +++++++++++++++++++ ...est_0742_module_fixture_units_isolation.py | 8 +++++++- 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index e9a0928e7..64c0a11df 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -66,6 +66,20 @@ # # The check is skipped when underworld3 is not importable — conftest.py is # loaded before the package is necessarily installed in CI. +# +# `UW_TEST_COLLECTION_GUARD=off` turns it off. That exists for harnesses which +# GENERATE an offending module on purpose: `test_0742` copies this conftest into +# a pytester sub-run together with a module that leaks units at import, because +# what it pins is that the module-scoped reset survives exactly that. The guard +# firing there is correct and would stop the test reaching its assertion. + + +_GUARD_ENABLED = os.environ.get("UW_TEST_COLLECTION_GUARD", "on").lower() not in ( + "0", + "off", + "false", + "no", +) def _global_state_fingerprint(): @@ -132,7 +146,7 @@ def _is_known_offender(nodeid): def pytest_collectstart(collector): - if isinstance(collector, pytest.Module): + if _GUARD_ENABLED and isinstance(collector, pytest.Module): _state_before_collect[collector.nodeid] = _global_state_fingerprint() diff --git a/tests/test_0051_collection_state_guard.py b/tests/test_0051_collection_state_guard.py index c76361496..2bcce84ee 100644 --- a/tests/test_0051_collection_state_guard.py +++ b/tests/test_0051_collection_state_guard.py @@ -53,6 +53,26 @@ def test_guard_fails_the_run_on_module_level_work(pytester): ) +def test_guard_can_be_turned_off_for_a_generated_offender(pytester, monkeypatch): + """`UW_TEST_COLLECTION_GUARD=off` lets the same module through. + + `test_0742` needs this: it generates a module that leaks units at import, + because what it pins is that the module-scoped reset survives exactly that. + """ + + monkeypatch.setenv("UW_TEST_COLLECTION_GUARD", "off") + pytester.makeconftest(_SUB_CONFTEST) + pytester.makepyfile( + test_offender="import underworld3 as uw\n" + "mesh = uw.meshing.StructuredQuadBox(elementRes=(2, 2))\n" + "def test_x():\n assert True\n" + ) + + result = pytester.runpytest_subprocess("--collect-only") + + assert result.ret == 0 + + def test_guard_is_silent_when_nothing_is_built(pytester): """The control: a module that only defines a test collects cleanly.""" diff --git a/tests/test_0742_module_fixture_units_isolation.py b/tests/test_0742_module_fixture_units_isolation.py index 9652ebf65..004b92960 100644 --- a/tests/test_0742_module_fixture_units_isolation.py +++ b/tests/test_0742_module_fixture_units_isolation.py @@ -67,7 +67,13 @@ def test_the_fixture_saw_the_undimensionalised_unit_box(coordinate_extent): ''' -def test_a_module_scoped_fixture_is_not_built_under_leaked_units(pytester): +def test_a_module_scoped_fixture_is_not_built_under_leaked_units(pytester, monkeypatch): + # The live conftest carries the collection-time guard (#575), which would + # correctly refuse the leaking module below and end the sub-run before it + # reached its assertion. The leak is this test's fixture, so the guard is + # turned off for the sub-run only. + monkeypatch.setenv("UW_TEST_COLLECTION_GUARD", "off") + pytester.makeconftest(_LIVE_CONFTEST) pytester.makepyfile(test_leaking_module=_LEAKING_MODULE) From 18e3833e8ee40549fb5c8ce68d2d440ca51f8ae9 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Sun, 16 Aug 2026 21:35:28 +1000 Subject: [PATCH 3/3] Report the collection offence as a collection error, not a session abort Aborting from pytest_collection_finish leaves an xdist worker part-collected. The controller then reports INTERNALERROR> assert not crashitem, (crashitem, node) with a return code of 3 and nothing a reader can act on, which is the mode CI runs in. pytest.exit(returncode=4) does the same. The check moves into a pytest_make_collect_report wrapper, which sees the report before it is dispatched, and marks the offending module's collection failed with the fingerprint difference as its message. Measured on a module that builds a mesh at import: serial RC=2 ERROR tests/test_offender.py xdist -n 2 RC=1 10 passed, 1 error clean tree RC=0 both runners test_0051 covers the distributed case, and asserts INTERNALERROR is absent rather than only that the run failed. Underworld development team with AI support from Claude Code --- tests/conftest.py | 49 ++++++++++-------- tests/test_0051_collection_state_guard.py | 60 ++++++++++++++--------- 2 files changed, 66 insertions(+), 43 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 64c0a11df..d104c7f10 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -141,17 +141,17 @@ def _is_known_offender(nodeid): return any(path.endswith(known) for known in _KNOWN_COLLECTION_TIME_WORK) -_state_before_collect = {} -_collection_offenders = [] +@pytest.hookimpl(hookwrapper=True) +def pytest_make_collect_report(collector): + """Fingerprint the process around a module's import, which is its collection.""" + if not (_GUARD_ENABLED and isinstance(collector, pytest.Module)): + yield + return -def pytest_collectstart(collector): - if _GUARD_ENABLED and isinstance(collector, pytest.Module): - _state_before_collect[collector.nodeid] = _global_state_fingerprint() - + before = _global_state_fingerprint() + outcome = yield -def pytest_collectreport(report): - before = _state_before_collect.pop(report.nodeid, None) if before is None: return @@ -160,28 +160,35 @@ def pytest_collectreport(report): return moved = {k: (before[k], after[k]) for k in before if before[k] != after[k]} - if moved and not _is_known_offender(report.nodeid): - _collection_offenders.append((report.nodeid, moved)) - + if not moved or _is_known_offender(collector.nodeid): + return -def pytest_collection_finish(session): - if not _collection_offenders: + report = outcome.get_result() + if report.failed: return + # Reported as a COLLECTION ERROR against the offending module rather than by + # aborting the session. An abort (`pytest.exit`, or raising from + # `pytest_collection_finish`) leaves an xdist worker part-collected, and the + # controller then reports `INTERNALERROR ... assert not crashitem` instead of + # anything a reader can act on. A collection error is a state both the serial + # and the distributed runner already know how to carry. lines = [ - "Test modules changed global state while being COLLECTED.", + f"{collector.nodeid} changed global state while being COLLECTED:", + "", + ] + for key, (was, now) in moved.items(): + lines.append(f" {key}: {was} -> {now}") + lines += [ "", "Work belongs inside a test function or a fixture. Code at module level", "runs at import, before the isolation fixtures in tests/conftest.py can", - "act, and it runs even under --collect-only (issues #567, #505).", - "", + "act, and it runs even under --collect-only (issues #567, #505, #587).", ] - for nodeid, moved in _collection_offenders: - lines.append(f" {nodeid}") - for key, (before, after) in moved.items(): - lines.append(f" {key}: {before} -> {after}") - raise pytest.UsageError("\n".join(lines)) + report.outcome = "failed" + report.longrepr = "\n".join(lines) + report.result = [] @pytest.fixture(scope="module", autouse=True) diff --git a/tests/test_0051_collection_state_guard.py b/tests/test_0051_collection_state_guard.py index 2bcce84ee..99b96104b 100644 --- a/tests/test_0051_collection_state_guard.py +++ b/tests/test_0051_collection_state_guard.py @@ -4,7 +4,14 @@ module that offends and on one that does not. Without the second run the first proves only that something failed. -The sub-run loads the guard's hooks out of the real `tests/conftest.py` by path, +The distributed case has its own test because it is what CI runs and because the +guard failed it in its first form: aborting the session from +`pytest_collection_finish` left an xdist worker part-collected, and the +controller reported `INTERNALERROR ... assert not crashitem`. Reporting a +collection error against the offending module instead is carried by both +runners. + +The sub-runs load the guard's hooks out of the real `tests/conftest.py` by path, so this tests the shipped hooks rather than a copy of them. """ @@ -29,30 +36,43 @@ guard = importlib.util.module_from_spec(spec) spec.loader.exec_module(guard) -pytest_collectstart = guard.pytest_collectstart -pytest_collectreport = guard.pytest_collectreport -pytest_collection_finish = guard.pytest_collection_finish +pytest_make_collect_report = guard.pytest_make_collect_report """ +_OFFENDER = ( + "import underworld3 as uw\n" + "mesh = uw.meshing.StructuredQuadBox(elementRes=(2, 2))\n" + "def test_x():\n assert True\n" +) -def test_guard_fails_the_run_on_module_level_work(pytester): - """A mesh built at module level is reported, with the module named.""" + +def test_guard_reports_module_level_work_as_a_collection_error(pytester): + """A mesh built at module level fails the run, with the module named.""" pytester.makeconftest(_SUB_CONFTEST) - pytester.makepyfile( - test_offender="import underworld3 as uw\n" - "mesh = uw.meshing.StructuredQuadBox(elementRes=(2, 2))\n" - "def test_x():\n assert True\n" - ) + pytester.makepyfile(test_offender=_OFFENDER) - result = pytester.runpytest_subprocess("--collect-only") + result = pytester.runpytest_subprocess("-q") assert result.ret != 0 - result.stderr.fnmatch_lines( - ["*changed global state while being COLLECTED*", "*test_offender.py*"] + result.stdout.fnmatch_lines( + ["*changed global state while being COLLECTED*", "*ERROR*test_offender.py*"] ) +def test_guard_reports_the_same_way_under_xdist(pytester): + """The distributed runner carries it as a collection error, not an INTERNALERROR.""" + + pytester.makeconftest(_SUB_CONFTEST) + pytester.makepyfile(test_offender=_OFFENDER) + + result = pytester.runpytest_subprocess("-q", "-n", "2") + + assert result.ret != 0 + result.stdout.fnmatch_lines(["*ERROR*test_offender.py*"]) + assert "INTERNALERROR" not in result.stdout.str() + + def test_guard_can_be_turned_off_for_a_generated_offender(pytester, monkeypatch): """`UW_TEST_COLLECTION_GUARD=off` lets the same module through. @@ -62,23 +82,19 @@ def test_guard_can_be_turned_off_for_a_generated_offender(pytester, monkeypatch) monkeypatch.setenv("UW_TEST_COLLECTION_GUARD", "off") pytester.makeconftest(_SUB_CONFTEST) - pytester.makepyfile( - test_offender="import underworld3 as uw\n" - "mesh = uw.meshing.StructuredQuadBox(elementRes=(2, 2))\n" - "def test_x():\n assert True\n" - ) + pytester.makepyfile(test_offender=_OFFENDER) - result = pytester.runpytest_subprocess("--collect-only") + result = pytester.runpytest_subprocess("-q") assert result.ret == 0 def test_guard_is_silent_when_nothing_is_built(pytester): - """The control: a module that only defines a test collects cleanly.""" + """The control: a module that only defines a test collects and runs cleanly.""" pytester.makeconftest(_SUB_CONFTEST) pytester.makepyfile(test_clean="def test_x():\n assert True\n") - result = pytester.runpytest_subprocess("--collect-only") + result = pytester.runpytest_subprocess("-q") assert result.ret == 0