Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#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:

Status badges

Description (last modified by jhpalmieri)

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

Attachments (2)

trac_12069-doctest.patch (3.5 KB) - added by jhpalmieri 10 years ago.
scripts repo
trac_12069-sage.patch (6.7 KB) - added by jhpalmieri 9 years ago.
Sage library

Download all attachments as: .zip

Change History (15)

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 jdemeyer

  • Status changed from needs_review to needs_work

Could you make a doctest (maybe in sage/tests/doctest.py) doing the following:

  1. Create a temporary file doctest.sage with some function and a doctest testing that function. (using http://docs.python.org/library/tempfile.html#tempfile.NamedTemporaryFile)
  2. Use test_executable from sage/tests/cmdline.py to execute sage -t the_temporary_file_created_in_step_1.sage.
  3. Check that this executable returned with exit status 0.

Also, it would be good to update the last section of doc/en/developer/doctesting.rst.

comment:3 Changed 10 years ago by jhpalmieri

  • 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?)

Changed 10 years ago by jhpalmieri

scripts repo

comment:4 Changed 10 years ago by jhpalmieri

  • Description modified (diff)

comment:5 Changed 10 years ago by jhpalmieri

  • Description modified (diff)

comment:6 Changed 10 years ago by fwclarke

  • 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 file doctest.py?

It looks like the sensible place to me.

comment:7 Changed 10 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

Here's a rebased patch.

comment:8 Changed 9 years ago by fwclarke

  • Reviewers set to Francis Clarke
  • Status changed from needs_review to positive_review

It's fine now. Positive review.

comment:9 Changed 9 years ago by jdemeyer

Just wondering: why

    sage: ret == 128 
    True 

instead of the more simple

    sage: ret
    128

comment:10 Changed 9 years ago by jhpalmieri

I don't think there's a good reason. I'll change it to

    sage: ret
    128

(Same with a few ret == 0 tests.)

Changed 9 years ago by jhpalmieri

Sage library

comment:11 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.0.beta11
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:12 Changed 9 years ago by leif

See #12805 for a follow-up, fixing left-over tmp/ subdirectories which prohibited the deletion of otherwise empty temporary doctesting directories.

comment:13 Changed 9 years ago by nthiery

Thanks for fixing this! That was a pain in the neck.

Note: See TracTickets for help on using tickets.