Run function field TestSuites via pytest

[orlitzky]

• Fix two slow doctest warnings in the CI by moving these tests to pytest.
• Speed some test suites up by using fewer repeated runs. Nowadays we run the tests on every PR, so repeated runs within repeated runs are less valuable.
• Use QQ instead of QQbar for some tests where this was the original intention.
• Clean up the docs by eliminating pure-test code from the docstrings.

[orlitzky]

These slow tests arose by mistake (inserting a QQbar example to clobber a QQ example before calling TestSuite.run()), but I think moving them to pytest is the right thing to do regardless of the slow doctest warnings.

[tobiasdiez]

I think it's a good idea to move those tests to pytest.

As a small suggestion, what do you think about using parametrized tests since they all just call the test suite? Would make easier in the future to actually move the test suite methods to pytest (see #30738)

[orlitzky]

I think it's a good idea to move those tests to pytest.

As a small suggestion, what do you think about using parametrized tests since they all just call the test suite? Would make easier in the future to actually move the test suite methods to pytest (see #30738)

Sure, what do you think should be parameterized? I would guess the field and number of TestSuite runs, but then we run into a problem because the "setup" code to generate the fields is itself a tiny bit slow and we don't want to run it while pytest is collecting tests. There are docs on working around that, but the constructions of the fields/rings are intertwined here, unlike in that example. We don't want to repeatedly construct them, so it's not obvious to me how to cleanly parameterize everything without e.g. ugly manual cacheing. I rarely use pytest however so I may be missing something obvious.

[user202729]

How exactly does pytest helps here?

[tobiasdiez]

I think it's a good idea to move those tests to pytest.
As a small suggestion, what do you think about using parametrized tests since they all just call the test suite? Would make easier in the future to actually move the test suite methods to pytest (see #30738)

Sure, what do you think should be parameterized? I would guess the field and number of TestSuite runs, but then we run into a problem because the "setup" code to generate the fields is itself a tiny bit slow and we don't want to run it while pytest is collecting tests. There are docs on working around that, but the constructions of the fields/rings are intertwined here, unlike in that example. We don't want to repeatedly construct them, so it's not obvious to me how to cleanly parameterize everything without e.g. ugly manual cacheing. I rarely use pytest however so I may be missing something obvious.

Okay, then let's leave it like its currently.

Would be nice to see if the tests are actually passing on CI. I tried to reactivate pytest in #40446, but there are a few strange errors (that I don't have the time right now to investigate further)

[orlitzky]

How exactly does pytest helps here?

Pytest isn't needed to speed up the tests, but there are some additional reasons to prefer it for these unit-test suites.

The main benefit of doctests is that they double as readable examples, but in this case the example is just TestSuite(X).run(). A user should never run that, and it doesn't explain what it's doing (testing associativity etc.), so the "pros" column for doctests here is empty.

In the cons column,

• The doctest runner is much slower to start than pytest
• There's no way to run only a subset of doctests, so if you are trying to test some new code you can wind up wasting a lot of time
• The doctests have to load the entire sage shell, whereas pytest only needs to import from sagelib what is necessary for the tests in that one file
• Somebody else maintains pytest so we get free improvements
• Ultimately it would be nice to get rid of sage.misc.sage_unittest. This was novel (and extremely nice) in 2009 but now is one of a thousand python unit test frameworks
• Some people probably disagree, but I think it's nicer to have the tests in a separate file so that editing the actual code becomes easier

[user202729]

1. you can use sage syntax in doctest, not so in test*.py file. Which is one of the contribution why this pull request is net + lines of code. (another contributor is the repeated import of TestSuite. What's the pytest warning about anyway?)

I think it's nicer to have the tests in a separate file so that editing the actual code becomes easier

I'm neutral to this. Having documentation far from actual code is a bad idea (people will modify the code and forget about the documentation), but for tests, it's not a problem (if people forget to modify tests, it will fails loudly)

On the other hand, then you need to jump between two files. IDE has split window, but still not as convenient.

The doctest runner is much slower to start than pytest

Is this a corollary of

The doctests have to load the entire sage shell, whereas pytest only needs to import from sagelib what is necessary for the tests in that one file

?

There's no way to run only a subset of doctests, so if you are trying to test some new code you can wind up wasting a lot of time

This is backwards. Ultimately, the "correct" (supported) way to use Sage is to type the commands directly into the command-line, so to test new code, you first type the code directly into the command-line (or jupyter notebook), then copy the whole session into doctest. Then you only run the subset you want without the extra overhead of remembering what's the test name you want to run.

5. Another advantage of pytest is you can also test importing individual modules of sagelib, but currently there are cases where import sage.all works but importing individual module raises circular dependency (if I recalled correctly), this might be more annoying on the short term.

[orlitzky]

1. you can use sage syntax in doctest, not so in test*.py file. Which is one of the contribution why this pull request is net + lines of code. (another contributor is the repeated import of TestSuite. What's the pytest warning about anyway?)

The main reason for the nonnegative line count is that I've left the existing examples in function_field.py after recreating them in the test file. The F.<x> sugar only saves a few lines (and is deeply annoying to anyone trying to write code for the sage library or a new package). But this reminds me of another nit: the sage preparser isn't really maintained, so you can't write doctests that use newer python syntax.

This is the warning that pytest displays if TestSuite is imported at the top level:

collecting ... /home/mjo/.local/lib/python3.13/site-packages/sage/misc/sage_unittest.py:21: PytestCollectionWarning: cannot collect test class 'TestSuite' because it has a init constructor (from: src/sage/rings/function_field/function_field_test.py)

---

The doctest runner is much slower to start than pytest

Is this a corollary of

The doctests have to load the entire sage shell, whereas pytest only needs to import from sagelib what is necessary for the tests in that one file

?

Not entirely. The doctest runner is just big and complicated. When it starts, it does all of this crap:

Features to be detected: 4ti2,SAGE_SRC,benzene,bliss,buckygen,conway_polynomials,coxeter3,csdp,cvxopt,cvxopt,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_cubic_hecke,database_ellcurves,database_graphs,database_jones_numfield,database_knotinfo,dot2tex,dvipng,ecm,flatter,fpylll,fricas,gap_package_atlasrep,gap_package_design,gap_package_grape,gap_package_guava,gap_package_hap,gap_package_polenta,gap_package_polycyclic,gap_package_qpa,gap_package_quagroup,gfan,giac,glucose,graphviz,imagemagick,info,ipython,jmol,jupymake,jupyter_sphinx,kenzo,kissat,latte_int,lrcalc_python,lrslib,mathics,matroid_database,mcqd,meataxe,meson_editable,mpmath,msolve,nauty,networkx,numpy,palp,pandoc,pdf2svg,pdftocairo,pexpect,phitigra,pillow,plantri,polytopes_db,polytopes_db_4d,pplpy,primecountpy,ptyprocess,pycosat,pycryptosat,pynormaliz,pyparsing,python_igraph,requests,rpy2,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.libs.braiding,sage.libs.ecl,sage.libs.flint,sage.libs.gap,sage.libs.giac,sage.libs.homfly,sage.libs.linbox,sage.libs.m4ri,sage.libs.ntl,sage.libs.pari,sage.libs.singular,sage.misc.cython,sage.modular,sage.modules,sage.numerical.mip,sage.plot,sage.rings.complex_double,sage.rings.finite_rings,sage.rings.function_field,sage.rings.number_field,sage.rings.padics,sage.rings.polynomial.pbori,sage.rings.real_double,sage.rings.real_mpfr,sage.sat,sage.schemes,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,scipy,singular,sirocco,sloane_database,sphinx,symengine_py,sympy,tdlib,threejs,topcom

Beyond that, doctests are inherently slow because they can only compare strings. Every test output is converted to a string, and then subjected to a string comparison. If you want to test a method that generates the first 1,000 primes, you can quickly do that with assert(actual == expected). But as a doctest,

sage: actual
[2, 3, 5, 7, ...]

is horrendously slow.

There's no way to run only a subset of doctests, so if you are trying to test some new code you can wind up wasting a lot of time

This is backwards. Ultimately, the "correct" (supported) way to use Sage is to type the commands directly into the command-line, so to test new code, you first type the code directly into the command-line (or jupyter notebook), then copy the whole session into doctest. Then you only run the subset you want without the extra overhead of remembering what's the test name you want to run.

I don't believe you :P

Sage has always been usable as a library. But what I am getting at is that certain tests need to be run many times. Take for example the random_cone() method that I tweaked in a recent PR. The results are random, and it was originally tested by running it a bajillion times. Sitting in front of a sage shell and hitting up, enter, up, enter, up, enter for weeks on end is not really a good use of time. Nor is running sage -t --file-iterations=10000, since that runs all of the tests in cone.py (most of which have nothing to do with my method). What I'd like to do is just re-run the few new tests that I've added.

In practice what I do is recreate the tests in a new python file and call sage -t on that. Being able to select a subset of tests to run would be a more serious solution though.

[user202729]

1. Yes, it's unfortunate that the sage preparser is not maintained, but since it happily ignores what it considers "syntax errors", it mostly works out of the box with new Python versions.

If I understood correctly, you want to say the reason why writing code for a new package is annoying is because if you read Sage from the syntax sugar, you wouldn't know how to write it in non-syntax-sugar way.

While this is valid, it is somewhat backward: if we could write with syntax sugar in both Sage shell and package file, problem would be solved. And the reason why the syntax sugar was invented is because it's more convenient to use the syntax sugar than without.

3. The point about startup being slow is valid, but not pertinent to the current issue: startup time is not counted toward the slow doctest warning.

You can do assert actual == expected in doctest too. But converting a string to a value to string is usually linear time i.e. O(time taken to generate the value), (There may be some O(n^2) or O(Python overhead × n) hidden somewhere, but that isn't inherent.)

4. You can do for i in range(10000): TestSuite(x).run() too.

What else are you doing, for i in {1..1000}; do sage -t file.py; done? Isn't that even slower (you need to wait for the doctest to start up every time)?

[orlitzky]

While this is valid, it is somewhat backward: if we could write with syntax sugar in both Sage shell and package file, problem would be solved. And the reason why the syntax sugar was invented is because it's more convenient to use the syntax sugar than without.

But we can't have the sugar in both places. And I'm not sure that saving one line is worth having the mental model broken in e.g.

sage: x = 3
sage: F.<x> = QQ[]
sage: x
x

where variables are injected into the namespace and existing names are clobbered.

3. The point about startup being slow is valid, but not pertinent to the current issue: startup time is not counted toward the slow doctest warning.

I completely admit that I could have fixed this inside the doctest.

You can do assert actual == expected in doctest too. But converting a string to a value to string is usually linear time i.e. O(time taken to generate the value), (There may be some O(n^2) or O(Python overhead × n) hidden somewhere, but that isn't inherent.)

If you're doing assert actual == expected in the doctests, then they're less helpful as readable examples. I think the string overhead would surprise you. What if I write [2,3,5,7,... instead? Ok, spaces are normalized. How about if I write

[2,
 3,
 5,
 ...

? That's OK too. What if computing the primes displays a warning? It depends, but sometimes we just ignore it. All of these preprocessing steps are performed on a long string. Absolute/relative tolerance is implemented as a regex. The "optional" and "long time" tags themselves are strings that need to be parsed, rather than code. It's all waaay slower than it has to be.

4. You can do `for i in range(10000): TestSuite(x).run()` too.

What else are you doing, for i in {1..1000}; do sage -t file.py; done? Isn't that even slower (you need to wait for the doctest to start up every time)?

A "for" loop only looks nice in this case because the TestSuite method already encapsulates the entire test. No other test will be that simple, and in any case, there will usually be more than one test for a new method.

I generally copy/paste the tests into a new file and use sage -t --file-iterations. A for loop in bash has the same problem that a for loop in sage does -- it keeps chugging even if one of the tests fails.

[user202729]

Anyway, about the pull request—if I understood correctly,

• the change from QQbar to QQ speeds up the test so that it no longer raises the warning.
• the migration from doctest to pytest is orthogonal to the issue.

I'm positive to the former, but negative to the latter.

[tobiasdiez]

I'm surprised that the migration to pytest is such a controversial topic. Since a year or so, pytest is a standard package - and it became standard with the idea to a) make it the default test runner and b) to migrate TEST stuff to pytest (for various reasons). This PR is doing the latter...

[tobiasdiez]

Could you please merge #40474 into this PR (perhaps only temporarily) to check that the pytest tests are indeed working across all system configs.

[orlitzky]

Anyway, about the pull request—if I understood correctly,

* the change from `QQbar` to `QQ` speeds up the test so that it no longer raises the warning.

* the migration from doctest to pytest is orthogonal to the issue.

I'm positive to the former, but negative to the latter.

The tests are still slow, even over QQ. The CI is faster than my laptop, but this is what happens on my laptop if merely reorganize the doctests and cut down on the number of runs:

**********************************************************************
File "src/sage/rings/function_field/function_field.py", line 114, in sage.rings.function_field.function_field
Warning: slow doctest:
    TestSuite(M).run(max_runs=1)  # long time
Test ran for 122.91s cpu, 122.98s wall
Check ran for 0.00s cpu, 0.00s wall
**********************************************************************
File "src/sage/rings/function_field/function_field.py", line 115, in sage.rings.function_field.function_field
Warning: slow doctest:
    TestSuite(N).run(max_runs=1)  # long time
Test ran for 72.05s cpu, 72.07s wall
Check ran for 0.01s cpu, 0.00s wall
    [325 tests, 316.26s wall]

[orlitzky]

Could you please merge #40474 into this PR (perhaps only temporarily) to check that the pytest tests are indeed working across all system configs.

Done. I also spent some time figuring out how to parameterize these tests. There's a little extra boilerplate now but I'm happy with it.

[tobiasdiez]

Looks nicer indeed! There are a few small linter issues. Feel free to set it to positive review after fixing those.

[orlitzky]

• Fixed lint
• Dropped your commits from my branch
• Set positive review

Thanks! I'm going to remove the dependency on #40474 but we should be able to get them merged simultaneously.

[user202729]

The tests are still slow, even over QQ.

in that case, pytest makes it worse. Now if some change makes the test takes 300s instead of 120s, nobody notices. While if a # long time (limit 150s) is added, that would have been caught.

(in other words, this is equivalent to

• a # long time (limit ∞s) (not really, one CI job is limited to 6 hours),
• minus the preparse syntax sugar (which I find convenient),
• plus the "faster to startup" and "can run individual test" (which I don't really care since it's usually just 5s and I can first test newly added code manually in e.g. jupyter notebook and copy the whole thing into TESTS:: later)

)

this problem remains as well:

The tests still get run, still take forever, still cause timeouts, and still cause the test suite to fail

[orlitzky]

The tests are still slow, even over QQ.

in that case, pytest makes it worse. Now if some change makes the test takes 300s instead of 120s, nobody notices. While if a # long time (limit 150s) is added, that would have been caught.

I'm reluctant to argue these points since largely you are right and I risk coming across as petty. That's not my intention, I just like talking about this stuff.

Pytest can output slow test information. No one is using it at the moment, but it's there, and doesn't require the same amount of custom code that the slow doctest warnings do. So at least in theory such a regression could be made just as noticeable and I think it's a big plus that we don't have to maintain it.

But, the format of pytests makes a regression much less likely. The regression was originally introduced because an extra example was added at the end of the module's EXAMPLES:: block. This inadvertently affected the tests because it's a coincidence that TESTS:: comes after EXAMPLES::, but more importantly, all of the module tests and examples implicitly depend on all previous tests and examples. (This is also why it's hard to parallelize doctests.)

The pytest formulation in contrast lists the dependencies between tests explicitly. In my latest commit there are several fixtures and the fixtures they depend on are listed in the function signature, e.g.

@pytest.fixture
def M(K, R, S):
    x = K.gen()
    y = R.gen()
    L = K.extension(y**3 - (x**3 + 2*x*y + 1/x))
    t = S.gen()
    return L.extension(t**2 - x*y)

This makes it clear that changing K, R, or S will change M (and the tests that use it) as well. Conversely, any dependencies between tests/fixtures that are not listed do not exist. Mistakenly changing the base ring of every test in this scenario is much harder.

this problem remains as well:

The tests still get run, still take forever, still cause timeouts, and still cause the test suite to fail

The CI aside, the doctest runner has its own per-file timeout with defaults in sage.doctest.control:

elif options.long:
    options.timeout = int(os.getenv('SAGE_TIMEOUT_LONG', 30 * 60))
else:
    options.timeout = int(os.getenv('SAGE_TIMEOUT', 10 * 60))

These are not an issue with pytest; nor are the "slow doctest" warnings (which I know you have another solution for). At least for real users, using pytest here does lessen the likelihood of a failure due to doctest timeout.

In any case, the argument for moving these to pytest is ultimately that the community has agreed on it:

• Meta-ticket: Adopt mainstream Python testing/linting infrastructure: tox, pytest, ..., describe in Developer's Guide #28936
• https://groups.google.com/g/sage-devel/c/lPLoA7zaoyg
• Replace custom Sage unit test discovery by pytest #30738

In addition to the other benefits, pytest is the standard python testing tool. Everyone knows it, so allowing (potential) new contributors to use it instead of learning the 20 years of quirks and cruft in sage.doctest is a win from that perspective as well.