Preparse sage files before passing them to pytest

[tobiasdiez]

We fix

$ ./sage -tp src/sage/tests/cmdline.py
too many failed tests, not using stored timings
Running doctests with ID 2022-03-20-11-46-21-1dc8fb11.
Using --optional=4ti2,buckygen,ccache,cryptominisat,debugpy,e_antic,gap_packages,homebrew,igraph,jupymake,latte_int,libsemigroups,lidia,lrslib,meataxe,normaliz,pip,polytopes_db_4d,pynormaliz,sage,sage_spkg,texttable
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 using 8 threads.
sage -t --random-seed=23402257756506608859254049173870803147 src/sage/tests/cmdline.py
**********************************************************************
File "src/sage/tests/cmdline.py", line 312, in sage.tests.cmdline.test_executable
Failed example:
    ret
Expected:
    0
Got:
    4

As analyzed in #33531 comment:9, this is due to pytest running on the temporary file my_script.sage and failing with

=================================================================== 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 [])

In other words, pytest doesn't know how to analyze .sage files. We fix this by letting pytest know that .sage files are essentially normal python files after they run through the preparser (this is accomplished by implementing an importlib.Loader for .sage files, which is then used by pytest).

Sadly, this doesn't completely fix the issue and the above test in cmdline still fails. However, the error is now

ERROR: not found: /home/tobias/.sage/temp/DESKTOP-MRB66LP/15784/dir_5qrixjen/my_script.sage
(no name '/home/tobias/.sage/temp/DESKTOP-MRB66LP/15784/dir_5qrixjen/my_script.sage' in any of [])

which comes from some issues of pytesting files outside of the sage source directory. In fact, running sage -t on a copy of the offending file somewhere in /src works.

CC: @mkoeppe @tornaria @kiwifb

Component: doctest framework

Author: Tobias Diez

Branch/Commit: public/tests/pytest_sage_files @ 24a0b17

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

[mkoeppe]

implementing an importlib.Loader for .sage files, which is then used by pytest

see #27074

[tobiasdiez]

comment:2

Replying to @mkoeppe:

implementing an importlib.Loader for .sage files, which is then used by pytest

see #27074

Thanks for the pointer. The code of this ticket could indeed be helpful for #27074. What would be a good place to put the added importlib loader so that the can easily be reused in #27074?

[tobiasdiez]

comment:3

@kiwifb How did you manage to keep the temporary .sage file that is generated in these doctests?

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

af92e19

Prefix test method so that pytest finds it

[sagetrac-git]

Changed commit from 8a79630 to af92e19

[kiwifb]

comment:6

I went through the steps to produce it manually from a sage session. And then printed its name to have a look at it.

[mkoeppe]

comment:7

Replying to @tobiasdiez:

The code of this ticket could indeed be helpful for #27074. What would be a good place to put the added importlib loader so that the can easily be reused in #27074?

For #27074, the thought was that this should work globally so that users can use it with their local scripts. But last time I was looking into #27074, I came across an old discussion that Python does NOT support this (and wontfix) - installing such a global handler. Sorry, can't find it right now.

[tobiasdiez]

comment:9

Replying to @kiwifb:

I went through the steps to produce it manually from a sage session. And then printed its name to have a look at it.

Okay thanks, that worked!

[tobiasdiez]

comment:10

Replying to @mkoeppe:

Replying to @tobiasdiez:

The code of this ticket could indeed be helpful for #27074. What would be a good place to put the added importlib loader so that the can easily be reused in #27074?

For #27074, the thought was that this should work globally so that users can use it with their local scripts. But last time I was looking into #27074, I came across an old discussion that Python does NOT support this (and wontfix) - installing such a global handler. Sorry, can't find it right now.

Okay, then I would propose to keep the changes localized in conftest and if someone is working on #27074 it can be easily extracted.

[mkoeppe]

comment:11

This actually looks very nice and perhaps the classes SageSourceFinder, SageSourceLoader could find a place somewhere in sage.repl? (Modularization context: #29941)

The actual activation (sys.meta_path.append(SageSourceFinder())) would stay where it is, in conftest.py

[tobiasdiez]

comment:12

Replying to @mkoeppe:

This actually looks very nice and perhaps the classes SageSourceFinder, SageSourceLoader could find a place somewhere in sage.repl? (Modularization context: #29941)

The actual activation (sys.meta_path.append(SageSourceFinder())) would stay where it is, in conftest.py

That would also work, but as long as they are only used for pytests I think it would make sense to leave them in conftest.py

[mkoeppe]

comment:13

No, the idea would be that we would not want to activate it by just loading the Sage library; but we may want to advertise it to sufficiently adventurous users.

[tobiasdiez]

comment:14

Replying to @mkoeppe:

No, the idea would be that we would not want to activate it by just loading the Sage library; but we may want to advertise it to sufficiently adventurous users.

Don't you think that goes way beyond the scope of this ticket and would need proper documentation and more testing?

[mkoeppe]

comment:15

Good point, but note that we don't actually have any .sage files in our test suite.

What you are trying to solve is a failure from testing the behavior of the Sage doctester on user files. And for this the metapath manipulation seems to be an overkill.

[tobiasdiez]

comment:16

What would be an easier solution to register the sage file loader then?

[mkoeppe]

comment:17

Just skipping .sage files because there is no existing practice of having pytest tests in them.

[tobiasdiez]

comment:18

But they can have "normal" doctests and for #33546 supporting this would be needed anyway. In fact, this ticket here is meant as an easy precursor to #33546, testing the grounds for custom test collectors without opening the box of doctest parsing.

[mkoeppe]

comment:19

Sure, it's fine.

[sagetrac-git]

Changed commit from af92e19 to 24a0b17

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

db1d505	Fix tests in sageinspect
a09d5ec	Fix test_long
7060ea1	Fix imports in modular decomposition
dec7b7b	Fix doctest in rings
1b7da1b	Merge remote-tracking branch 'trac/public/tests/doctests_pytest' into public/tests/pytest_wrong_test_methods
3cab51d	Fix spelling
e6312ec	Add docs to newly added pytest hook
6768bd2	src/sage/tests/cmdline.py: Restore dropped line in doctest
7644854	Fix typo
24a0b17	Merge remote-tracking branch 'origin/public/tests/pytest_wrong_test_methods' into public/tests/pytest_sage_files

[mkoeppe]

Branch has merge conflicts