Closed mkoeppe closed 2 years ago
Description changed:
---
+++
@@ -1,2 +1,6 @@
This new option would test all Sage modules installed (in `SAGE_LIB`) instead of `SAGE_SRC`. This would be useful for testing modularized distributions.
+(Actually, because portions of namespace packages can be installed in separate locations, the variable `sage.env.SAGE_LIB` is no longer meaningful. Instead, we make use of the `__path__` attribute of packages, https://python.readthedocs.io/en/stable/reference/import.html#module-path, which in the case of namespace packages is an iterable.)
+
+Part of Meta-ticket #33037.
+
Author: Matthias Koeppe
Description changed:
---
+++
@@ -1,4 +1,21 @@
-This new option would test all Sage modules installed (in `SAGE_LIB`) instead of `SAGE_SRC`. This would be useful for testing modularized distributions.
+This new option is similar to `--all` but it tests all Sage modules installed (in `SAGE_LIB`) instead of `SAGE_SRC`. This will be useful for testing modularized distributions.
+
+```
+$ ./sage -tp --installed
+too many failed tests, not using stored timings
+Running doctests with ID 2022-02-23-21-52-47-11831639.
+Using --optional=ccache,cryptominisat,debugpy,e_antic,homebrew,igraph,jupymake,lrslib,normaliz,pip,polytopes_db_4d,pycryptosat,pynormaliz,sage,sage_spkg
+Features to be detected: [...]
+Doctesting all installed modules of the Sage library.
+Sorting sources by runtime so that slower doctests are run first....
+Doctesting 3522 files using 8 threads.
+sage -t --random-seed=334799075370772165291245032923424842623 local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/groups/matrix_gps/heisenberg.py
+ [32 tests, 1.83 s]
+sage -t --random-seed=334799075370772165291245032923424842623 local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/combinat/crystals/alcove_path.py
+ [293 tests, 4.16 s]
+...
+```
+
(Actually, because portions of namespace packages can be installed in separate locations, the variable `sage.env.SAGE_LIB` is no longer meaningful. Instead, we make use of the `__path__` attribute of packages, https://python.readthedocs.io/en/stable/reference/import.html#module-path, which in the case of namespace packages is an iterable.)
Description changed:
---
+++
@@ -16,8 +16,11 @@
...
+(Actually, because portions of namespace packages can be installed in separate locations, the variable sage.env.SAGE_LIB
is no longer meaningful. Instead, we make use of the __path__
attribute of packages, https://python.readthedocs.io/en/stable/reference/import.html#module-path, which in the case of namespace packages is an iterable.)
-(Actually, because portions of namespace packages can be installed in separate locations, the variable sage.env.SAGE_LIB
is no longer meaningful. Instead, we make use of the __path__
attribute of packages, https://python.readthedocs.io/en/stable/reference/import.html#module-path, which in the case of namespace packages is an iterable.)
+This would also make sense for distribution packaging, which no longer would have to rely on the fallback SAGE_SRC
-> SAGE_LIB
.
+
+
Part of Meta-ticket #33037.
New commits:
de649be | src/sage/doctest/control.py, src/bin/sage-runtests: Implement sage -t --installed |
I think the move makes some sense especially in the context of the elimination of SAGE_LIB
and potentially more variables as sage matures more in the future.
The only thing I am not sure is introducing a new option. Why not make it the default instead? What workflow would that prevent?
Replying to @kiwifb:
Why not make it the default instead? What workflow would that prevent?
I think the default is valuable because doctests can be written / updated and immediately tested without having to build the Sage library.
Valid for pure python files but not .pyx
, you need to build those before testing them. But on the numbers they probably are the minority.
Replying to @kiwifb:
Valid for pure python files but not
.pyx
, you need to build those before testing them.
When you are done changing code, you can still edit doctests and in this phase you don't need to recompile in order to tests the doctests.
Replying to @mkoeppe:
Replying to @kiwifb:
Valid for pure python files but not
.pyx
, you need to build those before testing them.When you are done changing code, you can still edit doctests and in this phase you don't need to recompile in order to tests the doctests.
More to the point, it is a pain in the workflow to recompile in that case. But you should explicitly test a file or directory in that case. But let's go with a developer focused for now.
We may want to suggest not to use --installed
and --all
at the same time for distros as it will try to add a non-existent path once we remove the default SAGE_SRC
-> SAGE_LIB
. Also the current --all
will catch sage_setup
, sage_docbuild
and SAGE_DOC_SRC
(which in distros will point to SAGE_DOC
). But --installed
doesn't do any of that. Should it?
Replying to @kiwifb:
We may want to suggest not to use
--installed
and--all
at the same time
Yes, they are mutually exclusive options
Branch pushed to git repo; I updated commit sha1. New commits:
5dd2dd2 | src/sage/doctest/control.py: In sage -t --installed, also handle sage_setup, sage_docbuild |
Replying to @kiwifb:
Also the current
--all
will catchsage_setup
,sage_docbuild
andSAGE_DOC_SRC
(which in distros will point toSAGE_DOC
). But--installed
doesn't do any of that. Should it?
I have added sage_setup
, sage_docbuild
but I'm not sure about SAGE_DOC_SRC
because Sage does not install it; do you install it in the Gentoo package?
No, I don't install SAGE_DOC_SRC. But as I said it points to the installed SAGE_DOC
which effectively tests nothing. Because the point would be to test the .rst
files, but all those are installed as .rst.txt
for some reason. It would make sense to me to add SAGE_DOC
for --installed
if we could test those.
Ah, I see, in the _sources
subdirectories
I looked at these files but they do not seem useful
What else do you test in SAGE_DOC_SRC
? conf.py
files? Those do not have doctests either. If there is no value in testing a .rst.txt
in SAGE_DOC
, I am not sure there is value in testing the corresponding .rst
in SAGE_DOC_SRC
.
None of these files have TESTS
blocks but a few have stuff that does get tested
$ sage -t --long src/doc/en/thematic_tutorials/sandpile.rst
too many failed tests, not using stored timings
Running doctests with ID 2022-02-25-13-31-30-ea5e5e11.
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,pandoc,pdf2svg,pdftocairo,plantri,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 --long --random-seed=249406959745805344364707629509298952414 src/doc/en/thematic_tutorials/sandpile.rst
[705 tests, 12.50 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Yes, my mistake
Branch pushed to git repo; I updated commit sha1. New commits:
07f37d0 | src/sage/doctest/{control,sources}.py: Handle .rst.txt files like .rst files |
Branch pushed to git repo; I updated commit sha1. New commits:
e837614 | src/sage/doctest/control.py: In sage -t --installed, also test SAGE_DOC |
Description changed:
---
+++
@@ -1,4 +1,6 @@
-This new option is similar to `--all` but it tests all Sage modules installed (in `SAGE_LIB`) instead of `SAGE_SRC`. This will be useful for testing modularized distributions.
+This new option is similar to `--all` but
+- it tests all Sage modules installed (in `SAGE_LIB`) instead of `SAGE_SRC`. (This will be useful for testing modularized distributions.)
+- it tests the installed Sage documentation (specifically, the `.rst.txt` files installed in `html/**/_sources`) instead of `SAGE_DOC_SRC`.
$ ./sage -tp --installed
Description changed:
---
+++
@@ -20,7 +20,7 @@
(Actually, because portions of namespace packages can be installed in separate locations, the variable `sage.env.SAGE_LIB` is no longer meaningful. Instead, we make use of the `__path__` attribute of packages, https://python.readthedocs.io/en/stable/reference/import.html#module-path, which in the case of namespace packages is an iterable.)
-This would also make sense for distribution packaging, which no longer would have to rely on the fallback `SAGE_SRC` -> `SAGE_LIB`.
+This would also make sense for distribution packaging, which no longer would have to rely on the fallbacks `SAGE_SRC` -> `SAGE_LIB` and `SAGE_DOC_SRC -> SAGE_DOC`.
Branch pushed to git repo; I updated commit sha1. New commits:
f1c4ee7 | src/sage/doctest/sources.py: Update doctest output |
Should I remove the fallback SAGE_DOC_SRC
-> SAGE_DOC
?
Reviewer: François Bissey
Replying to @mkoeppe:
Should I remove the fallback
SAGE_DOC_SRC
->SAGE_DOC
?
Let's leave it for another clean up ticket. I am already quite happy that we fixed the .rst.txt
thing in this one. I'll want a clean slate in my brain to think properly of all the consequences when we get to it.
It was good work on this one, thank you for all the commits :)
If you have nothing to add, it is a positive review from me.
Branch pushed to git repo; I updated commit sha1. New commits:
1627cc9 | src/sage/doctest/control.py: Fall back from --all to --installed explicitly |
Here's another change, just stopping short of actually removing the fallbacks
But we can leave this for another ticket if you prefer.
Well, I'll be going for the school run in 20-25 minutes, I'll try to wrap my head around what this commit is about before going.
Branch pushed to git repo; I updated commit sha1. New commits:
e80c135 | src/sage/doctest/control.py: Use elif because options are exclusive |
Good spotting that last commit. The previous commit is neat, it almost negates the need for having the --installed
option. I can continue doing --all
on distro and it will behave as --installed
. It is only distinctive on vanilla sage where it does something different.
It feels like I won my first point about making it the default without you really conceding anything. Really well done. The code reorganisation around it is also quite good. It is all more clear about what each part does.
Replying to @kiwifb:
The previous commit is neat, it almost negates the need for having the
--installed
option.
Yes, for the Sage-on-distro use case, but not for my use case (doctesting a modularized subset distribution like sagemath-polyhedra).
Description changed:
---
+++
@@ -1,5 +1,5 @@
This new option is similar to `--all` but
-- it tests all Sage modules installed (in `SAGE_LIB`) instead of `SAGE_SRC`. (This will be useful for testing modularized distributions.)
+- it tests all Sage modules installed (in `SAGE_LIB`) instead of `SAGE_SRC`. (This will be useful for testing modularized distributions such as #32601.)
- it tests the installed Sage documentation (specifically, the `.rst.txt` files installed in `html/**/_sources`) instead of `SAGE_DOC_SRC`.
Thank you!
Test fail
Branch pushed to git repo; I updated commit sha1. New commits:
b4d2b8e | src/sage/doctest/control.py: Update doctest output |
Apologies, that was very sloppy of me.
Patchbots have been on strike, hard to see failures.
This new option is similar to
--all
butSAGE_LIB
) instead ofSAGE_SRC
. (This will be useful for testing modularized distributions such as #32601.).rst.txt
files installed inhtml/**/_sources
) instead ofSAGE_DOC_SRC
.(Actually, because portions of namespace packages can be installed in separate locations, the variable
sage.env.SAGE_LIB
is no longer meaningful. Instead, we make use of the__path__
attribute of packages, https://python.readthedocs.io/en/stable/reference/import.html#module-path, which in the case of namespace packages is an iterable.)This would also make sense for distribution packaging, which no longer would have to rely on the fallbacks
SAGE_SRC
->SAGE_LIB
andSAGE_DOC_SRC -> SAGE_DOC
.Part of Meta-ticket #33037.
CC: @kiwifb @antonio-rojas @tobiasdiez
Component: doctest framework
Author: Matthias Koeppe
Branch/Commit:
b4d2b8e
Reviewer: François Bissey, Antonio Rojas
Issue created by migration from https://trac.sagemath.org/ticket/33407