fix(tests): an optional-dependency gate skips its cells, not the whole module - #3757
Conversation
… the whole module
pytest.importorskip raises Skipped rather than returning None, and a decorator
is evaluated while the module is imported, so a gate written as
@pytest.mark.skipif(pytest.importorskip("x") is None, ...) skips the whole file
instead of the test or class it sits on. Two gates were written that way. On an
interpreter without the extra, each file collected nothing at all: the policies
kwargs module lost 17 cells that need no zmq, and the sim describe module lost
33 that need no mujoco - 50 cells dropped by gates protecting the 18 that do.
A merge check installs every extra, so it collected all 68 and read green.
Both now read presence as a spec, the convention the rest of the suite keeps,
and a new whole-tree guard refuses the shape so it cannot return.
yinsong1986
left a comment
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Fixes two optional-dependency gates that were written as @pytest.mark.skipif(pytest.importorskip("x") ...) — a shape that raises Skipped at module import time, muting every test in the file instead of just the gated cell (50 unrelated cells dropped on interpreters without the zmq/mujoco extras, invisible in CI because the merge check installs all extras). Both gates now read presence via importlib.util.find_spec, matching the suite's existing convention, and a new tree-wide AST guard (tests/test_an_optional_dependency_gate_does_not_mute_its_module.py) refuses the broken shape going forward.
What's good
- The new guard distinguishes scope-disagreeing gates (decorator-position
importorskip) from legitimate uses (module-scope deliberate mutes, test-body calls, spec-based gates), with parametrized positive and negative cases pinning the predicate. - Behavioural pin runs a synthetic module in a subprocess and asserts the actual collected/skipped counts for both spellings — tests the outcome, not the implementation (AGENTS.md convention 9).
- The
test_both_test_trees_are_scannedself-check guards against the sweep silently narrowing; the> 1000threshold holds against the current tree (1809 test modules undertests/+tests_integ/). - Test-only change with no public API, wire-format, or security surface; the subprocess invocation uses literal argv only.
Verification suggestions
On an interpreter without the extras (pip uninstall zmq mujoco in a scratch venv), confirm the two fixed files now collect their independent cells:
pytest tests/policies/test_a_misspelled_provider_kwarg_is_refused_naming_the_one_meant.py -q
pytest tests/simulation/test_sim_engine_describe_discovery.py -qExpect 17 passed / 1 skipped and 33 passed / 17 skipped respectively, per the table in the guard module's docstring.
2 participants
What
pytest.importorskipraises rather than returningNone, and a decorator is evaluated at import, so@pytest.mark.skipif(pytest.importorskip("x") is None, ...)skipped the whole file rather than the cell it named. Two gates were written that way. Both now read presence as a spec (importlib.util.find_spec), the convention the rest of the suite keeps, and a whole-tree guard refuses the shape.Why Without the extra, each file collected nothing: 17 cells needing no
zmqand 33 needing nomujoco, dropped by gates protecting the 18 that do. A merge check installs every extra, so all 68 ran there and read green.Tests The guard fails on the pre-fix tree naming both offenders and passes after (11 cells); 5/5 mutants caught; a subprocess pin measures both spellings (none collected vs 2 passed, 1 skipped). The two files still run 68 cells; ruff, format and mypy clean. #3343 edits the same sim module but neither the gate nor its imports.