Opened 11 years ago
Closed 10 years ago
#11615 closed enhancement (fixed)
optional doctests: clean up and document "known bug", "optional: requires PKG"
Reported by:  jhpalmieri  Owned by:  mvngu 

Priority:  minor  Milestone:  sage5.0 
Component:  doctest coverage  Keywords:  
Cc:  leif  Merged in:  sage5.0.beta10 
Authors:  John Palmieri  Reviewers:  KarlDieter Crisman 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
Marking a doctest # known bug
means that it is skipped unless the optional
flag is passed to sage t
. This needs to be documented.
Also, many doctests in Sage are marked # optional  requires PKG
, and this means that sage t onlyoptional=PKG ...
does not run the test: the word "requires" is interpreted as a package name. This is not ideal.
The attached patch to the scripts repo removes the word "requires" (and also "needs") automatically. It also allows a colon or comma instead of a hyphen, as in # optional: requires PKG
. It also converts tests marked as "known bug" to tests marked as "optional bug", so they are run when you do sage t onlyoptional=bug ...
. The attached patch to the main Sage library documents all of this.
Apply
 trac_11615scriptsrepo.patch to the scripts repo
 trac_11615documentknownbug.patch to the main Sage library repo
Attachments (2)
Change History (13)
comment:1 Changed 11 years ago by
 Description modified (diff)
 Status changed from new to needs_review
comment:2 Changed 11 years ago by
comment:3 Changed 11 years ago by
 Cc leif added
This should perhaps be added to the metaticket #11337.
comment:4 Changed 10 years ago by
 Reviewers set to KarlDieter Crisman
 Status changed from needs_review to needs_info
I like this, and it still applies!
Here is something to ask. Could have two if
statements (instead of some sort of elif
or something) cause trouble if a doctest was marked as optional and a bug? I've tried to make something break, but I'm not sure if this is the "right" behavior or not. Here is my example.
Adding doctest
sage: 2+2 # optional  known bug 5
to a file gives
$ ../../sage t onlyoptional=known,bug ../../devel/sage/sage/symbolic/expression_conversions.py sage t onlyoptional=known,bug "devel/sage/sage/symbolic/expression_conversions.py" [0.1 s]  All tests passed! Total time for all tests: 0.1 seconds
and the same happens with onlyoptional=bug
, but it fails (correctly) with optional
.
Otherwise the patch seems fine, and doc looks good (naturally!).
comment:5 Changed 10 years ago by
 Status changed from needs_info to needs_review
Good point. I've changed the code. Now if I doctest this function:
def testing(): """ EXAMPLES:: sage: 2+2 # optional  known bug 5 sage: 2+2 # known bug 6 sage: 2+2 # optional 7 """ pass
I get three failures with sage t ... optional
and two failures with sage t ... onlyoptional=bug
.
comment:6 followup: ↓ 7 Changed 10 years ago by
Okay, so it looks like # known bug
is run by sage t optional
. I think this was clear before, but I didn't realize it for some reason. I feel like this should be made more clear in the documentation in the other patch. ``sage t onlyoptional=bug ...``
isn't the only way this can be revealed, that is. Just
so it is also detected by ``onlyoptional=bug``.) +so it is detected by ``onlyoptional=bug`` and also ``optional``.)
Here is a very very corner case.
sage: 2+2 # optional  magma bug 7
This only runs with onlyoptional=magma,bug
. I guess that's ok.
What do you think about these two things? Maybe neither one is an issue.
Otherwise I do think this is in pretty good shape.
comment:7 in reply to: ↑ 6 Changed 10 years ago by
Replying to kcrisman:
Okay, so it looks like
# known bug
is run bysage t optional
. I think this was clear before, but I didn't realize it for some reason. I feel like this should be made more clear in the documentation in the other patch.
I can do that.
Here is a very very corner case.
sage: 2+2 # optional  magma bug 7
This only runs with
onlyoptional=magma,bug
.
This is the old behavior: marking a doctest # optional X Y Z
means it should only be run with onlyoptional=X,Y,Z
. So I don't think we need to add anything about it.
comment:8 Changed 10 years ago by
Ok, great. I hope I tested this enough.
comment:9 Changed 10 years ago by
Here is a new documentation patch, which just makes the following change:

doc/en/developer/conventions.rst
diff git a/doc/en/developer/conventions.rst b/doc/en/developer/conventions.rst
a b mind: 880 880 Then the doctest will be skipped by default, but could be revealed 881 881 by running ``sage t onlyoptional=bug ...``. (A doctest marked 882 882 as ``known bug`` gets automatically converted to ``optional bug``, 883 so it is also detected by ``o nlyoptional=bug``.)883 so it is also detected by ``optional`` or ``onlyoptional=bug``.) 884 884 885 885  If the entire documentation string contains all three words 886 886 ``optional``, ``package``, and ``installed``, then the entire
comment:10 Changed 10 years ago by
 Status changed from needs_review to positive_review
Thanks, this should be a good addition.
Hmm, Trac upgrade smells nice so far. And still seems leftaligned, nice.
comment:11 Changed 10 years ago by
 Merged in set to sage5.0.beta10
 Resolution set to fixed
 Status changed from positive_review to closed
By the way, the patch to the scripts library also fixes a bug: if you have a doctest marked
and run
sage t onlyoptional=pkg1,pkg2 ...
, the test would not be run: the functiononly_optional_include
in sagedoctest would convert "pkg1 pkg2" to "pkg1pkg2" with no spaces, so it would only be run withsage t optional
orsage t onlyoptional=pkg1pkg2
. With the new version, the string gets converted to'pkg1,pkg2'
, and then split into['pkg1', 'pkg2']
.Previously, you could use the undocumented
and it would work. With the patch, this no longer works unless there is white space between pkg1 and pkg2 (commas are still allowed). I couldn't find any instances of tags separated by commas but no spaces in the Sage library. The patch to the Sage library clarifies the documentation, saying that you need spaces between the package names.