sage -fixdoctests doesn't work correctly

[mderickx]

This patch fixes the following problems with the command line fixdoctests option:

• correctly deals with tests for which output it expected but none is generated, or for which no output is expected by some is generated.
• makes fixdoctests play nicely with the changes introduced in #12415
• adds some tests to sage/tests/cmdline.py
• makes the script work slightly better with tests that produce output with varying indentation
• cleans up the code of sage-fixdoctests
• sage-fixdoctests now checks to see if it can write to the specified output directory and if not it prompts the user for a better place to put the file
• the script now takes an optional argument --long so that fixdoctests can also massage --long doctests
• there is an updated usage message for fixdoctests printed by sage --advanced
• there is an updated usage message for fixdoctests in the reference manual

---

Original ticket:

To reproduce this remove all but the first test result from the patch at: #10568

maarten-derickxs-macbook-pro:sage maarten$ sage -t matrix/matrix_sparse.pyx
sage -t  "devel/sage-main/sage/matrix/matrix_sparse.pyx"    
**********************************************************************
File "/Applications/sage-4.6.rc0/devel/sage-main/sage/matrix/matrix_sparse.pyx", line 301:
    sage: (2/3)*M
Expected nothing
Got:
    [   0  2/3  4/3    2  8/3 10/3]
    [   4 14/3 16/3    6 20/3 22/3]
    [   8 26/3 28/3   10 32/3 34/3]
**********************************************************************
File "/Applications/sage-4.6.rc0/devel/sage-main/sage/matrix/matrix_sparse.pyx", line 302:
    sage: 7*M
Expected nothing
Got:
    [  0   7  14  21  28  35]
    [ 42  49  56  63  70  77]
    [ 84  91  98 105 112 119]
**********************************************************************
File "/Applications/sage-4.6.rc0/devel/sage-main/sage/matrix/matrix_sparse.pyx", line 303:
    sage: (1/4)*M
Expected nothing
Got:
    [   0  1/4  1/2  3/4    1  5/4]
    [ 3/2  7/4    2  9/4  5/2 11/4]
    [   3 13/4  7/2 15/4    4 17/4]
**********************************************************************
File "/Applications/sage-4.6.rc0/devel/sage-main/sage/matrix/matrix_sparse.pyx", line 306:
    sage: m==(97/42)*(42/97*m)
Expected nothing
Got:
    True
**********************************************************************
1 items had failures:
   4 of   9 in __main__.example_6
***Test Failed*** 4 failures.
For whitespace errors, see the file /Users/maarten/.sage//tmp/.doctest_matrix_sparse.py
 [12.5 s]
 
----------------------------------------------------------------------
The following tests failed:


sage -t  "devel/sage-main/sage/matrix/matrix_sparse.pyx"
Total time for all tests: 12.6 seconds
maarten-derickxs-macbook-pro:sage maarten$ sage -fixdoctests matrix/matrix_sparse.pyx

The bug was caused by the script not allowing for the cases when either the expected or computed returns from the doctests were empty. The attached patch does this properly and also cleans up the script a little (well, a lot, actually).

Note that the script lives in $SAGE_ROOT/local/bin which is not part of the standard repository. The (new) doctests for the script live in sage/tests/cmdline.py

Apply: trac_10589--doctests_for_fixdoctests-am.patch​

Apply to scripts repository: trac_10589--fixdoctest_failures-am.patch​ (modifies $SAGE_ROOT/local/bin)

Applt to root repository: trac_10589--updating_fixdoctests_command_line_options-am.patch​ (modifies $SAGE_ROOT/spkg/bin)

[jhpalmieri]

To doctest this, you could add some tests to devel/sage/sage/tests/cmdline.py

[andrew.mathas]

Replying to jhpalmieri:

To doctest this, you could add some tests to devel/sage/sage/tests/cmdline.py

Thanks John. I will have a look at this and add some tests.

[andrew.mathas]

Updated fixdoctests patch so that it works with the new doctest framework in #12415 and added some tests to sage/tests/cmdline.py for the script

[roed]

Shouldn't that be the scripts repo?

[andrew.mathas]

Replying to roed:

Shouldn't that be the scripts repo?

Yes, thanks. Wasn't sure what the right name was, just knew where to find it...

[benjaminfjones]

I used the patch here to solve problems I was getting with sage -fixdoctests. Thanks, hope this can get reviewed and merged soon!

[tscrim]

Looks good to me minus two minors thing:

• you're missing the :: on line 405 in tests/cmdline.py,
• could you de-indent the AUTHORS: block in sage-fixdoctests and align the extra text by 2 spaces.

Thanks Andrew.

[andrew.mathas]

Replying to tscrim:

Looks good to me minus two minors thing:

• you're missing the :: on line 405 in tests/cmdline.py,
• could you de-indent the AUTHORS: block in sage-fixdoctests and align the extra text by 2 spaces.

Thanks Travis, I have fixed both of these.

Andrew

[tscrim]

Then it's a positive review. Thanks Andrew.

[jdemeyer]

The file trac_10589--fixdoctest_failures-am.patch​ contains garbage at the start, please fix that.

[andrew.mathas]

Replying to jdemeyer:

The file trac_10589--fixdoctest_failures-am.patch​ contains garbage at the start, please fix that.

Sorry. Fixed.

[jdemeyer]

I get

sage -t --long devel/sage/sage/tests/cmdline.py
**********************************************************************
File "devel/sage/sage/tests/cmdline.py", line 414, in sage.tests.cmdline.test_executable
Failed example:
    ret
Expected:
    0
Got:
    1
**********************************************************************
File "devel/sage/sage/tests/cmdline.py", line 418, in sage.tests.cmdline.test_executable
Failed example:
    print output[output.find('     SAGE: 1+1'):output.find(' \"\"\"')-1]
Expected:
         SAGE: 1+1              # incorrect output
    -    3
    +    2
         SAGE: m=matrix(ZZ,3)   # output when none is expected
    +    SAGE: (2/3)*m          # no output when it is expected
    +    [0 0 0]
         [0 0 0]
         [0 0 0]
    -    [1 0 0]
    -    SAGE: (2/3)*m          # no output when it is expected
         SAGE: mu=PartitionTuple([[4,4],[3,3,2,1],[1,1]])   # output when none is expected
    -    [4, 4, 3, 3, 2, 1, 1]
         SAGE: mu.pp()          # uneven indentation
    -    ****
    -    ****
    +       ****   ***   *
    +       ****   ***   *
    +              **
    +              *
         SAGE: PartitionTuples.global_options(convention="French")
         SAGE: mu.pp()         # fix doctest with uneven indentation
    +    *
    +    **
    +    ****   ***   *
    +    ****   ***   *
         SAGE: PartitionTuples.global_options.reset()
Got:
    <BLANKLINE>
**********************************************************************

[andrew.mathas]

Replying to jdemeyer:

I get

sage -t --long devel/sage/sage/tests/cmdline.py
**********************************************************************
File "devel/sage/sage/tests/cmdline.py", line 414, in sage.tests.cmdline.test_executable
Failed example:
   
 ...

Hi Jeroen,

I am fairly sure that this failure is because the patch in the scripts directory has not been applied - at least, I get this error if the patch trac_10589--fixdoctest_failures-am.patch has not been applied to the scripts repository and I do not get it when it is applied.

Can you please confirm that both patches have been applied correctly. I'm putting the status back to positive review as I am not sure if you will read this otherwise.

Andrew

[jdemeyer]

I'm pretty sure I did apply both patches, but I'll try again...

[jdemeyer]

After adding a doctest line

sage: print err

(which I guess all cmdline.py tests should have):

**********************************************************************
File "devel/sage/sage/tests/cmdline.py", line 416, in sage.tests.cmdline.test_executable
Failed example:
    print err
Expected nothing
Got:
    sh: cannot create .tmp: Permission denied
    Traceback (most recent call last):
      File "/scratch/release/merger/sage-5.13.beta0/local/bin/sage-fixdoctests", line 35, in <module>
        doc_out = open(doc_out).read()
    IOError: [Errno 2] No such file or directory: '.tmp'
    <BLANKLINE>
**********************************************************************

So the problem is that sage -fixdoctests writes to the current working directory, which isn't guaranteed to be writable.

Also: please use --fixdoctests everywhere.

[andrew.mathas]

Thanks for identifying the problem Jeroen as I couldn't see this the way I was testing it. I've fixed both of these points. Andrew

[jdemeyer]

I think these changes should be reviewed. Travis, can you do that?

[andrew.mathas]

Replying to jdemeyer:

I think these changes should be reviewed. Travis, can you do that?

Hi Travis,

FYI, the trivial changes made were:

• in trac_10589--doctests_for_fixdoctests-am.patch​ two instances of -fixdoctests were changes to --fixdoctests and and print ret was changed to print err
• in trac_10589--fixdoctest_failures-am.patch the output is sent to doc_out=tmp_dir()+'.tmp' rather than just .tmp, which required a new import statement (this was an error in the original sage-fixdoctests which wasn't picked up when it was first added to sage...).

Andrew

[tscrim]

Hey Andrew,

Is there some way we could make this behave better when we don't have write permissions to a given directory, such as output query the user or just output to the console?

Hey Jeroen,

Thanks for noticing that, I never would have thought to check write permissions of the current directory.

Best,
Travis

[andrew.mathas]

Replying to tscrim:

Hey Andrew,

Is there some way we could make this behave better when we don't have write permissions to a given directory, such as output query the user or just output to the console?

Hi Travis,

This already happens. The problem that Jeroen found concerns a tempory file which the sage-fixdoctests script uses to dump the current state of the doc-tests. This file is now written to sage's tmp_dir() which is always writable.

The only other file that sage-fixdoctests writes is the replacement source file. By default this is put in the same directory as the original source, however, the user can supply an optional argument to have the file written elsewhere:

Usage: sage -fixdoctests <source file> [doctest output]
Creates a file <source file>.out that passes the doctests (modulo any raised exceptions)

Having said this, I just chcked and this optional argument is not mentioned under sage -advanced, so probably I should add it there. I could also make the script more user friendy and have it remind the user of this when it is unable to write its output file. Is this what you have in mind?

Andrew

[tscrim]

Replying to andrew.mathas:

This already happens. The problem that Jeroen found concerns a tempory file which the sage-fixdoctests script uses to dump the current state of the doc-tests. This file is now written to sage's tmp_dir() which is always writable.

The only other file that sage-fixdoctests writes is the replacement source file. By default this is put in the same directory as the original source, however, the user can supply an optional argument to have the file written elsewhere:

Usage: sage -fixdoctests <source file> [doctest output]
Creates a file <source file>.out that passes the doctests (modulo any raised exceptions)

Having said this, I just chcked and this optional argument is not mentioned under sage -advanced, so probably I should add it there. I could also make the script more user friendy and have it remind the user of this when it is unable to write its output file. Is this what you have in mind?

Hey Andrew,

Yes, that would be good.

Thanks,
Travis

[jdemeyer]

I confirm that the new patches seem to work (but I haven't looked at the code).

[andrew.mathas]

Replying to jdemeyer:

I confirm that the new patches seem to work (but I haven't looked at the code).

Thanks! I haven't yet incororated Travis' suggestions above. Btw, can any one tell me where the sage usage message lives so that I can mention the optional argument to fixdoctests there?

[jhpalmieri]

It's in SAGE_ROOT/spkg/bin/sage. You should also update the documentation at devel/sage/doc/en/reference/cmd/options.rst.

[andrew.mathas]

OK, I have just uploaded a new version of the patch, which now consists of three files for three different repositories (main, scripts and spkg). The new features of the patch are:

• sage-fixdoctests now checks to see if it can write to the specified output directory and if not it prompts the user for a better place to put the file
• the script now takes an optional argument --long so that fixdoctests can also massage --long doctests
• there is an updated usage message for fixdoctests printed by sage --advanced
• there is an updated usage message for fixdoctests in the reference manual

Ready for review...

[tscrim]

One very minor point, could you put a space after the colon on line 122 in the fixdoctest file? I prefer my prompts like:

prompt: foo

as opposed to:

prompt:foo

After that, you can set this to positive review. Thanks.

[andrew.mathas]

Replying to tscrim:

One very minor point, could you put a space after the colon on line 122 in the fixdoctest file? I prefer my prompts like:

After that, you can set this to positive review. Thanks.

Thanks very much Travis. Both done!

Andrew

[jdemeyer]

There is an obvious doctest failure, which should have been caught both by the author and reviewer here:

sage -t devel/sage/sage/tests/cmdline.py
**********************************************************************
File "devel/sage/sage/tests/cmdline.py", line 178, in sage.tests.cmdline.test_executable
Failed example:
    err
Expected:
    ''
Got:
    '/scratch/release/merger/sage-5.13.beta0/spkg/bin/sage: line 155: file.py: command not found\n/scratch/release/merger/sage-5.13.beta0/spkg/bin/sage: line 155: outpu
t_file: command not found\n/scratch/release/merger/sage-5.13.beta0/spkg/bin/sage: line 156: file.py.out: command not found\n/scratch/release/merger/sage-5.13.beta0/spkg
/bin/sage: line 157: --long: command not found\n'
**********************************************************************

[andrew.mathas]

Fixing help strings

[andrew.mathas]

Replying to jdemeyer:

There is an obvious doctest failure, which should have been caught both by the author and reviewer here:

Yep, agreed. Sorry. Fixed.

Jeroen, I always get the feeling you don't like me.

A.

[jdemeyer]

Replying to andrew.mathas:

Jeroen, I always get the feeling you don't like me.

It is true that failures like these are annoying to me, because they make me lose time while being completely avoidable. On the other hand, it is nothing personal (usually, I don't even look at ticket's authors). I certainly didn't mean to offend, and I apologize if I did.

[andrew.mathas]

Replying to jdemeyer:

Replying to andrew.mathas:

Jeroen, I always get the feeling you don't like me.

It is true that failures like these are annoying to me, because they make me lose time while being completely avoidable. On the other hand, it is nothing personal (usually, I don't even look at ticket's authors). I certainly didn't mean to offend, and I apologize if I did.

No, not offended as I completely understand: it annoys me when I do this as well.

Apologies for wasting your time annd Travis' (and mine), as I am sure we all have better thing to do.

A.

[tscrim]

Sorry about that Jeroen. I'm somewhat surprised that changes to the output of the --advanced broke that particular doctest. With Andrew's new patch it is fixed:

travis@Kristine-Desktop:~/sage-5.12.beta5/devel/sage-combinat/sage/tests$ sage -t --long ../tests/cmdline.py 
Running doctests with ID 2013-10-04-07-25-10-694770dc.
Doctesting 1 file.
sage -t --long cmdline.py
    [213 tests, 95.98 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 96.4 seconds
    cpu time: 0.4 seconds
    cumulative wall time: 96.0 seconds

[jdemeyer]

Replying to tscrim:

Sorry about that Jeroen. I'm somewhat surprised that changes to the output of the --advanced broke that particular doctest.

There reason is that ` is a special character for the shell.

[tscrim]

Replying to jdemeyer:

Replying to tscrim:

Sorry about that Jeroen. I'm somewhat surprised that changes to the output of the --advanced broke that particular doctest.

There reason is that ` is a special character for the shell.

Ah. Thanks.

[jdemeyer]

The diff output isn't portable, on Solaris:

sage -t --long devel/sage/sage/tests/cmdline.py
**********************************************************************
File "devel/sage/sage/tests/cmdline.py", line 417, in sage.tests.cmdline.test_executable
Failed example:
    print output[output.find('     SAGE: 1+1'):output.find(' \"\"\"')-1]
Expected:
         SAGE: 1+1              # incorrect output
    -    3
    +    2
         SAGE: m=matrix(ZZ,3)   # output when none is expected
    +    SAGE: (2/3)*m          # no output when it is expected
    +    [0 0 0]
         [0 0 0]
         [0 0 0]
    -    [1 0 0]
    -    SAGE: (2/3)*m          # no output when it is expected
         SAGE: mu=PartitionTuple([[4,4],[3,3,2,1],[1,1]])   # output when none is expected
    -    [4, 4, 3, 3, 2, 1, 1]
         SAGE: mu.pp()          # uneven indentation
    -    ****
    -    ****
    +       ****   ***   *
    +       ****   ***   *
    +              **
    +              *
         SAGE: PartitionTuples.global_options(convention="French")
         SAGE: mu.pp()         # fix doctest with uneven indentation
    +    *
    +    **
    +    ****   ***   *
    +    ****   ***   *
         SAGE: PartitionTuples.global_options.reset()
Got:
         SAGE: 1+1              # incorrect output
    -    3
    +    2
         SAGE: m=matrix(ZZ,3)   # output when none is expected
    +    SAGE: (2/3)*m          # no output when it is expected
         [0 0 0]
         [0 0 0]
    -    [1 0 0]
    -    SAGE: (2/3)*m          # no output when it is expected
    +    [0 0 0]
         SAGE: mu=PartitionTuple([[4,4],[3,3,2,1],[1,1]])   # output when none is expected
    -    [4, 4, 3, 3, 2, 1, 1]
         SAGE: mu.pp()          # uneven indentation
    -    ****
    -    ****
    +       ****   ***   *
    +       ****   ***   *
    +              **
    +              *
         SAGE: PartitionTuples.global_options(convention="French")
         SAGE: mu.pp()         # fix doctest with uneven indentation
    +    *
    +    **
    +    ****   ***   *
    +    ****   ***   *
         SAGE: PartitionTuples.global_options.reset()
**********************************************************************

[andrew.mathas]

Replying to jdemeyer:

The diff output isn't portable, on Solaris:

This is going to be tricky as I don't have access to a solaris machine so I can't test this. The only difference between the two diffs is where the + [0 0 0] appears.

It seems that the problem may be that -fixdoctests uses a universal diff:

os.system('diff -u %s %s | more'%(test_file, test_output))

Aunty google suggests that a context diff may work better:

os.system('diff -c %s %s | more'%(test_file, test_output))

Is anyone able to confirm this?

Looking at this there is also an issue because diff may not be installed on the system, or it may not be in the users PATH. The script should try and trap the resulting errors in these cases...although I'll need to find appropriate exceptions to look for. Of course, in all of these cases this test will fail but I don't see a way around this. Having diff installed presumably isn't, and shouldn't be, a requirement for sage.

Is there a way to mark tests for the doc-test suite to ignore a test on certain systems?

A.

[andrew.mathas]

Please ignore my queries: I have just discovered difflib which should fix both issues. A.

[andrew.mathas]

Makes fixdoctests use difflib to generate diffs

[andrew.mathas]

OK, the script now uses difflib to compare the input and output files so hopefully this will fix the solaris problem and potential issues with other operating systems.

The tests pass me for me on a mac.

sage -t ../tests/cmdline.py
    [204 tests, 48.07 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 48.1 seconds
    cpu time: 0.2 seconds
    cumulative wall time: 48.1 seconds

A.

[tscrim]

My output on Linux (Ubuntu) matches the output on Solaris. In other words, I also get the doctest failure noted above.

[tscrim]

Actually, since it does diff properly, should we:

• mark the doctest as # random,
• look at the beginning and ending with a ... inbetween since it matches there?

[andrew.mathas]

Replying to tscrim:

My output on Linux (Ubuntu) matches the output on Solaris. In other words, I also get the doctest failure noted above.

Hi Travis,

Sorry, this is my mistake: previously when you tested the script it worked, so the problem you're seeing is not because ubuntu's diff returns the same output as solaris. Rather that when I tested the the new patch I thought that I had the full patch applied but I was in the wrong branch where the patch was not applied. Sorry.

The new output, I hope, should be independent of the system diff as it uses difflib, which is a python module for generating diffs. In any case the new diff is closer to that returned by solaris than the previous universal diff.

With the patch applied, this time, I get the following:

sage -t cmdline.py
    [213 tests, 52.19 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 52.3 seconds
    cpu time: 0.2 seconds
    cumulative wall time: 52.2 seconds

The new version of trac_10589--doctests_for_fixdoctests-am.patch​ is now on trac.

Andrew

[andrew.mathas]

Makes fixdoctests use difflib to generate diffs

[tscrim]

Okay the new patch works for me on linux. Thanks Andrew.