Opened 10 years ago

Closed 9 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: sage-5.0
Component: doctest coverage Keywords:
Cc: leif Merged in: sage-5.0.beta10
Authors: John Palmieri Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jhpalmieri)

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 -only-optional=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 -only-optional=bug .... The attached patch to the main Sage library documents all of this.


Apply

Attachments (2)

trac_11615-scripts-repo.patch (1.9 KB) - added by jhpalmieri 9 years ago.
scripts repo
trac_11615-document-known-bug.patch (2.0 KB) - added by jhpalmieri 9 years ago.
Sage library

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by jhpalmieri

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 10 years ago by jhpalmieri

By the way, the patch to the scripts library also fixes a bug: if you have a doctest marked

    sage: 2+2 # optional: pkg1 pkg2
    17

and run sage -t -only-optional=pkg1,pkg2 ..., the test would not be run: the function only_optional_include in sage-doctest would convert "pkg1 pkg2" to "pkg1pkg2" with no spaces, so it would only be run with sage -t -optional or sage -t -only-optional=pkg1pkg2. With the new version, the string gets converted to 'pkg1,pkg2', and then split into ['pkg1', 'pkg2'].

Previously, you could use the undocumented

   sage: 2+2 # optional: pkg1,pkg2
   17

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.

comment:3 Changed 10 years ago by leif

  • Cc leif added

This should perhaps be added to the meta-ticket #11337.

comment:4 Changed 9 years ago by kcrisman

  • Reviewers set to Karl-Dieter 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 -only-optional=known,bug ../../devel/sage/sage/symbolic/expression_conversions.py 
sage -t -only-optional=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 -only-optional=bug, but it fails (correctly) with -optional.

Otherwise the patch seems fine, and doc looks good (naturally!).

comment:5 Changed 9 years ago by jhpalmieri

  • 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 ... --only-optional=bug.

Changed 9 years ago by jhpalmieri

scripts repo

comment:6 follow-up: Changed 9 years ago by kcrisman

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 --only-optional=bug ...`` isn't the only way this can be revealed, that is. Just

-so it is also detected by ``--only-optional=bug``.)
+so it is detected by ``--only-optional=bug`` and also ``--optional``.)

Here is a very very corner case.

        sage: 2+2 # optional -- magma bug
        7

This only runs with --only-optional=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 9 years ago by jhpalmieri

Replying to kcrisman:

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.

I can do that.

Here is a very very corner case.

        sage: 2+2 # optional -- magma bug
        7

This only runs with --only-optional=magma,bug.

This is the old behavior: marking a doctest # optional X Y Z means it should only be run with --only-optional=X,Y,Z. So I don't think we need to add anything about it.

comment:8 Changed 9 years ago by kcrisman

Ok, great. I hope I tested this enough.

comment:9 Changed 9 years ago by jhpalmieri

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: 
    880880  Then the doctest will be skipped by default, but could be revealed
    881881  by running ``sage -t --only-optional=bug ...``.  (A doctest marked
    882882  as ``known bug`` gets automatically converted to ``optional bug``,
    883   so it is also detected by ``--only-optional=bug``.)
     883  so it is also detected by ``--optional`` or  ``--only-optional=bug``.)
    884884
    885885-  If the entire documentation string contains all three words
    886886   ``optional``, ``package``, and ``installed``, then the entire

Changed 9 years ago by jhpalmieri

Sage library

comment:10 Changed 9 years ago by kcrisman

  • 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 left-aligned, nice.

comment:11 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.0.beta10
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.