Open tobiasdiez opened 2 years ago
implementing an importlib.Loader for
.sage
files, which is then used by pytest
see #27074
Replying to @mkoeppe:
implementing an importlib.Loader for
.sage
files, which is then used by pytestsee #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?
@kiwifb How did you manage to keep the temporary .sage
file that is generated in these doctests?
Description changed:
---
+++
@@ -17,7 +17,7 @@
Got:
4
-As analyzed in #33531 comment:9, this is due to pytest running on this temprorary sage file failing with
+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 ===================================================================
Branch pushed to git repo; I updated commit sha1. New commits:
af92e19 | Prefix test method so that pytest finds it |
I went through the steps to produce it manually from a sage session. And then printed its name to have a look at it.
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.
Description changed:
---
+++
@@ -32,3 +32,11 @@
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.
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!
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.
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
Replying to @mkoeppe:
This actually looks very nice and perhaps the classes
SageSourceFinder
,SageSourceLoader
could find a place somewhere insage.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
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.
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?
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.
What would be an easier solution to register the sage file loader then?
Just skipping .sage files because there is no existing practice of having pytest tests in them.
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.
Sure, it's fine.
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 |
Branch has merge conflicts
We fix
As analyzed in #33531 comment:9, this is due to pytest running on the temporary file
my_script.sage
and failing withIn 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
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