Opened 7 years ago

Closed 6 years ago

#20182 closed enhancement (fixed)

Automatic doctest for external softwares

Reported by: Kwankyu Lee Owned by:
Priority: major Milestone: sage-7.2
Component: doctest framework Keywords:
Cc: Merged in:
Authors: Kwankyu Lee Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: c4e48bd (Commits, GitHub, GitLab) Commit: c4e48bd22aa1f018436795c14efbdc78a47673e1
Dependencies: Stopgaps:

Status badges

Description (last modified by Kwankyu Lee)

Many doctests depend on softwares external to Sage (including "internet") available on the system, such as latex, magma, mathematica, maple, etc. This patch allows to run the optional doctests for the softwares available.

With the patch,

"sage -t --optional=PKGS": only run doctests including one of the "# optional - xxx" tags where xxx is listed in PKGS;

if "sage" is listed, will also run the standard doctests; if "optional" is listed, will also run tests for installed optional (new-style) packages; if "external" is listed, will also run tests for available external softwares; if set to "all", then all tests will be run

This is a rewrite of #18904 and #13540.

Change History (73)

comment:1 Changed 7 years ago by Kwankyu Lee

Branch: public/20182

comment:2 Changed 7 years ago by git

Commit: a0df81845fd4b85ed411ce322ce879ac84e2df52

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

a0df818Rewrite of automatic doctests for nonpackages

comment:3 Changed 7 years ago by Kwankyu Lee

The patch is a draft or a proof of concept.

comment:4 Changed 7 years ago by Karl-Dieter Crisman

Don't forget internet - or should that be its own ticket, since it's a different kind of non-package? One could presumably do some connectivity test and if it failed, not run the internet tests - they should really nearly always be run.

comment:5 Changed 7 years ago by Jeroen Demeyer

I don't like the fact that you explicitly need to specify --optional=nonpackages. That's almost as much "effort" as writing --optional=latex,magma,internet. Also the name "nonpackages" is a bit strange: why is "octave" a nonpackage?

Also, I don't believe that have_latex() is a sufficient test to check that latex works. You should really run latex to be sure. All the other tests actually run the optional program.

comment:6 Changed 7 years ago by Jeroen Demeyer

Description: modified (diff)

comment:7 Changed 7 years ago by Kwankyu Lee

Description: modified (diff)

The intention is to test the doctests of the form "#optional - xxx" where xxx is not a Sage package. So I think internet is also a "nonpackage" :-)

comment:8 in reply to:  5 ; Changed 7 years ago by Jeroen Demeyer

Replying to jdemeyer:

I don't like the fact that you explicitly need to specify --optional=nonpackages.

To be clear, I meant that nonpackages should be passed by default if no --optional argument is given.

comment:9 Changed 7 years ago by Jeroen Demeyer

It would be cool if this would automatically go over all test functions instead of this ugly error-prone code:

    if test_latex(): pkgs.append('latex')
    if test_magma(): pkgs.append('magma')
    if test_matlab(): pkgs.append('matlab')     
    if test_mathematica(): pkgs.append('mathematica')  
    if test_macaulay2(): pkgs.append('macaulay2')  
    if test_octave(): pkgs.append('octave')  
    if test_scilab(): pkgs.append('scilab')  
    if test_maple(): pkgs.append('maple')
    if test_cplex(): pkgs.append('cplex')  
    if test_gurobi(): pkgs.append('gurobi')

comment:10 Changed 7 years ago by Jeroen Demeyer

You should also consider performance issues. I don't want to run all these tests every time I doctest something. You should look at #13540, which uses a lazy mechanism for testing optional tags.

comment:11 in reply to:  8 ; Changed 7 years ago by Kwankyu Lee

Replying to jdemeyer:

Replying to jdemeyer:

I don't like the fact that you explicitly need to specify --optional=nonpackages.

To be clear, I meant that nonpackages should be passed by default if no --optional argument is given.

Currently doctests for Sage optional packages, if installed on the system, are automatically run only if "--optional" is given. Doctests for nonpackages are run only if "--optional=nonpackages" is given. Perhaps we should keep this ability because of the performance issues that you mention...

comment:12 in reply to:  9 Changed 7 years ago by Kwankyu Lee

Replying to jdemeyer:

It would be cool if this would automatically go over all test functions instead of this ugly error-prone code:

    if test_latex(): pkgs.append('latex')
    if test_magma(): pkgs.append('magma')
    if test_matlab(): pkgs.append('matlab')     
    if test_mathematica(): pkgs.append('mathematica')  
    if test_macaulay2(): pkgs.append('macaulay2')  
    if test_octave(): pkgs.append('octave')  
    if test_scilab(): pkgs.append('scilab')  
    if test_maple(): pkgs.append('maple')
    if test_cplex(): pkgs.append('cplex')  
    if test_gurobi(): pkgs.append('gurobi')

I totally agree! But I needed to write the code before I go to sleep, as I am living on the other side of the globe :-) You can help!

comment:13 Changed 7 years ago by Jeroen Demeyer

I think this is only really useful if "nonpackages" are tested by default.

comment:14 in reply to:  4 Changed 7 years ago by John Palmieri

Replying to kcrisman:

Don't forget internet - or should that be its own ticket, since it's a different kind of non-package? One could presumably do some connectivity test and if it failed, not run the internet tests - they should really nearly always be run.

There were some concerns about running internet tests whenever possible. I'm not sure I agree with those, but maybe there are other reasons not to do these all the time. (What if I lose my internet connection after testing testing connectivity but in the middle of doctesting? Do we have to test connectivity before every internet test?) But I think some of the patchbots could use the internet flag when doctesting.

comment:15 in reply to:  11 Changed 7 years ago by John Palmieri

Replying to klee:

Currently doctests for Sage optional packages, if installed on the system, are automatically run only if "--optional" is given.

I don't think this is correct. When I run doctests without any extra flags, I see (after installing a few optional packages for testing purposes)

$ sage -tp src/sage/homology/
...
Using --optional=ccache,database_gap,gcc,mpir,ore_algebra,python2,sage
...

comment:16 Changed 7 years ago by Volker Braun

I would prefer "external" instead of "nonpackage", otherwise the --optional=nonpackages has a confusing double negation vibe.

comment:17 Changed 7 years ago by Kwankyu Lee

The behavior "--optional" tag has changed after the merged ticket: http://trac.sagemath.org/ticket/18558, but the developer's manual seems not updated completely to reflect the changes. The following is from experiments and the documentation. So would you verify my understanding?

(1) "-t" : tests all ordinary doctests and also doctests about installed optional packages, in effect equivalent to "-t --optional=optional". (2) "-t --optional": no-op (3) "-t --optional=PKGS": only run doctests including one of the "# optional - xxx" tags where xxx is listed in PKGS;

if "sage" is listed will also test the standard doctests; if "optional" is listed will also test all available(installed) optional packages; if set to "all", then all tests will be run;

If this is correct, then

(A) Via this ticket I want to add one more line to (3):

if "external" is listed, then also include all doctests with "# optional - xxx" where xxx is an available external softwares and resources like "magma", "matlab", "internet", "latex", etc (basically everything that is not a Sage standard or optional package).

(B) Also we can change (1) to equate "-t" to "-t --optional=optional,external".

Do you agree?

comment:18 Changed 7 years ago by Karl-Dieter Crisman

This seems reasonable to me as a concept (implementation of (B) shouldn't slow things down ridiculously, though, so the devil may be in the details). Should "-t --optional" be equivalent to "-t" or is the desired behavior still thus?

sage-runtests: error: --optional option requires an argument
Last edited 7 years ago by Karl-Dieter Crisman (previous) (diff)

comment:19 in reply to:  18 Changed 7 years ago by Kwankyu Lee

Replying to kcrisman:

This seems reasonable to me as a concept (implementation of (B) shouldn't slow things down ridiculously, though, so the devil may be in the details).

Agree. We definitely need to implement a lazy mechanism like that in #13540. I am reading the related codes in Sage.

Should "-t --optional" be equivalent to "-t" or is the desired behavior still thus?

sage-runtests: error: --optional option requires an argument

Once we implement "external", "all" will mean "sage"+"optional"+"external". Then we may set "-t --optional" to mean "-t --optional=all".

comment:20 Changed 7 years ago by git

Commit: a0df81845fd4b85ed411ce322ce879ac84e2df52126e4e50e2270ba43b3cfd026f0405f568fff9be

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

61b5136Merge Sage 7.1.rc0 into trac20182
126e4e5Initial implementaion using the lazyset mechanism

comment:21 Changed 7 years ago by Kwankyu Lee

Authors: Kwankyu Lee
Description: modified (diff)
Priority: minormajor
Summary: Automatic doctest for nonpackagesAutomatic doctest for external softwares

comment:22 Changed 7 years ago by git

Commit: 126e4e50e2270ba43b3cfd026f0405f568fff9be2098dd0b60081231baa95475b879f2a38cfc6f55

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

2098dd0Fixed gross logical bugs

comment:23 Changed 7 years ago by git

Commit: 2098dd0b60081231baa95475b879f2a38cfc6f5582d58ebf89b13e168af232dc4f10e92fe48ee205

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

82d58ebFixed a bug and the doc

comment:24 Changed 7 years ago by git

Commit: 82d58ebf89b13e168af232dc4f10e92fe48ee205b95936b7af18809d2c169da70ad14a9456b0f09b

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

b95936bFixed a typo

comment:25 Changed 7 years ago by git

Commit: b95936b7af18809d2c169da70ad14a9456b0f09b20f692a848b3b373ab555c7dcddffb1c63e03361

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

ca3f0d9Modified the help text for --optional
4a0ec28Corrected the help text for --optional
c56e94fCorrected the help text a bit
20f692aAdded more doctests

comment:26 Changed 7 years ago by git

Commit: 20f692a848b3b373ab555c7dcddffb1c63e0336139e7390be155409d19a2b972a1af860f24e86b40

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

39e7390Fixed a doctest failure

comment:27 Changed 7 years ago by git

Commit: 39e7390be155409d19a2b972a1af860f24e86b40ee257470f2bf355efff9065b4d6da6ac4b23829e

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

ee25747Trimmed the codes and removed whitespaces

comment:28 Changed 7 years ago by Kwankyu Lee

Description: modified (diff)

comment:29 Changed 7 years ago by Jeroen Demeyer

Why does LazySet inherit from set? Are you actually using any set functionality?

comment:30 Changed 7 years ago by John Palmieri

If I do sage -tp --optional=sage,optional,external ..., then instead of printing

Using --optional=...,external,...

it would be nice if it printed

Using --optional=...,internet,latex,maple,...

comment:31 Changed 7 years ago by git

Commit: ee257470f2bf355efff9065b4d6da6ac4b23829e86a7e6f8cb3845aa4551422941ee081cfc46f713

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

86a7e6fRemoved set inheritance of LazySet

comment:32 in reply to:  29 Changed 7 years ago by Kwankyu Lee

Replying to jdemeyer:

Why does LazySet inherit from set? Are you actually using any set functionality?

No. I was not. See the last commit.

comment:33 in reply to:  30 ; Changed 7 years ago by Kwankyu Lee

Replying to jhpalmieri:

If I do sage -tp --optional=sage,optional,external ..., then instead of printing

Using --optional=...,external,...

it would be nice if it printed

Using --optional=...,internet,latex,maple,...

Hmmm. It seems possible... But what do you think it means that "maple" appears in the list? Does it mean that maple is available and tests depending on it will be tested. This behavior defeats the lazy mechanism. Does it simply mean that any doctests depending on maple will run if maple turns out to be available lazily and not otherwise? Then this is very different from an optional package, say gcc or mpir, appearing in the list.

comment:34 in reply to:  33 ; Changed 7 years ago by John Palmieri

Replying to klee:

Replying to jhpalmieri:

If I do sage -tp --optional=sage,optional,external ..., then instead of printing

Using --optional=...,external,...

it would be nice if it printed

Using --optional=...,internet,latex,maple,...

Hmmm. It seems possible... But what do you think it means that "maple" appears in the list? Does it mean that maple is available and tests depending on it will be tested. This behavior defeats the lazy mechanism. Does it simply mean that any doctests depending on maple will run if maple turns out to be available lazily and not otherwise?

I say no to the last question. I don't know how to balance the laziness with what I'm looking for. Maybe a summary at the end of doctesting of which external packages were detected, or (even better) the intersection of those external packages which were detected and those for which a test was run. I think it is important to provide feedback about this in order to debug failed tests (or slow tests, etc.).

Last edited 7 years ago by John Palmieri (previous) (diff)

comment:35 in reply to:  34 Changed 7 years ago by Kwankyu Lee

I don't know how to balance the laziness with what I'm looking for. Maybe a summary at the end of doctesting of which external packages were detected, or (even better) the intersection of those external packages which were detected and those for which a test was run.

Because of the lazyset mechanism, at least one test was run for an external software detected in most cases. There may be exceptional cases to this, but is this important? By the way, I prefer "external software" than "external package". Internet is rather a software than a package :-)

I think it is important to provide feedback about this in order to debug failed tests (or slow tests, etc.).

I agree. Perhaps we can list the detected softwares at the end of doctesting as you suggested.

comment:36 Changed 7 years ago by git

Commit: 86a7e6f8cb3845aa4551422941ee081cfc46f713f58377af2e7a7ac9e08335a42e52c31736e49626

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

60bc96dReimplement for parallel testing
245e473Merged with the previous commit
f58377aThe tag all also turns on external

comment:37 Changed 7 years ago by git

Commit: f58377af2e7a7ac9e08335a42e52c31736e49626f4df0b843b9afcc8eebd7dd6267bf5045481a8f0

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

f4df0b8Cleaned up util

comment:38 Changed 7 years ago by Kwankyu Lee

The latest patch copes with the multiprocessing of Sage doctesting.

comment:39 Changed 7 years ago by Jeroen Demeyer

This is silly:

raise Exception # should not happen!

Replace it by something like

raise AssertionError("invalid value for self.seen")

comment:40 Changed 7 years ago by Jeroen Demeyer

Can you add a timeout (using alarm()) for this:

urllib.request.urlopen("http://www.sagemath.org")

comment:41 Changed 7 years ago by git

Commit: f4df0b843b9afcc8eebd7dd6267bf5045481a8f06d360016e067bc0035aac4a9c324e371953360f9

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

6d36001Improved the raise statement

comment:42 Changed 7 years ago by git

Commit: 6d360016e067bc0035aac4a9c324e371953360f9781796004587c55aaa385754f69498c1cecfea0d

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

61c3727Add timeout to internet test
7817960Fixed a doctest failure

comment:43 Changed 7 years ago by John Palmieri

On my computer, doctesting the file external.py causes two windows to open up, showing the results from view('2+3'). Try to find another way to test whether latex is present and functional, maybe something like this:

  • src/sage/doctest/external.py

    diff --git a/src/sage/doctest/external.py b/src/sage/doctest/external.py
    index f05bc8d..463413a 100644
    a b def has_latex(): 
    5959        sage: has_latex() # random
    6060        True
    6161    """
    62     from sage.misc.latex import view
     62    from sage.misc.latex import _run_latex_, _latex_file_
     63    from sage.misc.temporary_file import tmp_filename
    6364    try:
    64         view('2+3')
     65        f = tmp_filename(ext='.tex')
     66        O = open(f, 'w')
     67        O.write(_latex_file_('3'))
     68        O.close()
     69        _run_latex_(f)
    6570        return True
    6671    except Exception:
    6772        return False

Also, you might consider adding ImageMagick (and in particular the convert command) to the set of external programs to be tested.

Last edited 7 years ago by John Palmieri (previous) (diff)

comment:44 Changed 7 years ago by John Palmieri

Also, I'm getting a lot of failed internet doctests, and some for latex, too. If we enable testing those by default, Sage won't pass doctests anymore. They need to be fixed: on separate tickets?

comment:45 in reply to:  44 Changed 7 years ago by Karl-Dieter Crisman

Also, I'm getting a lot of failed internet doctests, and some for latex, too. If we enable testing those by default, Sage won't pass doctests anymore. They need to be fixed: on separate tickets?

Yes. Like #16302. Are the ones (OEIS) in #16252 at least still fixed?

comment:46 Changed 7 years ago by John Palmieri

I had to make this change in oeis.py to fix one failure:

  • src/sage/databases/oeis.py

    diff --git a/src/sage/databases/oeis.py b/src/sage/databases/oeis.py
    index ff88286..b8de905 100644
    a b class OEISSequence(SageObject): 
    841841            A000045: Fibonacci numbers: F(n) = F(n-1) + F(n-2) with F(0) = 0 and F(1) = 1.
    842842
    843843            sage: f.keywords()                          # optional -- internet
    844             ('core', 'nonn', 'nice', 'easy', 'hear')
     844            ('core', 'nonn', 'nice', 'easy', 'hear', 'changed')
    845845
    846846        TESTS::
    847847

comment:47 Changed 7 years ago by John Palmieri

I think that the internet doctests have to be robust enough that if an external website changes something, our tests will still pass. I tried to argue for this at #16252, but there was disagreement. I think it's either that or we don't enable internet testing by default.

comment:48 Changed 7 years ago by git

Commit: 781796004587c55aaa385754f69498c1cecfea0d4dc6df1f078cd58a8cc1c0f25dd941b94dbe5664

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

fc0b885Improved latex detection
4dc6df1Fixed a bug; trimmed the docs

comment:49 in reply to:  43 Changed 7 years ago by Kwankyu Lee

Replying to jhpalmieri:

Also, you might consider adding ImageMagick (and in particular the convert command) to the set of external programs to be tested.

For this ticket, I will stop at providing the framework for tests requiring external softwares. Thus

(1) Any expert is cordially invited to contribute commits for better and new has_xxx tester for the software xxx.

(2) Turning on external by default should be dealt with in another ticket.

(3) Fixing all doctest failures that result from changing the default should be dealt with separate tickets.

comment:50 Changed 7 years ago by git

Commit: 4dc6df1f078cd58a8cc1c0f25dd941b94dbe566482b20cac6b47ecebc3852a21bfad1142635a73bd

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

82b20caAdded a doctest

comment:51 Changed 7 years ago by Jeroen Demeyer

Or perhaps you should indeed define make (p)testall(long) to use --optional=sage,optional,external

comment:52 Changed 7 years ago by git

Commit: 82b20cac6b47ecebc3852a21bfad1142635a73bd36b4f7aefaab9453600f93e0bce43998e19da50d

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

36b4f7aDefine make (p)testall(long) to use --optional=sage,optional,external

comment:53 in reply to:  51 Changed 7 years ago by Kwankyu Lee

Replying to jdemeyer:

Or perhaps you should indeed define make (p)testall(long) to use --optional=sage,optional,external

Done. What about make (p)testoptional(long)? Perhaps --optional=sage,optional?

comment:54 in reply to:  47 Changed 7 years ago by Karl-Dieter Crisman

I think that the internet doctests have to be robust enough that if an external website changes something, our tests will still pass. I tried to argue for this at #16252, but there was disagreement. I think it's either that or we don't enable internet testing by default.

Agreed. (In your example, presumably 'core' in f.keywords() would be better?) But I also think we must enable the internet testing within some framework people will test relatively often - if make testall is that place, that's fine. Otherwise untested is broken - between OEIS and some of the timeseries stuff there is probably a lot eventually broken.

comment:55 Changed 7 years ago by git

Commit: 36b4f7aefaab9453600f93e0bce43998e19da50d6d7d3d5b9bb1f1ba3e068fdd5a3766a5415dd41c

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

6d7d3d5Define make (p)testoptional(long) to use --optional=sage,optional

comment:56 Changed 7 years ago by Kwankyu Lee

Status: newneeds_review

comment:57 Changed 7 years ago by Kwankyu Lee

Milestone: sage-7.1sage-7.2

comment:58 Changed 7 years ago by Kwankyu Lee

Why the patchbot is not working on this ticket? Since I made changes on Makefile?

comment:59 Changed 6 years ago by Julian Rüth

I am working on a more generic way of checking for features of the system (including nice error messages) at #20382. klee: What do you think about it? You could potentially rebase your code to use the features there or I could do that once #20182 got merged.

comment:60 Changed 6 years ago by Kwankyu Lee

As this ticket is related to #20382, but independent from it. I think this ticket could be reviewed and merged independently, though still no one bothers reviewing it. Then I would be happy that you open a new ticket to use your features once #20382 got merged.

If #20382 is merged first, then I would also be very happy that you rebase this ticket to #20382.

comment:61 Changed 6 years ago by John Palmieri

This doctest should be marked "random":

        sage: available_softwares.seen()
        ['internet', 'latex', 'magma']

Also, throughout, "softwares" should be "software" -- "which software do you have installed" is correct, for example, not "which softwares do you have installed".

comment:62 Changed 6 years ago by John Palmieri

What should happen with failing doctests? klee's opinion is that they should be fixed on another ticket. I guess that's okay as long as they are not run by default. Anyone else have opinions?

For example:

sage -t src/sage/plot/graphics.py
**********************************************************************
File "src/sage/plot/graphics.py", line 1624, in sage.plot.graphics.Graphics.show
Failed example:
    plot(x, typeset='latex') # optional - latex
Expected nothing
Got:
    Graphics object consisting of 1 graphics primitive
**********************************************************************
File "src/sage/plot/graphics.py", line 1630, in sage.plot.graphics.Graphics.show
Failed example:
    plot(x, typeset='type1') # optional - latex
Expected nothing
Got:
    Graphics object consisting of 1 graphics primitive
**********************************************************************
1 item had failures:
   2 of  87 in sage.plot.graphics.Graphics.show

I get this at the end of make ptestall:

----------------------------------------------------------------------
sage -t src/sage/plot/graphics.py  # 2 doctests failed
sage -t src/sage/plot/plot.py  # 1 doctest failed
sage -t src/sage/arith/misc.py  # 1 doctest failed
sage -t src/sage/tests/cmdline.py  # 1 doctest failed
sage -t src/sage/dev/sagedev.py  # 2 doctests failed
sage -t src/sage/structure/sage_object.pyx  # 2 doctests failed
sage -t src/sage/combinat/designs/bibd.py  # 1 doctest failed
sage -t src/sage/misc/package.py  # 6 doctests failed
sage -t src/sage/doctest/external.py  # 1 doctest failed
sage -t src/sage/repl/rich_output/pretty_print.py  # 1 doctest failed
sage -t src/sage/dev/trac_interface.py  # 5 doctests failed
sage -t src/sage/repl/load.py  # 2 doctests failed
sage -t src/doc/en/developer/coding_basics.rst  # 1 doctest failed
sage -t src/sage/dev/patch.py  # 3 doctests failed
sage -t src/sage/databases/oeis.py  # 2 doctests failed
sage -t src/sage/combinat/integer_lists/invlex.pyx  # 1 doctest failed
sage -t src/sage/dev/digest_transport.py  # 1 doctest failed
sage -t src/sage/finance/stock.py  # 2 doctests failed
sage -t src/sage/misc/remote_file.py  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 1929.7 seconds
    cpu time: 3833.1 seconds
    cumulative wall time: 7184.9 seconds
External softwares detected for doctesting: internet,latex
Last edited 6 years ago by John Palmieri (previous) (diff)

comment:63 Changed 6 years ago by Vincent Delecroix

minor remark: there is a timeout optional argument for urlopen (that will gives you a URLError in case of failure). And in this particular test, why are you catching Exception?

comment:64 in reply to:  62 Changed 6 years ago by Kwankyu Lee

Replying to jhpalmieri:

What should happen with failing doctests? klee's opinion is that they should be fixed on another ticket. I guess that's okay as long as they are not run by default.

This patch can be safely merged as long as the patchbot and the people checking a new Sage release use make ptestlong, which I think is the current practice. Then we need to open a ticket for the failing doctests for each external software, before we at some point switch to use make ptestall for a new release.

comment:65 Changed 6 years ago by git

Commit: 6d7d3d5b9bb1f1ba3e068fdd5a3766a5415dd41cb243bd6365aebb2bac096eedf262ed961a862f07

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

306067aMinor fixes and URLError for internet timeout
b243bd6Merge branch Sage 7.2.beta4 into trac20182

comment:66 Changed 6 years ago by John Palmieri

Every function and method needs documentation and doctests.

$ sage --coverage src/sage/doctest/external.py 
------------------------------------------------------------------------
SCORE src/sage/doctest/external.py: 64.7% (11 of 17)

Missing documentation:
     * line 272: def __init__(self)
     * line 277: def __contains__(self, item)

Missing doctests:
     * line 224: def external_software()
     * line 236: def _lookup(software)
     * line 294: def issuperset(self, other)
     * line 303: def seen(self)
------------------------------------------------------------------------

comment:67 Changed 6 years ago by git

Commit: b243bd6365aebb2bac096eedf262ed961a862f07308643a1d25c8683d1945d94f5ae182035f32025

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

308643aAdd and trim docstrings

comment:68 Changed 6 years ago by git

Commit: 308643a1d25c8683d1945d94f5ae182035f3202508aca6e4b275abe7f86adffc6deeb047a68dedab

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

08aca6etrac 20182: minor rewording, remove "random" from one doctest

comment:69 Changed 6 years ago by John Palmieri

Here are a few minor rewordings. The only possibly significant change is removing "# random" from one doctest, but the same thing is doctested later in the same file (in AvailableSoftware) without that marking.

I am happy with this now. Does anyone else have any comments?

comment:70 Changed 6 years ago by git

Commit: 08aca6e4b275abe7f86adffc6deeb047a68dedabc4e48bd22aa1f018436795c14efbdc78a47673e1

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

69bcf21Change one doctest to be nonrandom
c4e48bdMerge with other modifications

comment:71 Changed 6 years ago by Kwankyu Lee

The list "external_software" should not be random. For convenience of later modification, I changed one of the duplicate doctests. Thank you for elaborate review!

comment:72 Changed 6 years ago by John Palmieri

Reviewers: John Palmieri
Status: needs_reviewpositive_review

comment:73 Changed 6 years ago by Volker Braun

Branch: public/20182c4e48bd22aa1f018436795c14efbdc78a47673e1
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.