pytest: harden the test integration and clear the collection blocklist#42449
Draft
mwhansen wants to merge 14 commits into
Draft
pytest: harden the test integration and clear the collection blocklist#42449mwhansen wants to merge 14 commits into
mwhansen wants to merge 14 commits into
Conversation
Bring the pytest unit-test runner closer to `sage -t` semantics: - Add `--long`/`--longlong` options and a collection hook that skips tests marked `long`/`longlong` unless requested. The markers were declared but never enforced, so long tests always ran. - Add an `optional` marker that skips a test unless all named Sage features are present, using the same `available_software` detection as the doctester (mirrors the `# optional`/`# needs` doctest tags). - Seed Sage's RNG before each test via an autouse fixture; the seed is taken from `--random-seed`/`SAGE_PYTEST_RANDOM_SEED` (else random) and reported in the test header so a run can be reproduced. - Add an `assert_close` fixture for numerically comparing symbolic or exact values, filling the gap pytest.approx leaves. Document all of the above in the developer guide.
`sage -t` already runs the pytest unit tests after the doctests, but it selected long/longlong tests with a hand-built `-m` marker expression and did not share its random seed with pytest. That conflicted with the new conftest gating: `sage -t --long` would have skipped long unit tests. Make conftest.py the single source of truth: forward `--long` (and the resolved `--random-seed`) to pytest instead of the `-m` expression. The whole `sage -t` run is now seeded from the one seed it reports, and `sage -t --long` again runs long unit tests while `longlong` stays off. Document the unified behaviour in the developer guide.
Two related robustness fixes for the pytest test paths: - SageDoctestModule now resolves a module's LazyImport objects before the doctest finder walks its namespace. The finder iterates module.__dict__, and touching a lazy import there resolves it, mutating the dict and raising "RuntimeError: dictionary changed size during iteration". This removes the sage.crypto and sage.crypto.mq entries from the collection blocklist (the categories lie-algebras module crashes deeper and stays blocked for now). - cvxpy_backend_test / scip_backend_test used pytest.importorskip as a class decorator, which returns the imported module; the decoration would fail with "module object is not callable" whenever the solver is actually installed. Use the module-level importorskip statement instead.
Generalize the lazy-member pre-resolution to classes and move it into the doctest finder's per-object _find hook. Besides module-level LazyImport, this also resolves lazy_class_attribute descriptors (e.g. `_axiom` on a category-with-axiom), which cache into the subclass dict on first access and otherwise mutate it while the finder iterates, raising "dictionary changed size during iteration". Removes the finite_dimensional_lie_algebras_with_basis entry from the collection blocklist; all of src/sage/categories now collects cleanly (2623 doctests).
…ashes pytest's faulthandler plugin installs its own SIGSEGV/SIGFPE handlers (via sigaltstack) that conflict with cysignals, turning signals Sage handles internally into spurious "Fatal Python error" aborts during doctest collection/execution. Disable it (`-p no:faulthandler`) so cysignals owns signal handling, as under `sage -t`. This clears the whole "Fatal Python error" collection blocklist. Six of the seven modules now collect and pass; finitely_generated.py needed a doctest fixed to use parentheses instead of a backslash continuation (which does not survive __doc__ extraction). finitely_presented.py stays blocked for an unrelated __doc__-vs-raw-source incompatibility, now documented accurately.
The combinat directory was excluded from pytest collection entirely with a vague "Crashes CI for some reason" since conftest.py was first added. The crash was at collection time and is resolved by the lazy-import resolution and faulthandler fixes: all of src/sage/combinat now collects cleanly (11715 doctests) and a full doctest run completes without any crash or worker death (271 advisory failures of the known __doc__-vs-raw-source / ordering class, all of which pass under `sage -t`). Removing the block brings combinat under the same collection-health gate as the rest of the library; doctest *execution* via pytest remains advisory, with `sage -t` authoritative.
System-level cleanups for the *_test.py unit tests: - Mark sage.misc.sage_unittest.TestSuite with __test__ = False so pytest no longer warns "cannot collect test class 'TestSuite'". This lets the unit tests import it at module level; previously seven files imported it inside the test function to dodge the warning. - Add a run_test_suite conftest fixture wrapping TestSuite(obj).run(...). It defaults raise_on_failure=True, which is required for a failing suite to actually fail the pytest test (the bare TestSuite.run default only prints). Migrate all seven call sites to it. - padic_lattice_element_test: replace a module-level filterwarnings() call (which leaked the FutureWarning filter into the whole session) with a scoped @pytest.mark.filterwarnings on the test.
finitely_presented.py collects cleanly (47 doctests); only doctest *execution* fails, and only under pytest -- it passes under `sage -t`, the authoritative doctest runner. Excluding it from collection was over-broad: it dropped the file from the collection-health gate (which we want) merely to avoid advisory execution failures, inconsistent with how combinat and the rest of the tree are treated (collected; pytest doctest execution is advisory, `|| true` in CI). This leaves no content-specific file exclusions in conftest.py; the remaining IgnoreCollector cases are structural (.pyx, __main__.py/setup.py, the nbconvert executable, and the doctest framework's own forker/reporting).
Make SageDoctestModule skip a module whose ``__test__`` is ``False`` (the standard pytest opt-out). The check happens before the doctest finder runs, because the stdlib finder otherwise treats ``__test__`` as a dict of extra doctests and crashes on the bool. Use it to replace the conftest blocklist entry for the doctest framework's own forker.py and reporting.py: their doctests spoof stdin/stdout and run nested doctest runners, which conflicts with pytest's capture (e.g. ``sys.stdout.fileno()`` raises). The opt-out now lives in those files, and ``sage -t`` still runs their doctests since it reads the raw source and ignores ``__test__``. conftest.py now has no per-file content exclusions; the remaining IgnoreCollector cases are purely structural (.pyx, __main__.py/setup.py, the nbconvert executable script).
Describe the module-level __test__ = False opt-out introduced for the doctest collector, including when to use it (mis-collected Test*/test_* names, doctests that conflict with pytest's stdout capture) and that `sage -t` ignores it.
Move the root conftest.py to src/sage/_pytest_plugin.py and load it via `addopts = "... -p sage._pytest_plugin"` in pyproject.toml. Because addopts is read from the rootdir config regardless of the working directory, the plugin's options (--doctest, --long, --longlong, --random-seed) are registered even with no conftest.py at the repository root -- which is what broke the plain "move conftest into src/sage/" approach (sagemathgh-42203): pytest_addoption in a non-root conftest is loaded too late when pytest runs from the root. This removes the last conftest.py from the repository root (avoiding the wheel-in-subdirectory clash that motivated sagemathgh-42203) while keeping every option, collection hook, and fixture working. Unlike a pytest11 entry point, loading via addopts keeps the plugin scoped to this repository, so it does not affect pytest runs in other projects that merely have sagemath installed. Verified: `pytest --doctest`/`--long`/`--random-seed` work from both the repo root and src/; unit tests, doctests, the lazy_import/lazy_class_attribute collection fixes, __test__=False handling, and `sage -t` all behave as before. Relates to sagemathgh-42203.
conftest.py no longer exists at the repository root (it moved to src/sage/_pytest_plugin.py and is loaded via addopts), so the generated Dockerfile's `ADD ... conftest.py ... /new/` failed with "/conftest.py": not found. The pytest plugin now ships with the rest of the source under src/sage/.
Contributor
|
Thanks for doing this! I'll take a closer look when it's ready for review. This one stands out because I recently added it:
Running the long and longlong tests with a plain "pytest" was (for better or worse) intentional. It ensures that the longlong tests are run on the CI, while |
|
Documentation preview for this PR (built with commit 3adf25b; changes) is ready! 🎉 |
Per review (sagemathgh-42449): keep the `long`/`longlong` markers running by default under a plain `pytest` invocation -- that is how they get CI coverage -- and select them with the standard pytest marker expression instead of `sage -t` -style `--long`/`--longlong` flags. - Drop the `--long`/`--longlong` options and the skip-by-default logic from the plugin; only the feature-based `optional` skip remains in pytest_collection_modifyitems. - `sage -t` again restricts its internal pytest run with `-m 'not longlong'` (and `and not long` unless `--long`), while still forwarding `--random-seed`. The `long`/`longlong` markers remain registered in pyproject.toml so `-m '...'` selection works under --strict-markers.
Contributor
Author
Sounds good -- I've updated it to use to run those by default. |
At its old root location conftest.py was outside pyright's include
("src/sage"), so the file-level 'pyright: strict' directive was never
enforced. Now that it lives under src/sage/, strict would flag ~190 errors
that are inherent to pytest's largely untyped hook/fixture API. Drop the
directive so the module is checked under the project's default (lenient)
pyright configuration, like the rest of src/sage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📜 Summary
This brings Sage's
pytestintegration closer to feature parity withsage -t,removes every per-file/per-directory workaround from the collection blocklist by
fixing the underlying causes, and restructures the configuration so that
conftest.pyno longer has to live at the repository root.It is a set of small, independent commits under the umbrella of #28936 (adopt
mainstream Python testing infrastructure). Nothing changes for authors writing
ordinary doctests;
sage -tremains the authoritative doctest runner.🤔 Motivation
Running
pytestover the Sage tree relied on a large hand-maintained blocklistin
conftest.py("Crashes CI for some reason", "Fails with Fatal Pythonerror", …) and the custom options (
--long, random seed) were either missingor duplicated between
sage -tandconftest.py. Investigating the blocklistshowed every entry was a fixable root cause rather than an inherent
limitation.
🛠️ What this does
Runner parity / pytest-native selection
long/longlongmarkers. A plainpytestrunexecutes them by default (this is how they get CI coverage); restrict a run
with the standard marker expression, e.g.
-m 'not longlong'.sage -tapplies that expression to its internal
pytestrun, so it skips both unless--longis given (and never runslonglong), matching its historicbehaviour. (Earlier revisions of this PR added
--long/--longlongflagsthat skipped by default; dropped in favour of marker selection per review —
see thread.)
optional("feature", …)marker that skips unless all named Sagefeatures are present, using the same detection as the doctester
(
# optional/# needs).--random-seed/SAGE_PYTEST_RANDOM_SEED(else a fresh seed), reported in the header forreproducibility;
sage -tforwards its resolved seed to the internalpytestrun so the whole invocation replays from a single seed.assert_closefixture (Sage-awarepytest.approxfor symbolic/exactvalues) and a
run_test_suitefixture that wrapsTestSuite(obj).run(...)and defaults
raise_on_failure=True— required for a failing suite toactually fail the test. Migrate the existing call sites.
Collection robustness (blocklist → 0 content exclusions)
LazyImportandlazy_class_attributemembers before the doctestfinder walks a module/class, fixing
RuntimeError: dictionary changed size during iteration(re-enablessage.crypto,sage.crypto.mq, and thecategories with axioms, e.g.
finite_dimensional_lie_algebras_with_basis).faulthandlerplugin (-p no:faulthandler): it installsSIGSEGV/SIGFPE handlers that conflict with cysignals, turning signals Sage
handles internally into spurious "Fatal Python error" aborts (re-enables the
GAP/PARI modules: arithgroup, lfunctions/pari, permgroup_named, matrix_gps,
groups/libgap_mixin, generators/classical_geometries).
combinatexclusion (collection-time crash, now fixed):11715 doctests collect and a full run completes with no crash.
__test__ = Falsein the doctest collector, and use it forthe doctest framework's own
forker.py/reporting.py(their doctests spoofstdin/stdout and run nested doctest runners, which conflicts with pytest's
capture).
sage -tignores__test__and still runs them.pytest.importorskipas a class decorator,and a
\-continuation doctest inmatrix_gps/finitely_generated.pythat doesnot survive
__doc__extraction.After this,
conftest.pyhas no per-file content exclusions; the onlyIgnoreCollectorcases left are structural (.pyx,__main__.py/setup.py,the nbconvert executable script).
Configuration as a plugin (no root
conftest.py)conftest.pytosrc/sage/_pytest_plugin.pyand load it viaaddopts = "… -p sage._pytest_plugin". Becauseaddoptsis read from therootdir config regardless of the working directory, the plugin's options
register at startup even with no
conftest.pyat the repo root. This is whatblocked the plain "move conftest into
src/sage/" approach in Moveconftest.pyintosrc/sage/#42203(
pytest_addoptionin a non-root conftest is loaded too late). Loading viaaddoptskeeps the plugin scoped to this repository, unlike apytest11entry point which would load for every pytest user that has
sagemathinstalled.
Docs
vs
TestSuite, the*_test.pyconvention, thelong/optionalmarkers, therandom-seed/
assert_close/run_test_suitehelpers, and the__test__ = Falseopt-out.✅ Testing
pytest --doctest --long --random-seed=…works from both the repo root andsrc/.no crash; categories; the GAP/PARI modules).
*_test.pyunit suite passes;sage -t(doctest controller + internalpytest) is unchanged end to end, seeded from the single reported seed.
🔗 Related
conftest.pyintosrc/sage/#42203 — achieves theconftest.pyrelocation and fixes theoption-registration that blocked it.
Test-prefixed class pytest mis-collects):TestSuitenow sets__test__ = False.TestSuite→ pytest); under the Meta-ticket: Adopt mainstream Python testing/linting infrastructure: tox, pytest, ..., describe in Developer's Guide #28936 umbrella;related to Replace custom Sage unit test discovery by pytest #30738, Rename test_... functions to avoid the naming scheme reserved by pytest #33549, Add doctest utility to handle deprecation and future warnings #39211.
📝 Checklist
validated by running the existing doctest/unit suites — see Testing)
⏳ Dependencies
None. Coordinates with #42203 (this supersedes it).