Use pytest for more TestSuite tests #40971

orlitzky · 2025-10-03T21:00:29Z

Fix some "slow doctest" warning by moving these out of the doctests, and into pytest where (IMO) they belong. They aren't for end-user consumption, and we aren't checking the output from them (as is the point of a doctest).

github-actions · 2025-10-03T22:14:08Z

Documentation preview for this PR (built with commit a424b8e; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

tobiasdiez · 2025-10-04T08:33:36Z

src/sage/rings/padics/padic_lattice_element_test.py

+        R3 = QpLC(2)
+        R4 = QpLF(2)
+
+    for R in (R1, R2, R3, R4):


Use paramatrized tests instead of the internal for loop?

orlitzky · 2025-10-04T20:34:37Z

The last run had a strange failure in dev_tools.py, I just re-pushed the same branch to see if it's a fluke before I waste any time.

orlitzky · 2025-10-04T23:07:59Z

No such luck:

src/bin/sage -t --long --warn-long 30.0 --random-seed=286735480429121101562228604801325644303 src/sage/misc/dev_tools.py
**********************************************************************
Error: Failed example:: Exception raised:
Traceback (most recent call last):
  File "/sage/src/sage/doctest/forker.py", line 733, in _run
    self.compile_and_execute(example, compiler, test.globs)
  File "/sage/src/sage/doctest/forker.py", line 1157, in compile_and_execute
    exec(compiled, globs)
  File "<doctest sage.misc.dev_tools.find_objects_from_name[1]>", line 1, in <module>
    dt.find_objects_from_name('FareySymbol')                                  # needs sage.modular
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/sage/src/sage/misc/dev_tools.py", line 279, in find_objects_from_name
    for smodule_name, smodule in sys.modules.items():
RuntimeError: dictionary changed size during iteration

    dt.find_objects_from_name('FareySymbol')                                  # needs sage.modular
Exception raised:
    Traceback (most recent call last):
      File "/sage/src/sage/doctest/forker.py", line 733, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/sage/src/sage/doctest/forker.py", line 1157, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.dev_tools.find_objects_from_name[1]>", line 1, in <module>
        dt.find_objects_from_name('FareySymbol')                                  # needs sage.modular
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/sage/src/sage/misc/dev_tools.py", line 279, in find_objects_from_name
        for smodule_name, smodule in sys.modules.items():
    RuntimeError: dictionary changed size during iteration
**********************************************************************
Error: Failed example:: Exception raised:
Traceback (most recent call last):
  File "/sage/src/sage/doctest/forker.py", line 733, in _run
    self.compile_and_execute(example, compiler, test.globs)
  File "/sage/src/sage/doctest/forker.py", line 1157, in compile_and_execute
    exec(compiled, globs)
  File "<doctest sage.misc.dev_tools.find_objects_from_name[3]>", line 1, in <module>
    dt.find_objects_from_name('RR')
  File "/sage/src/sage/misc/dev_tools.py", line 279, in find_objects_from_name
    for smodule_name, smodule in sys.modules.items():
RuntimeError: dictionary changed size during iteration

    dt.find_objects_from_name('RR')
Exception raised:
    Traceback (most recent call last):
      File "/sage/src/sage/doctest/forker.py", line 733, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/sage/src/sage/doctest/forker.py", line 1157, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.dev_tools.find_objects_from_name[3]>", line 1, in <module>
        dt.find_objects_from_name('RR')
      File "/sage/src/sage/misc/dev_tools.py", line 279, in find_objects_from_name
        for smodule_name, smodule in sys.modules.items():
    RuntimeError: dictionary changed size during iteration
**********************************************************************
Error: Failed example:: Exception raised:
Traceback (most recent call last):
  File "/sage/src/sage/doctest/forker.py", line 733, in _run
    self.compile_and_execute(example, compiler, test.globs)
  File "/sage/src/sage/doctest/forker.py", line 1157, in compile_and_execute
    exec(compiled, globs)
  File "<doctest sage.misc.dev_tools.find_objects_from_name[7]>", line 1, in <module>
    objs = dt.find_objects_from_name('find_objects_from_name')
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/sage/src/sage/misc/dev_tools.py", line 279, in find_objects_from_name
    for smodule_name, smodule in sys.modules.items():
RuntimeError: dictionary changed size during iteration

    objs = dt.find_objects_from_name('find_objects_from_name')
Exception raised:
    Traceback (most recent call last):
        import_statements('Z')
      File "/sage/src/sage/misc/dev_tools.py", line 528, in import_statements
        modules.update(find_object_modules(o))
                       ^^^^^^^^^^^^^^^^^^^^^^
      File "/sage/src/sage/misc/sageinspect.py", line 2778, in find_object_modules
        for module_name, module in sys.modules.items():
    RuntimeError: dictionary changed size during iteration
**********************************************************************
Error: Failed example:: Got: 
Traceback (most recent call last):
  File "/sage/src/sage/doctest/forker.py", line 733, in _run
    self.compile_and_execute(example, compiler, test.globs)
  File "/sage/src/sage/doctest/forker.py", line 1157, in compile_and_execute
    exec(compiled, globs)
  File "<doctest sage.misc.dev_tools.import_statements[17]>", line 1, in <module>
    import_statements('HeckeMonoid')
  File "/sage/src/sage/misc/dev_tools.py", line 506, in import_statements
    obj = find_objects_from_name(name, include_lazy_imports=True)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/sage/src/sage/misc/dev_tools.py", line 279, in find_objects_from_name
    for smodule_name, smodule in sys.modules.items():
RuntimeError: dictionary changed size during iteration

    import_statements('HeckeMonoid')
Expected:
    Traceback (most recent call last):
    ...
    LookupError: no object named 'HeckeMonoid'
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/sage/src/sage/doctest/forker.py", line 733, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/sage/src/sage/doctest/forker.py", line 1157, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.dev_tools.import_statements[17]>", line 1, in <module>
        import_statements('HeckeMonoid')
      File "/sage/src/sage/misc/dev_tools.py", line 506, in import_statements
        obj = find_objects_from_name(name, include_lazy_imports=True)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/sage/src/sage/misc/dev_tools.py", line 279, in find_objects_from_name
        for smodule_name, smodule in sys.modules.items():
    RuntimeError: dictionary changed size during iteration
**********************************************************************
2 items had failures:
   5 of  12 in sage.misc.dev_tools.find_objects_from_name
   4 of  38 in sage.misc.dev_tools.import_statements
    [60 tests, 9 failures, 3.16s wall]

Naturally it works fine when I re-run it on my machine.

tobiasdiez

LGTM. I still have two questions, but they are not major and don't need to be 'resolved' before merging.

tobiasdiez · 2025-10-05T05:41:43Z

src/sage/combinat/positive_integer_semigroup_test.py

+    PP = PositiveIntegerSemigroup()
+
+    # fewer max_runs since these are kind of slow
+    TestSuite(PP).run(verbose=True,


Is the verbose = True option needed for the functionality? If not, I think, I would prefer the quiet version, or maybe couple it to pytests verbosity level via pytestconfig.get_verbosity() > 0 (or 1?)

Pytest already swallows stdout unless you pass -s, so i was thinking the added verbosity would only matter if the tests fail. I guess we could go back and make them all depend on the pytest verbosity though. I don't feel strongly about it either way.

I noticed it mostly in CI, where it bloats the log without adding too much value (at least when everything passes). For the moment, that's okay but might get more annoying once more tests are migrated.

So maybe I do this later if I get annoyed enough - or I'll actually extract all of these testsuite _test methods (in the actual classes) to pytest.

We could probably get away without -s in the CI. I think pytest will still dump the output that it captured in the event of a failure, while leaving everything nice and quiet if it passes.

tobiasdiez · 2025-10-05T05:44:22Z

src/sage/rings/padics/padic_lattice_element_test.py

+
+@pytest.fixture
+def R1():
+    from warnings import catch_warnings, filterwarnings


What's the reason for the local imports?

Though it's ugly, my reasoning was that the fixtures are loaded on-demand by the tests, and in the future we may run only a subset of tests (requiring a subset of fixtures). In that case the local imports won't slow things down.

Are these imports really so expensive that it's worth optimizing this (at the cost of readability)?

🤷 I moved them to the top level

tobiasdiez

okay, let's get it in

user202729 · 2025-10-08T10:40:34Z

src/sage/rings/padics/padic_lattice_element_test.py

+elements = ( "R1", "R2", "R3", "R4" )
+
+
+@pytest.mark.parametrize("e", elements)


the amount of boilerplate is scary.

Why can't you parametrize e over [ZpLC(2), ZpLF(2), etc.] directly?

There are a couple of related problems. (I'm not happy about the boilerplate either, but it should decrease in relative volume as more tests are moved to pytest.)

The big one is that we want to cache the fixtures and create them on-demand, so they aren't recreated multiple times, or created when they won't be used (if only a subset of tests is being run). For that magic to work, we need to mark the fixtures with @pytest.fixture... but then you can't call them directly:

_____ ERROR collecting src/sage/rings/padics/padic_lattice_element_test.py _____ Fixture "R1" called directly. Fixtures are not meant to be called directly, but are created automatically when test functions request them as parameters.

If we forego the caching / on-demand creation, we can create the objects ourselves, but then we lose the nice fixture naming. if I pass in ZpLC(2), for example, we get...

src/sage/rings/padics/padic_lattice_element_test.py::test_padic_lattice_element[e0]...

instead of

src/sage/rings/padics/padic_lattice_element_test.py::test_padic_lattice_element[R1]...

which uses the fixture name. Now obviously R1 is a lame name for a fixture and doesn't improve anything, but I'm trying to structure these in a decent way for future copy/pasting where the fixtures may actually be meaningful and take a while to construct.

user202729 · 2025-10-08T10:41:23Z

src/sage/rings/valuation/mapped_valuation_test.py

+extensions = ( "w0", "w1" )
+
+
+@pytest.mark.parametrize("e", extensions)


This part can do parametrize("index", [0, 1]) I guess.

Ok, this one was easy to improve without hurting anything. I rebased onto the latest develop, but only the final commit is new.

user202729 · 2025-10-08T10:43:21Z

src/sage/rings/valuation/mapped_valuation_test.py

+def test_finite_extension_from_limit_valuation(e, request):
+    r"""
+    Run the ``TestSuite()`` for two examples given in the
+    ``FiniteExtensionFromLimitValuation`` documentation.


Speaking of more downsides of pytest migration, a cost of having the test far from the code is that now if the example given in the FiniteExtensionFromLimitValuation documentation is changed, this docstring will silently become false.

You can change the docstring to "run the test suite for two examples that were given in the FiniteExtensionFromLimitValuation documentation", but then what's the value of the docstring?

This is just laziness on my part and not inherent to pytest. I'd love to describe the objects being tested more intelligently, or explain why they're being tested... but the original doctests didn't come with an explanation, and I don't know anything about mapped valuations, so this was all I could think of.

Copy two TestSuite() runs from a TESTS:: block to pytest. These are on the verge of hitting the "slow doctest" limit for long tests, and sometimes cause warnings in the CI. But more importantly, they are simply better suited to pytest: the user would never want to run them, and the output is not meaningful as documentation.

These have been moved to pytest and no longer need to be run as doctests.

Move some of the TestSuite() tests from a TESTS block to pytest. These are slow, and not really meant for end-user consumption, so are more appropriate here.

Delete a few doctests that have been moved to pytest.

Move some TestSuite tests from src/sage/combinat/backtrack.py (for the PositiveIntegerSemigroup class) to a new pytest file. These tests aren't meant for end-user consumption, and they are slow, so they are more appropriate here.

Remove a TestSuite() doctest that has been moved to pytest.

Shuffling around the doctests has made some FutureWarnings happen in different places.

Run tools/update-meson.py to install the new pytest source file.

Use parameterized tests and fixtures instead of an internal "for" loop to improve the reporting a bit.

Use parameterized tests and fixtures to improve the reporting from these tests.

In an attempt to avoid the following error on the CI... dt.find_objects_from_name('FareySymbol') ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/sage/src/sage/misc/dev_tools.py", line 279, in find_objects_from_name for smodule_name, smodule in sys.modules.items(): RuntimeError: dictionary changed size during iteration we make a copy of the sys.modules dict before iterating over its items.

Move the padic imports and FutureWarning filter to the top level, so we don't have to repeat them four times.

The one test in this file is easy to parameterize based on the index (an int) of a fixture supplied to it. This allows us to eliminate a lot of the boilerplate associated with fixture parameterizations.

orlitzky marked this pull request as draft October 3, 2025 21:00

orlitzky force-pushed the pytest-for-testsuite branch from 7e6b8e0 to 49afbb0 Compare October 3, 2025 21:04

tobiasdiez reviewed Oct 4, 2025

View reviewed changes

orlitzky force-pushed the pytest-for-testsuite branch 2 times, most recently from 1848691 to 4b603c1 Compare October 4, 2025 20:33

orlitzky marked this pull request as ready for review October 4, 2025 20:33

github-actions bot added the s: needs review label Oct 4, 2025

tobiasdiez reviewed Oct 5, 2025

View reviewed changes

orlitzky mentioned this pull request Oct 5, 2025

Reenable pytest again as part of sage -t #40446

Open

5 tasks

tobiasdiez approved these changes Oct 6, 2025

View reviewed changes

tobiasdiez added s: positive review and removed s: needs review labels Oct 6, 2025

user202729 reviewed Oct 8, 2025

View reviewed changes

orlitzky added 12 commits October 8, 2025 12:21

src.sage.rings.valuation.mapped_valuation: drop two TestSuite() tests

277fa29

These have been moved to pytest and no longer need to be run as doctests.

sage.rings.padics.padic_lattice_element_test: new pytests

66b496f

Move some of the TestSuite() tests from a TESTS block to pytest. These are slow, and not really meant for end-user consumption, so are more appropriate here.

sage.rings.padics.padic_lattice_element: delete old TESTS

09bca86

Delete a few doctests that have been moved to pytest.

sage.combinat.backtrack: delete old doctest

d5a6af0

Remove a TestSuite() doctest that has been moved to pytest.

sage.rings.padics.padic_lattice_element: add back FutureWarnings

75ec8a6

Shuffling around the doctests has made some FutureWarnings happen in different places.

src/sage/rings/padics/meson.build: add padic_lattice_element_test.py

82bba0d

Run tools/update-meson.py to install the new pytest source file.

src/sage/combinat/meson.build: add positive_integer_semigroup_test.py

b9988a7

Run tools/update-meson.py to install the new pytest source file.

sage.rings.padics.padic_lattice_element_test: parameterize tests

0f42046

Use parameterized tests and fixtures instead of an internal "for" loop to improve the reporting a bit.

sage.rings.valuation.mapped_valuation_test: parameterize tests

546c87f

Use parameterized tests and fixtures to improve the reporting from these tests.

orlitzky added 2 commits October 8, 2025 12:21

sage.rings.padics.padic_lattice_element_test: improve readability

73f297f

Move the padic imports and FutureWarning filter to the top level, so we don't have to repeat them four times.

sage.rings.valuation.mapped_valuation_test: eliminate boilerplate

a424b8e

The one test in this file is easy to parameterize based on the index (an int) of a fixture supplied to it. This allows us to eliminate a lot of the boilerplate associated with fixture parameterizations.

orlitzky force-pushed the pytest-for-testsuite branch from 3cd9ba7 to a424b8e Compare October 8, 2025 16:55

github-actions bot added s: needs review and removed s: positive review labels Oct 8, 2025

		elements = ( "R1", "R2", "R3", "R4" )


		@pytest.mark.parametrize("e", elements)

		extensions = ( "w0", "w1" )


		@pytest.mark.parametrize("e", extensions)

Uh oh!

Use pytest for more TestSuite tests #40971

Are you sure you want to change the base?

Use pytest for more TestSuite tests #40971

Conversation

orlitzky commented Oct 3, 2025

Uh oh!

github-actions bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orlitzky commented Oct 4, 2025

Uh oh!

orlitzky commented Oct 4, 2025

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Oct 3, 2025 •

edited

Loading