pytest in sage is broken, please make it really optional

[tornaria]

• pytest support was introduced in Add minimal pytest configuration #31003 and merged on the basis that it was a no-op
• Later in sage.numerical.backends: Replace use of TestSuite by pytest #31103 some code was added at the end of src/bin/sage-runtests to run pytest (see 719e442)
• This code is broken in many ways, some that have been fixed (Fix exit code for pytest in case of no matched files #32892) and some that have not (sage -t: Do not run pytest on individual Python files unless they match the pytest file pattern #31924, also #31924 comment:23)
• The bug in sage -t: Do not run pytest on individual Python files unless they match the pytest file pattern #31924 is a PITA and the only way to avoid it is to patch out the code that runs pytest in `sage-runtests
• Result is: if pytest is not installed, we get an annoying warning that induces to install it, but if pytest is installed then doctesting is broken (as in we get scary warnings at the end and a non-zero exit code, on top of a 3s overhead)

$ time sage -t src/sage/all.py ; echo $?
Running doctests with ID 2022-03-19-12-07-01-0e558614.
Using --optional=build,pip,sage,sage_spkg,void
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,pandoc,pdf2svg,plantri,pynormaliz,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.plot,sage.rings.number_field,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib
Doctesting 1 file.
sage -t --warn-long 60.2 --random-seed=67815784290108767013007019285883868229 src/sage/all.py
    [13 tests, 0.75 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.8 seconds
    cpu time: 0.7 seconds
    cumulative wall time: 0.7 seconds
Features detected for doctesting: 
============================================================================ test session starts =============================================================================
platform linux -- Python 3.10.3, pytest-6.2.5, py-1.10.0, pluggy-0.13.1
rootdir: /opt/sage/sage-git/develop/src, configfile: tox.ini
plugins: anyio-3.5.0, xonsh-0.11.0
collected 0 items / 1 error                                                                                                                                                  

=================================================================================== ERRORS ===================================================================================
________________________________________________________________________ ERROR collecting sage/all.py ________________________________________________________________________
src/sage/all.py:315: in <module>
    sage.misc.lazy_import.finish_startup()
sage/misc/lazy_import.pyx:87: in sage.misc.lazy_import.finish_startup (build/cythonized/sage/misc/lazy_import.c:1995)
    ???
sage/misc/lazy_import.pyx:103: in sage.misc.lazy_import.finish_startup (build/cythonized/sage/misc/lazy_import.c:1929)
    ???
E   AssertionError: finish_startup() must be called exactly once
========================================================================== short test summary info ===========================================================================
ERROR src/sage/all.py - AssertionError: finish_startup() must be called exactly once
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
============================================================================== 1 error in 0.34s ==============================================================================

real	0m6.917s
user	0m6.496s
sys	0m0.486s
2

Expected behaviour (this is what happens when pytest is patched out in sage-runtests):

$ time sage -t src/sage/all.py ; echo $?
Running doctests with ID 2022-03-19-12-07-20-a46d44e6.
Using --optional=build,pip,sage,sage_spkg,void
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,pandoc,pdf2svg,plantri,pynormaliz,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.plot,sage.rings.number_field,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib
Doctesting 1 file.
sage -t --warn-long 60.2 --random-seed=339422732397923267068138499972932430419 src/sage/all.py
    [13 tests, 0.75 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.8 seconds
    cpu time: 0.8 seconds
    cumulative wall time: 0.8 seconds
Features detected for doctesting: 

real	0m4.115s
user	0m3.783s
sys	0m0.323s
0

My proposal:

• add a feature flag "pytest" and make running pytest conditional to that
• do NOT add pytest to the default flags until all issues are fixed and it is known to work ok (at the very least sage -t: Do not run pytest on individual Python files unless they match the pytest file pattern #31924 should be fixed)

In fact pytest should only run when there's some file *_test.py in the list of files, and only in those files. I think DC already traversed the filesystem in search of files, so instead of having pytest traverse the filesystem again, just filter the list of files that DC obtained for *_test.py and run pytest just on those and only if there's some.

Even if/after pytest integration is fixed, there are advantages to place it behind a feature flag so there's an easy way to run doctests skipping pytest (just add --optional=sage or whatever else one wants to test, leaving out pytest)

Depends on #31924

CC: @tobiasdiez @mkoeppe

Component: doctest framework

Reviewer: Gonzalo Tornaría

Issue created by migration from https://trac.sagemath.org/ticket/33531

[tornaria]

comment:1

This is what I have in mind (warning, this patch ignores whitespace to make it human-readable so it won't apply):

     DC = DocTestController(args, args.filenames)
     err = DC.run()
 
+    if "pytest" in DC.options.optional:
         try:
             exit_code_pytest = 0
             import pytest
@@ -163,5 +164,5 @@ if __name__ == "__main__":
 
         if err == 0:
             sys.exit(exit_code_pytest)
-    else:
+
     sys.exit(err)

A proper patch (with whitespace changes) is:

--- a/src/bin/sage-runtests
+++ b/src/bin/sage-runtests
@@ -147,21 +147,22 @@ if __name__ == "__main__":
     DC = DocTestController(args, args.filenames)
     err = DC.run()
 
-    try:
-        exit_code_pytest = 0
-        import pytest
-        pytest_options = ["--import-mode", "importlib"]
-        if args.verbose:
-            pytest_options.append("-v")
-        exit_code_pytest = pytest.main(pytest_options + args.filenames)
-        if exit_code_pytest == 5:
-            # Exit code 5 means there were no test files, pass in this case
+    if "pytest" in DC.options.optional:
+        try:
             exit_code_pytest = 0
-
-    except ModuleNotFoundError:
-        print("Pytest is not installed, skip checking tests that rely on it.")
-
-    if err == 0:
-        sys.exit(exit_code_pytest)
-    else:
-        sys.exit(err)
+            import pytest
+            pytest_options = ["--import-mode", "importlib"]
+            if args.verbose:
+                pytest_options.append("-v")
+            exit_code_pytest = pytest.main(pytest_options + args.filenames)
+            if exit_code_pytest == 5:
+                # Exit code 5 means there were no test files, pass in this case
+                exit_code_pytest = 0
+
+        except ModuleNotFoundError:
+            print("Pytest is not installed, skip checking tests that rely on it.")
+
+        if err == 0:
+            sys.exit(exit_code_pytest)
+
+    sys.exit(err)

[tornaria]

Branch: u/tornaria/pytest-optional

[tornaria]

Commit: f5608aa

[tornaria]

comment:2

And here's the commit, ready for review.

Once pytest is working reliably, "pytest" can be added to the list of default features in _get_optional_defaults() so it's run by default. In any case using e.g. --optional=sage will skip running pytest.

---

New commits:

f5608aa

trac #33531: don't run pytest by default

[tornaria]

Author: Gonzalo Tornaría

[mkoeppe]

comment:3

This looks like a good fix

[tobiasdiez]

comment:4

Currently pytest is an optional package, so you have to install it manually and only in that case the tests are run. Why do you think an additional switch is necessary?

Even if/after pytest integration is fixed, there are advantages to place it behind a feature flag so there's an easy way to run doctests skipping pytest

The long term goal is to use pytest also for run the doctests (completely removing sage's custom doctest runner). So I don't like to introduce switches that differentiate between doctests and "pytests".

The error that occurs while testing the all module should be fixed with #30748 (ready for review for months).

[mkoeppe]

comment:5

#30748 does not describe itself as fixing a bug.

[mkoeppe]

Dependencies: #32975

[tornaria]

comment:7

Replying to @tobiasdiez:

Currently pytest is an optional package, so you have to install it manually and only in that case the tests are run. Why do you think an additional switch is necessary?

Maybe pytest is installed because it's needed for another purpose. In my case, sage uses system python packages and pytest was installed in the system for whatever reason. I don't even remember, but it's kind of surprising (and annoying) that doctesting breaks because of that.

My proposal is to make the choice to run pytest explicit rather than implicit because one just happens to have pytest installed, at least while you are developing it.

Even if/after pytest integration is fixed, there are advantages to place it behind a feature flag so there's an easy way to run doctests skipping pytest

The long term goal is to use pytest also for run the doctests (completely removing sage's custom doctest runner). So I don't like to introduce switches that differentiate between doctests and "pytests".

If/when that happens, this will be easy to re evaluate.

The error that occurs while testing the all module should be fixed with #30748 (ready for review for months).

That's just one random example. Doctesting any files is broken, since the logic in sage-runtests will always call pytest with all given arguments rather than only the files that need to be run by pytest.

Replying to @mkoeppe:

Dependencies set to #32975

It looks to me independent. That ticket doesn't seem to touch the logic in sage-runtests.

[tobiasdiez]

comment:8

I can see that it's annoying that some things are not working perfectly. On the other hand, I think its also important to get experience with pytest and find these little issues that popup in different settings. Thus, I'm not a big fan of hiding pytest behind two switches.

Doctesting any files is broken, since the logic in sage-runtests will always call pytest with all given arguments rather than only the files that need to be run by pytest.

Yes, pytest is always run on these files (if you use -t). But normally it's a no-op. There are two things that can potentially go wrong:

• the import of these modules is not working (that is what happened with sage.all)
• pytest finds test methods that are actually no pytests (that is what happens with tests.commandline)

The first issue should be fixed anyway because one would encounter the same issues when using sage as a library (which is pushed by the modularization effort). And the second issue should be fixed with #31924.

Currently, pytest is invoked as part of some github actions runs and this should also be kept to not lower the test coverage. So one would need to pass the pytest switch there as well. Moreover, I think the documentation needs to be adapted, too.

[kiwifb]

comment:9

I'll add an amusing failure I have now linked to pytest in sage-on-gentoo to the list of issues to fix. I hope #31924 will cover it. I have noticed the following doctest failure on and off

sage -t --long --random-seed=22919418159437026337026400262276501942 /usr/lib/python3.10/site-packages/sage/tests/cmdline.py
**********************************************************************
File "/usr/lib/python3.10/site-packages/sage/tests/cmdline.py", line 308, in sage.tests.cmdline.test_executable
Failed example:
    ret
Expected:
    0
Got:
    4

This only happens if pytest is available and found. To put things into perspective this is the code section tested with all you need to know to reproduce the behavior

    Testing ``sage --preparse FILE`` and ``sage -t FILE``.  First create
    a file and preparse it::

        sage: s = "# -*- coding: utf-8 -*-\n'''This is a test file.\nAnd I am its doctest'''\ndef my_add(a):\n    '''\n    Add 2 to a.\n\n        EXAMPLES::\n\n            sage: my_add(2)\n            4\n        '''\n    return a + 2\n"
        sage: script = os.path.join(tmp_dir(), 'my_script.sage')
        sage: script_py = script + '.py'
        sage: F = open(script, 'w')
        sage: _ = F.write(s)
        sage: F.close()
        sage: (out, err, ret) = test_executable(["sage", "--preparse", script])
        sage: ret
        0
        sage: os.path.isfile(script_py)
        True

    Now test my_script.sage and the preparsed version my_script.sage.py::

        sage: (out, err, ret) = test_executable(["sage", "-t", script])
        sage: ret
        0

Why is it happening? Well I manually ran the above line with the temporary script

$ sage -t /home/fbissey/.sage/temp/tarazed/1679924/dir_v9uzl0t4/my_script.sage
too many failed tests, not using stored timings
Running doctests with ID 2022-03-21-14-28-37-40dde3a2.
Using --optional=pip,sage
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,nauty,palp,pandoc,pdf2svg,pdftocairo,plantri,polytopes_db,polytopes_db_4d,pynormaliz,python_igraph,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.plot,sage.rings.number_field,sage.rings.padics,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib
Doctesting 1 file.
sage -t --random-seed=330965964155200917833729525818621666531 /home/fbissey/.sage/temp/tarazed/1679924/dir_v9uzl0t4/my_script.sage
    [1 test, 0.00 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 0.0 seconds
    cpu time: 0.0 seconds
    cumulative wall time: 0.0 seconds
Features detected for doctesting: 
=================================================================== test session starts ===================================================================
platform linux -- Python 3.10.2, pytest-7.0.1, pluggy-1.0.0
rootdir: /home/fbissey
plugins: hypothesis-6.38.0
collected 0 items                                                                                                                                         

================================================================== no tests ran in 0.00s ==================================================================
ERROR: not found: /home/fbissey/.sage/temp/tarazed/1679924/dir_v9uzl0t4/my_script.sage
(no name '/home/fbissey/.sage/temp/tarazed/1679924/dir_v9uzl0t4/my_script.sage' in any of [])

The doctest test doctesting and it ends up with a classic #31924 kind of failure.

[tornaria]

comment:10

Replying to @tobiasdiez:

I can see that it's annoying that some things are not working perfectly. On the other hand, I think its also important to get experience with pytest and find these little issues that popup in different settings.

But these are not "little issues", they actually break sage -t unless I patch out the code that runs pytest at the end of sage-runtests. Worse: this will go unnoticed if one works in a box where pytest is not installed, only to break when working in a box where pytest happens to be installed.

It's absolutely crucial that one can pass individual files to sage -t. When one asks it to doctest a file that is NOT using pytest, it should NOT call pytest at all.

Thus, I'm not a big fan of hiding pytest behind two switches.

All I'm asking is for ONE switch to run pytest. I suggested that it defaults to OFF while all the "little issues" are ironed out, and that it could default to ON later.

Doctesting any files is broken, since the logic in sage-runtests will always call pytest with all given arguments rather than only the files that need to be run by pytest.

Yes, pytest is always run on these files (if you use -t). But normally it's a no-op. There are two things that can potentially go wrong:

• the import of these modules is not working (that is what happened with sage.all)
• pytest finds test methods that are actually no pytests (that is what happens with tests.commandline)

The first issue should be fixed anyway because one would encounter the same issues when using sage as a library (which is pushed by the modularization effort). And the second issue should be fixed with #31924.

Also:
a. running pytest on a file with no pytests adds a completely unnecessary 3s overhead.
b. running pytest on a directory scans the whole tree again, but DC has already scanned the tree.
c. e.g. doctesting sage.doctest.test ranges between "broken" and "takes 2x time" when pytest is installed.

If the current standing is that *_test.py files go through pytest, the rest of files go through DC, can't you get the list of files from DC, filter out the ones ending in _test.py and just pass those to pytest?

Currently, pytest is invoked as part of some github actions runs and this should also be kept to not lower the test coverage. So one would need to pass the pytest switch there as well. Moreover, I think the documentation needs to be adapted, too.

That sounds a good idea, those github actions need to be aware of pytest anyway (as in: install optional package pytest, etc) so you may as well turn the "pytest switch" on while it defaults to off. This gives you the opportunity and freedom to develop pytest integration without fear of breaking standard doctesting. I'd say it's better not move tests from the sage doctest framework to pytest framework until everything is working, pytest is a standard packages, and the "pytest switch" defaults to on.

[mkoeppe]

comment:11

I have a fix in #31924, please review

[mkoeppe]

Changed dependencies from #32975 to #31924

[mkoeppe]

comment:12

I believe #31924 makes this unnecessary.

[mkoeppe]

comment:13

Is there still brokenness, or can we close this ticket?

[tornaria]

comment:14

I'm running with undef SAGE_ROOT which makes SAGE_SRC=SAGE_LIB (normally the python site-packages).

After #33521, this means pytest is never run, so the issue is gone 👍️.

OTOH, if I actually set SAGE_SRC to point to the actual sage source, pytest breaks with the following error:

pluggy._manager.PluginValidationError: Plugin '/home/tornaria/src/sage/sage-git/src/conftest.py' for hook 'pytest_collect_file'
hookimpl definition: pytest_collect_file(file_path: 'Path', parent: 'pytest.File') -> 'pytest.Collector | None'
Argument(s) {'file_path'} are declared in the hookimpl but can not be found in the hookspec

This may be because pytest is too old in void linux (6.2.5, instead of 7.x).

If I comment out the definition of pytest_collect_file in src/conftest.py then it works with an exception I document below.

However, I couldn't help but notice that pytest will use a single thread to run, which is a HUGE regression if tests start migrating from the sage doctest framework to pytest !!! Right now running pytest through all these 9 files takes 80s, while the doctest step in --long --all mode takes just 312s wall time (using 36 threads).

---

Out of all the files *_test.py in sage source tree (9 files), the only one that fails to pytest is sage/structure/sage_object_test.py which gives me the following error:

=================================================================================== ERRORS ===================================================================================
____________________________________________________________ ERROR collecting sage/structure/sage_object_test.py _____________________________________________________________
ImportError while importing test module '/home/tornaria/src/sage/sage-git/src/sage/structure/sage_object_test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
src/sage/structure/sage_object_test.py:3: in <module>
    from .sage_object import SageObject
E   ImportError: attempted relative import with no known parent package
========================================================================== short test summary info ===========================================================================

This goes away if I change the import to be absolute (sage.structure.sage_object), but then no items are collected from this file, I'm not sure that's expected.

[mkoeppe]

comment:15

I have added a comment to #33546 and opened #33783.

Maybe Tobias could comment on the pytest version needed?

[mkoeppe]

Changed author from Gonzalo Tornaría to none

[mkoeppe]

Reviewer: Gonzalo Tornaría

[mkoeppe]

comment:16

I take 👍️ as positive review

[tornaria]

Changed branch from u/tornaria/pytest-optional to none

[tornaria]

comment:17

Replying to @mkoeppe:

I take 👍️ as positive review

Rather, that we can close this ticket since #33521 made the issue go away, there's nothing left to merge here, and you picked up the other issues in new tickets (thanks).

I don't think I can actually close the ticket though. Can you?

[tornaria]

Changed commit from f5608aa to none

[mkoeppe]

comment:18

Replying to @tornaria:

Replying to @mkoeppe:

I take 👍️ as positive review

Rather, that we can close this ticket since #33521 made the issue go away,

Yes, that's what positive review with the milestone "sage-duplicate/invalid/wontfix" indicates. It's not really necessary to remove the branch; it won't be accidentally merged.