#12069 closed defect (fixed)
fix doctesting of .sage files
Reported by: | jhpalmieri | Owned by: | mvngu |
---|---|---|---|
Priority: | critical | Milestone: | sage-5.0 |
Component: | doctest coverage | Keywords: | doctest |
Cc: | leif, jdemeyer | Merged in: | sage-5.0.beta11 |
Authors: | John Palmieri | Reviewers: | Francis Clarke |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Trac #9739 broke doctesting of .sage files (as in sage -t my_file.sage
). The attached patch fixes this.
The problem was that before #9739, doctesting file.sage
produced two Python files: a preparsed version of file.sage
, and a file with the doctesting framework and the doctests, but not the actual Python/Sage? code -- it just included a line like from file import *
. In #9739, we missed this, and we just produced one Python file, the second one, so the original Python/Sage? code is nowhere to be found, and thus not importable. (Actually we produced the preparsed version, but then overwrote it with the file containing the doctesting framework.)
I think the same was happening with non-library Python files -- there should have been two Python files created, but sage-doctest was creating one and then overwriting it. The patch fixes this, too.
At the same time, this patch fixes a small bug in the function delete_tmpfiles
: the function used to quit completely if it couldn't find some file in the list tmpfiles
. Now if it can't find the file, it skips it and goes on to the next one.
Apply
- trac_12069-doctest.patch to the scripts repo,
- trac_12069-sage.patch to the Sage library.
Attachments (2)
Change History (15)
comment:1 Changed 10 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Status changed from needs_review to needs_work
comment:3 Changed 10 years ago by
- Status changed from needs_work to needs_review
Okay, here are new patches which add documentation to the sage-doctest
script, update doc/en/developer/doctesting.rst
, and add doctests to tests/cmdline.py
. (Is it okay to add to cmdline.py
instead of creating a new file doctest.py
?)
comment:4 Changed 10 years ago by
- Description modified (diff)
comment:5 Changed 10 years ago by
- Description modified (diff)
comment:6 Changed 10 years ago by
- Status changed from needs_review to needs_work
I'd give this a positive review, but for the fact that it won't apply on sage-5.0.beta8 (the problem being with cmdline.py
). But patching that file by hand, everything seems to work as it should.
Replying to jhpalmieri:
Is it okay to add to
cmdline.py
instead of creating a new filedoctest.py
?
It looks like the sensible place to me.
comment:7 Changed 10 years ago by
- Status changed from needs_work to needs_review
Here's a rebased patch.
comment:8 Changed 10 years ago by
- Reviewers set to Francis Clarke
- Status changed from needs_review to positive_review
It's fine now. Positive review.
comment:9 Changed 10 years ago by
Just wondering: why
sage: ret == 128 True
instead of the more simple
sage: ret 128
comment:10 Changed 10 years ago by
I don't think there's a good reason. I'll change it to
sage: ret 128
(Same with a few ret == 0
tests.)
comment:11 Changed 10 years ago by
- Merged in set to sage-5.0.beta11
- Resolution set to fixed
- Status changed from positive_review to closed
comment:12 Changed 10 years ago by
See #12805 for a follow-up, fixing left-over tmp/
subdirectories which prohibited the deletion of otherwise empty temporary doctesting directories.
comment:13 Changed 10 years ago by
Thanks for fixing this! That was a pain in the neck.
Could you make a doctest (maybe in
sage/tests/doctest.py
) doing the following:test_executable
fromsage/tests/cmdline.py
to executesage -t the_temporary_file_created_in_step_1.sage
.Also, it would be good to update the last section of
doc/en/developer/doctesting.rst
.