Test modules must not build state while being collected

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

tests/conftest.py

@@ -47,6 +47,150 @@
     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.
+#
+# `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():
+    """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)
+
+
+@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
+
+    before = _global_state_fingerprint()
+    outcome = yield
+
+    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 not moved or _is_known_offender(collector.nodeid):
+        return
+
+    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 = [
+        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, #587).",
+    ]
+
+    report.outcome = "failed"
+    report.longrepr = "\n".join(lines)
+    report.result = []
+
+
 @pytest.fixture(scope="module", autouse=True)
 def isolate_module_state():
     """Reset the global model BEFORE a module's own fixtures are built.

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)