#10589 closed defect (fixed)
sage -fixdoctests doesn't work correctly
Reported by: | mderickx | Owned by: | mvngu |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.13 |
Component: | scripts | Keywords: | days45 |
Cc: | Merged in: | sage-5.13.beta1 | |
Authors: | Andrew Mathas | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12415 | Stopgaps: |
Description (last modified by )
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)
Attachments (3)
Change History (69)
comment:1 Changed 7 years ago by
- Description modified (diff)
- Keywords sage45 added
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
- Description modified (diff)
- Milestone set to sage-5.8
comment:3 Changed 7 years ago by
- Description modified (diff)
comment:4 Changed 7 years ago by
- Description modified (diff)
comment:5 Changed 7 years ago by
- Description modified (diff)
comment:6 follow-up: ↓ 8 Changed 7 years ago by
comment:7 Changed 7 years ago by
- Keywords days45 added; sage45 removed
comment:8 in reply to: ↑ 6 Changed 7 years ago by
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.
comment:9 Changed 7 years ago by
- Status changed from needs_review to needs_work
comment:10 Changed 7 years ago by
- Component changed from doctest to doctest framework
comment:11 Changed 7 years ago by
- Dependencies set to 12415
- Status changed from needs_work to needs_review
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
comment:12 Changed 7 years ago by
- Description modified (diff)
comment:13 Changed 7 years ago by
- Description modified (diff)
comment:14 Changed 7 years ago by
- Description modified (diff)
comment:15 Changed 7 years ago by
- Description modified (diff)
comment:16 Changed 7 years ago by
- Description modified (diff)
comment:17 follow-up: ↓ 18 Changed 7 years ago by
Shouldn't that be the scripts repo?
comment:18 in reply to: ↑ 17 Changed 7 years ago by
- Description modified (diff)
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...
comment:19 Changed 7 years ago by
- Component changed from doctest framework to scripts
- Dependencies changed from 12415 to #12415
comment:20 Changed 7 years ago by
I used the patch here to solve problems I was getting with sage -fixdoctests
. Thanks, hope this can get reviewed and merged soon!
comment:21 Changed 7 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:22 Changed 7 years ago by
- Description modified (diff)
comment:23 follow-up: ↓ 24 Changed 7 years ago by
Looks good to me minus two minors thing:
- you're missing the
::
on line 405 intests/cmdline.py
, - could you de-indent the
AUTHORS:
block insage-fixdoctests
and align the extra text by 2 spaces.
Thanks Andrew.
comment:24 in reply to: ↑ 23 Changed 7 years ago by
Replying to tscrim:
Looks good to me minus two minors thing:
- you're missing the
::
on line 405 intests/cmdline.py
,- could you de-indent the
AUTHORS:
block insage-fixdoctests
and align the extra text by 2 spaces.
Thanks Travis, I have fixed both of these.
Andrew
comment:25 Changed 7 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Then it's a positive review. Thanks Andrew.
comment:26 follow-up: ↓ 27 Changed 7 years ago by
- Status changed from positive_review to needs_work
The file trac_10589--fixdoctest_failures-am.patch contains garbage at the start, please fix that.
comment:27 in reply to: ↑ 26 Changed 7 years ago by
Replying to jdemeyer:
The file trac_10589--fixdoctest_failures-am.patch contains garbage at the start, please fix that.
Sorry. Fixed.
comment:28 Changed 7 years ago by
- Status changed from needs_work to positive_review
comment:29 follow-up: ↓ 31 Changed 7 years ago by
- Status changed from positive_review to needs_work
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> **********************************************************************
comment:30 Changed 7 years ago by
- Description modified (diff)
comment:31 in reply to: ↑ 29 Changed 7 years ago by
- Status changed from needs_work to positive_review
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
comment:32 Changed 7 years ago by
- Milestone changed from sage-5.12 to sage-5.13
comment:33 Changed 7 years ago by
I'm pretty sure I did apply both patches, but I'll try again...
comment:34 Changed 7 years ago by
- Status changed from positive_review to needs_work
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.
comment:35 Changed 7 years ago by
- Status changed from needs_work to positive_review
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
comment:36 Changed 7 years ago by
- Status changed from positive_review to needs_work
comment:37 follow-up: ↓ 38 Changed 7 years ago by
- Status changed from needs_work to needs_review
I think these changes should be reviewed. Travis, can you do that?
comment:38 in reply to: ↑ 37 Changed 7 years ago by
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 todoc_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
comment:39 follow-up: ↓ 40 Changed 7 years ago by
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
comment:40 in reply to: ↑ 39 ; follow-up: ↓ 41 Changed 7 years ago by
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
comment:41 in reply to: ↑ 40 Changed 7 years ago by
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
comment:42 follow-up: ↓ 43 Changed 6 years ago by
I confirm that the new patches seem to work (but I haven't looked at the code).
comment:43 in reply to: ↑ 42 Changed 6 years ago by
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?
comment:44 Changed 6 years ago by
It's in SAGE_ROOT/spkg/bin/sage
. You should also update the documentation at devel/sage/doc/en/reference/cmd/options.rst
.
comment:45 Changed 6 years ago by
- Description modified (diff)
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...
comment:46 follow-up: ↓ 47 Changed 6 years ago by
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.
comment:47 in reply to: ↑ 46 Changed 6 years ago by
- Status changed from needs_review to positive_review
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
comment:48 Changed 6 years ago by
- Description modified (diff)
comment:49 follow-up: ↓ 50 Changed 6 years ago by
- Status changed from positive_review to needs_work
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' **********************************************************************
comment:50 in reply to: ↑ 49 ; follow-up: ↓ 51 Changed 6 years ago by
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.
comment:51 in reply to: ↑ 50 ; follow-up: ↓ 52 Changed 6 years ago by
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.
comment:52 in reply to: ↑ 51 Changed 6 years ago by
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.
comment:53 follow-up: ↓ 54 Changed 6 years ago by
- Status changed from needs_work to positive_review
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
comment:54 in reply to: ↑ 53 ; follow-up: ↓ 55 Changed 6 years ago by
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.
comment:55 in reply to: ↑ 54 Changed 6 years ago by
comment:56 follow-up: ↓ 57 Changed 6 years ago by
- Status changed from positive_review to needs_work
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() **********************************************************************
comment:57 in reply to: ↑ 56 ; follow-up: ↓ 58 Changed 6 years ago by
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.
comment:58 in reply to: ↑ 57 Changed 6 years ago by
Please ignore my queries: I have just discovered difflib which should fix both issues. A.
comment:59 Changed 6 years ago by
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.
comment:60 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:61 follow-up: ↓ 63 Changed 6 years ago by
My output on Linux (Ubuntu) matches the output on Solaris. In other words, I also get the doctest failure noted above.
comment:62 Changed 6 years ago by
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?
comment:63 in reply to: ↑ 61 Changed 6 years ago by
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
comment:64 Changed 6 years ago by
- Status changed from needs_review to positive_review
Okay the new patch works for me on linux. Thanks Andrew.
comment:65 Changed 6 years ago by
- Merged in set to sage-5.13.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
comment:66 Changed 5 years ago by
- Description modified (diff)
To doctest this, you could add some tests to
devel/sage/sage/tests/cmdline.py