Skip to content

test: isolate test_pybamm_import from sys.modules mutation (fixes #5402) - #5538

Open
gaoflow wants to merge 2 commits into
pybamm-team:mainfrom
gaoflow:fix/test-pybamm-import-sys-modules-isolation
Open

test: isolate test_pybamm_import from sys.modules mutation (fixes #5402)#5538
gaoflow wants to merge 2 commits into
pybamm-team:mainfrom
gaoflow:fix/test-pybamm-import-sys-modules-isolation

Conversation

@gaoflow

@gaoflow gaoflow commented May 28, 2026

Copy link
Copy Markdown

Isolate test_pybamm_import and test_import_optional_dependency from sys.modules mutation

Fixes #5402.

Problem

tests/unit/test_util.py::TestUtil::test_pybamm_import mutates sys.modules
in-process to pretend that all optional dependencies are absent, then unloads
pybamm itself so the next importlib.import_module("pybamm") re-runs the
top-level import logic. The hand-rolled try/finally restores entries
afterwards, but for the duration of the test the pytest worker is in a
partially-consistent state, and any partial import state cached during the
re-import (logging handlers, import locks, optional-dependency fallbacks
already evaluated in other modules) cannot be undone by writing back to
sys.modules.

With pytest-xdist that surfaces as order-dependent CI flakiness in
unrelated tests
— the issue author confirmed in #5402 that simply
@pytest.mark.skip-ing this single test made all CI tests pass consistently.
@pytest.mark.forked did not help because fork() inherits the parent's
already-cached import state.

Fix

Remove the in-process mutation entirely.

  • test_pybamm_import now runs the import pybamm check inside a fresh
    subprocess.run([sys.executable, "-c", ...]). The script masks every present
    optional dependency (and any already-imported sub-modules) inside its own
    throw-away interpreter, then imports pybamm. The parent worker's
    sys.modules is never touched. A non-zero exit code propagates stdout
    and stderr into the pytest failure message so debugging stays easy.

    This is strictly stronger isolation than pytest.mark.forked: a spawned
    Python interpreter starts with a clean slate, while fork() copies the
    parent's already-cached import state.

  • test_import_optional_dependency uses pytest's monkeypatch.setitem
    to mask sys.modules entries. The mutation is automatically reverted by
    pytest's fixture teardown even on failure — there is no need for this
    test to run in a subprocess because it does not unload pybamm.

Verification

The refactored tests preserve the exact original intent:

  • test_pybamm_import still asserts that import pybamm succeeds when every
    present optional dependency is marked missing.
  • test_import_optional_dependency still asserts that
    pybamm.util.import_optional_dependency(pkg) raises
    ModuleNotFoundError with the expected message.
$ pytest tests/unit/test_util.py -v
...
tests/unit/test_util.py::TestUtil::test_pybamm_import PASSED
tests/unit/test_util.py::TestUtil::test_import_optional_dependency PASSED
tests/unit/test_util.py::TestUtil::test_optional_dependencies PASSED
tests/unit/test_util.py::TestUtil::test_fuzzy_dict PASSED
tests/unit/test_util.py::TestUtil::test_get_parameters_filepath PASSED
tests/unit/test_util.py::TestUtil::test_is_constant_and_can_evaluate PASSED
tests/unit/test_util.py::TestSearch::test_url_gets_to_stdout PASSED
============================== 7 passed in 4.15s ===============================

Negative-test sanity (deliberately mask numpy instead of an optional dep):

$ python -c "import subprocess, sys, textwrap; ..."
returncode: 1
stderr tail: ['ModuleNotFoundError: import of numpy halted; None in sys.modules']

so the subprocess approach still surfaces a regression if pybamm ever
starts to require an optional dep at import time.

ruff check tests/unit/test_util.py and ruff format --check tests/unit/test_util.py are both clean.

Why this scope

The smallest change that fixes the root cause: only test_util.py is touched.
No production code is altered. The two adjacent tests that exhibit the same
in-process-mutation anti-pattern are fixed together so the file is internally
consistent.

@gaoflow
gaoflow requested a review from a team as a code owner May 28, 2026 06:12
gaoflow added 2 commits June 19, 2026 00:28
…from sys.modules mutation

`test_pybamm_import` was mutating `sys.modules` in-process to pretend that
all optional dependencies were absent, then unloading `pybamm` itself so
the next `importlib.import_module("pybamm")` would re-run the top-level
import logic. The hand-rolled try/finally restored entries afterwards,
but the mutation window left the pytest worker in a partially-consistent
state for the duration of the test — and worse, any partial import state
cached during the re-import (logging handlers, import locks, optional-
dependency fallbacks already evaluated in other modules) could not be
undone by restoring `sys.modules`. With pytest-xdist that surfaces as
order-dependent CI flakiness in unrelated tests; pytest-forked did not
help because fork() inherits the parent's already-cached import state
(see pybamm-team#5402 for the full diagnosis).

This commit removes the in-process mutation:

* `test_pybamm_import` now runs the `import pybamm` check inside a
  fresh `subprocess.run([sys.executable, "-c", ...])`. The script
  marks every present optional dependency (and any already-imported
  sub-modules) as missing inside its own throw-away interpreter,
  then imports `pybamm`. The parent worker's `sys.modules` is never
  touched. A non-zero exit code propagates `stdout`/`stderr` into
  the pytest failure message.

* `test_import_optional_dependency` uses pytest's `monkeypatch.setitem`
  to mask `sys.modules` entries. The mutation is automatically reverted
  by pytest's fixture teardown even on failure — there is no need for
  this test to run in a subprocess because it does not unload `pybamm`.

Fixes pybamm-team#5402.
Match the precedent already in `src/pybamm/codegen/compilation.py` and
`tests/unit/test_solvers/test_solution.py`: the script run via
`subprocess.run([sys.executable, "-c", ...])` is a literal string
constructed in the test itself, not user input, so the standard
`# nosec B404` (import) and `# nosec B603` (call) markers are
appropriate. No behavior change.
@gaoflow
gaoflow force-pushed the fix/test-pybamm-import-sys-modules-isolation branch from 2aea61a to 3e6e199 Compare June 18, 2026 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: test_pybamm_import introduces global state mutation via sys.modules leading to CI instability

1 participant