Opened 11 years ago

Closed 8 years ago

#8708 closed enhancement (duplicate)

allow doctest script to handle docstrings with triple single quotes

Reported by: mvngu Owned by: tbd
Priority: minor Milestone: sage-duplicate/invalid/wontfix
Component: doctest coverage Keywords: sd41
Cc: ranosch Merged in:
Authors: Reviewers: John Palmieri, Burcin Erocal
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jhpalmieri)

As the subject says. This is related to #8699. See this sage-devel thread for some background information.


Apply

Attachments (5)

trac_8708-doctest_quotes.patch (2.1 KB) - added by burcin 9 years ago.
trac_8708-jhp.patch (3.5 KB) - added by jhpalmieri 9 years ago.
apply to scripts repo
trac_8708-testing.py (202 bytes) - added by jhpalmieri 9 years ago.
small Python file for testing purposes
trac_8708-jhp.v2.patch (3.5 KB) - added by jhpalmieri 9 years ago.
apply to scripts repo
trac_8708-doctests.patch (799 bytes) - added by jhpalmieri 9 years ago.
Sage repository

Download all attachments as: .zip

Change History (29)

Changed 9 years ago by burcin

comment:1 Changed 9 years ago by burcin

  • Authors set to Burcin Erocal
  • Cc ranosch added
  • Milestone changed from sage-4.7.1 to sage-4.7.2
  • Status changed from new to needs_review

comment:2 follow-up: Changed 9 years ago by jhpalmieri

  • Authors changed from Burcin Erocal to Burcin Erocal, John Palmieri

This doesn't work for me: it skips all doctests in a typical file. The issue is the line

min_ind = min(dq_ind, sq_ind)

which means that if either (not just both) search fails, then min_ind is -1, so the loop exits.

I also don't think we need to modify doc_prefix and doc_suffix, since this is part of the string written to the temporary doctesting file. Do the triple quotes in that file have to match the original ones, or can they all be """?

I'm attaching a new patch which uses regular expressions instead, and which doesn't modify doc_prefix and doc_suffix. I'm also attaching a small file for testing purposes; testing on this file should produce three failures, and if you add some print statements to the function extract_doc, it should extract the correct string.

comment:3 in reply to: ↑ 2 Changed 9 years ago by burcin

Replying to jhpalmieri:

This doesn't work for me: it skips all doctests in a typical file. The issue is the line

min_ind = min(dq_ind, sq_ind)

which means that if either (not just both) search fails, then min_ind is -1, so the loop exits.

Good catch! Thanks for looking into this.

I also don't think we need to modify doc_prefix and doc_suffix, since this is part of the string written to the temporary doctesting file. Do the triple quotes in that file have to match the original ones, or can they all be """?

What happens if you have something like:

def f(arg):
    '''
    some text
        
    """
    some more text
    """

    '''

comment:4 Changed 9 years ago by jhpalmieri

Oh, you're right. I'm replacing my patch with a new one, the difference being

  • sage-doctest

    diff --git a/sage-doctest b/sage-doctest
    a b def change_warning_output(file): 
    452452            tmpfiles.append(os.path.join(SAGE_TESTDIR, name + '.pyc'))
    453453
    454454    # Prefix/suffix for all doctests replacing the starting/ending """
    455     doc_prefix = 'r""">>> set_random_seed(0L)\n\n>>> change_warning_output(sys.stdout)\n\n'
    456     doc_suffix = '\n>>> sig_on_count()\n0\n"""'
     455    doc_prefix_dq = 'r""">>> set_random_seed(0L)\n\n>>> change_warning_output(sys.stdout)\n\n'
     456    doc_prefix_sq = doc_prefix_dq.replace('"""',"'''")
     457    doc_suffix_dq = '\n>>> sig_on_count()\n0\n"""'
     458    doc_suffix_sq = doc_suffix_dq.replace('"""',"'''")
    457459
    458460    n = 0
    459461    # Now extract the docstring by using a regular expression search
    def change_warning_output(file): 
    499501            name = "example"
    500502            s += "def %s_%s():"%(name,n_str)
    501503            n += 1
    502             s += "\t"+ doc_prefix + doc + doc_suffix + "\n\n"
     504            if m.groups()[0].find("'") > -1: # single quotes
     505                s += "\t"+ doc_prefix_sq + doc + doc_suffix_sq + "\n\n"
     506            else:
     507                s += "\t"+ doc_prefix_dq + doc + doc_suffix_dq + "\n\n"
    503508
    504509    if n == 0:
    505510        return  ''

I'll also add an example like this to the testing file.

Changed 9 years ago by jhpalmieri

apply to scripts repo

Changed 9 years ago by jhpalmieri

small Python file for testing purposes

comment:5 Changed 9 years ago by burcin

  • Authors changed from Burcin Erocal, John Palmieri to John Palmieri
  • Reviewers set to Burcin Erocal
  • Status changed from needs_review to positive_review

Looks good to me. Thanks for looking into this John.

comment:6 Changed 9 years ago by leif

  • Description modified (diff)

comment:7 Changed 9 years ago by leif

  • Dependencies set to #9739
  • Status changed from positive_review to needs_work
  • Work issues set to Rebase on #9739 (and perhaps also 10952).

This needs to be rebased on #9739, and perhaps also #10952 (both merged into Sage 4.7.2.alpha3).

comment:8 Changed 9 years ago by jhpalmieri

  • Priority changed from minor to blocker
  • Status changed from needs_work to needs_review

We missed an or two issue in #9739: with non-library files, first we just had a bug, using "source" instead of "file_name" or "root_name" or something. Second, in the import statement added to the file,

s += "from %s import *\n\n" % target_name

we need to replace target_name with os.path.basename(target_name).

In the new patch, I've made these two changes and also rebased. Given that there is actually new content, this needs a new review. Without the new content, doctesting .py non-library files completely fails, so I'm marking this as a blocker. Feel free to downgrade if you think it's appropriate.

Oh, I also realized that the attached file for testing purposes has a bad name: replace the hyphen with an underscore.

The part needing review:

  • sage-doctest

    diff --git a/sage-doctest b/sage-doctest
    a b def check_with_tolerance(expected, actua 
    510510
    511511        elif ext in ['.py', '.sage']:
    512512
    513             target_name = "%s_%d" % (file_name, os.getpid()) # like 'name', but
     513            target_name = "%s_%d" % (root_name, os.getpid()) # like 'name', but
    514514            target_base = os.path.join(SAGE_TESTDIR, target_name) # like 'base'
    515515
    516516            if ext == '.sage':
    def check_with_tolerance(expected, actua 
    528528                # TODO: instead of copying the file, add its source
    529529                # directory to PYTHONPATH.  We would also have to
    530530                # import from 'name' instead of 'target_name'.
    531                 os.system("cp '%s' %s.py" % (source, target_base))
     531                os.system("cp '%s' %s.py" % (file_name, target_base))
    532532
    533             s += "from %s import *\n\n" % target_name
     533            s += "from %s import *\n\n" % os.path.basename(target_name)
    534534
    535535            tmpfiles.append(target_base + ".py") # preparsed or copied original
    536536            tmpfiles.append(target_base + ".pyc") # compiled version of it

comment:9 Changed 9 years ago by jhpalmieri

  • Description modified (diff)

comment:10 Changed 9 years ago by jhpalmieri

Sorry, here's a new version. The relevant changes:

  • sage-doctest

    diff --git a/sage-doctest b/sage-doctest
    a b def check_with_tolerance(expected, actua 
    510510
    511511        elif ext in ['.py', '.sage']:
    512512
    513             target_name = "%s_%d" % (file_name, os.getpid()) # like 'name', but unique
    514             target_base = os.path.join(SAGE_TESTDIR, target_name) # like 'base', also unique
     513            root_name = os.path.basename(root_name)
     514            target_name = "%s_%d" % (root_name, os.getpid()) # like 'root_name', but unique
     515            target_base = os.path.join(SAGE_TESTDIR, target_name) # like 'target_name' but with full path
    515516
    516517            if ext == '.sage':
    517518                # TODO: preparse "<file>.sage" with a Sage library call
    def check_with_tolerance(expected, actua 
    528529                # TODO: instead of copying the file, add its source
    529530                # directory to PYTHONPATH.  We would also have to
    530531                # import from 'name' instead of 'target_name'.
    531                 os.system("cp '%s' %s.py" % (source, target_base))
     532                os.system("cp '%s' %s.py" % (file_name, target_base))
    532533
    533534            s += "from %s import *\n\n" % target_name

Changing "source" to "file_name" is necessary: doctesting non-library py files fails otherwise. The other change is to avoid failures if you specify a path name for the file to be tested: sage -t ./test.py will fail without this, as compared to sage -t test.py.

comment:11 Changed 9 years ago by leif

Well, at some point in the evolution of #9739 it worked (or was correct)... ;-)

Can anybody [else] review this [quickly]?

Sorry, I don't have the time, and I'm actually also not in the mood right now.

comment:12 Changed 9 years ago by jhpalmieri

Some things to test:

  • First apply the patches from #9739 and #10952.
  • Create a file "test.py" (not a Sage library file) and test it with "sage -t test.py", and also with "sage -t /path/to/test.py". Try before and after applying trac_8708-jhp.v2.patch. You could just use trac_8708-testing.py, but rename it so the name doesn't have any hyphens. This file is supposed to have 3 failures.
  • Do the same with a file "test.sage". (Just renaming your Python file to "test.sage" should be good enough.)

comment:13 Changed 9 years ago by leif

  • Status changed from needs_review to needs_work

I now get the following doctest failures, certainly due to the triple single quote patch / modification:

The following tests failed:

	sage -t  -long -force_lib devel/sagenb-main/sagenb/notebook/worksheet.py # Exception from doctest framework
	sage -t  -long -force_lib devel/sage/sage/interacts/library_cython.pyx # 1 doctests failed
	sage -t  -long -force_lib devel/sage/sage/misc/sageinspect.py # Exception from doctest framework

With -verbose:

$ ./sage -t -long -verbose devel/sagenb-main/sagenb/notebook/worksheet.py
sage -t -long -verbose "devel/sagenb-main/sagenb/notebook/worksheet.py"
Traceback (most recent call last):
  File "/home/leif/.sage//tmp/worksheet_15100.py", line 3046, in <module>
    runner=runner)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/sagedoctest.py", line 54, in testmod_returning_runner
    runner=runner)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 1819, in testmod_returning_runner
    for test in finder.find(m, name, globs=globs, extraglobs=extraglobs):
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 839, in find
    self._find(tests, obj, name, module, source_lines, globs, {})
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 893, in _find
    globs, seen)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 881, in _find
    test = self._get_test(obj, name, module, globs, source_lines)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 965, in _get_test
    filename, lineno)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 594, in get_doctest
    return DocTest(self.get_examples(string, name), globs,
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 608, in get_examples
    return [x for x in self.parse(string, name)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 570, in parse
    self._parse_example(m, name, lineno)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 640, in _parse_example
    lineno + len(source_lines))
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 726, in _check_prefix
    (lineno+i+1, name, line))
ValueError: line 11 of the docstring for __main__.example_109 has inconsistent leading whitespace: '    "'
Exception raised by doctesting framework. Use -verbose for details.
$ ./sage -t -long -verbose devel/sage/sage/misc/sageinspect.py
sage -t -long -verbose "devel/sage/sage/misc/sageinspect.py"
Traceback (most recent call last):
  File "/home/leif/.sage//tmp/sageinspect_15131.py", line 1327, in <module>
    runner=runner)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/sagedoctest.py", line 54, in testmod_returning_runner
    runner=runner)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 1819, in testmod_returning_runner
    for test in finder.find(m, name, globs=globs, extraglobs=extraglobs):
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 839, in find
    self._find(tests, obj, name, module, source_lines, globs, {})
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 893, in _find
    globs, seen)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 881, in _find
    test = self._get_test(obj, name, module, globs, source_lines)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 965, in _get_test
    filename, lineno)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 594, in get_doctest
    return DocTest(self.get_examples(string, name), globs,
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 608, in get_examples
    return [x for x in self.parse(string, name)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 570, in parse
    self._parse_example(m, name, lineno)
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 640, in _parse_example
    lineno + len(source_lines))
  File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/local/bin/ncadoctest.py", line 726, in _check_prefix
    (lineno+i+1, name, line))
ValueError: line 53 of the docstring for __main__.example_28 has inconsistent leading whitespace: '    """'
Exception raised by doctesting framework. Use -verbose for details.
$ ./sage -t -long -verbose devel/sage/sage/interacts/library_cython.pyxsage -t -long -verbose "devel/sage/sage/interacts/library_cython.pyx"

...

Trying:
    from sage.interacts.library_cython import cellular###line 91:_sage_    >>> from sage.interacts.library_cython import cellular
Expecting nothing
ok
Trying:
    rule = [Integer(1), Integer(0), Integer(1), Integer(0), Integer(0), Integer(1), Integer(1), Integer(0)]###line 92:_sage_    >>> rule = [1, 0, 1, 0, 0, 1, 1, 0]
Expecting nothing
ok
Trying:
    N = Integer(3)###line 93:_sage_    >>> N = 3
Expecting nothing
ok
Trying:
    print cellular(rule, Integer(3))###line 94:_sage_    >>> print cellular(rule, 3)
Expecting nothing
**********************************************************************
File "/home/leif/Sage/sage-4.7.2.alpha3-prerelease3/devel/sage/sage/interacts/library_cython.pyx", line ?, in __main__.example_3
Failed example:
    print cellular(rule, Integer(3))###line 94:_sage_    >>> print cellular(rule, 3)
Expected nothing
Got:
    [[0 0 0 1 0 0 0]
     [0 0 0 1 0 0 0]
     [0 1 0 1 0 1 0]]
Trying:
    sig_on_count()
Expecting:
    0
ok
4 items had no tests:
    __main__
    __main__.change_warning_output
    __main__.check_with_tolerance
    __main__.warning_function
3 items passed all tests:
   3 tests in __main__.example_0
   9 tests in __main__.example_1
   8 tests in __main__.example_2
**********************************************************************
1 items had failures:
   1 of   7 in __main__.example_3
27 tests in 8 items.
26 passed and 1 failed.
***Test Failed*** 1 failures.

comment:14 Changed 9 years ago by leif

P.S.:

John, could you perhaps separate the fix concerning doctesting non-library files, and attach it to #9739 (not modifying the other patches there, i.e., as a patch to be applied on top of the others)?

comment:15 follow-up: Changed 9 years ago by jhpalmieri

  • Priority changed from blocker to minor

Okay, I've done that now. I'm also downgrading the priority of this ticket. (We could also open a new ticket just for the issue now added on to #9739, if you think that would be cleaner. #9739 is still marked as closed, by the way. I didn't reopen it.)

Changed 9 years ago by jhpalmieri

apply to scripts repo

comment:16 in reply to: ↑ 15 Changed 9 years ago by leif

Replying to jhpalmieri:

Okay, I've done that now.

Thanks; just the commit message there refers to this ticket (and its subject), i.e., is unrelated.

#9739 is still marked as closed, by the way. I didn't reopen it.

Unless nobody else reopens or closes tickets, or attaches files to / updates files on tickets I've already closed (and I didn't ask him to) I don't really care.

Reopening tickets IMHO only makes sense if I'm going to actually back it out.

comment:17 Changed 9 years ago by leif

s/nobody/somebody/ or s/unless/as long as/

It's getting late...

comment:18 Changed 9 years ago by jhpalmieri

  • Description modified (diff)
  • Status changed from needs_work to needs_info

The test failure in interacts/libary_cython.pyx is because of a doctest which would have failed in the past, but it was enclosed in triple single quotes, and so was skipped by the doctesting framework. (There is a line saying sage: print cellular(rule, 3) with no output after it.) We can certainly just stick in the output, but I have no idea if it's right.

I have a fix for sageinspect.py, but I'm not sure about worksheet.py. I can work on it some more, but we should check whether there are failures in the flask notebook before working too hard on this one: if the file is going to go away soon, it's not worth working too hard fixing it.

I'm attaching a patch for the Sage library fixing the two files there; I'll try to get information about whether the patch to library_cython.pyx is actually the right thing to do.

comment:19 Changed 9 years ago by jhpalmieri

For the sagenb problem, this patch seems to fix things:

  • sagenb/notebook/worksheet.py

    diff --git a/sagenb/notebook/worksheet.py b/sagenb/notebook/worksheet.py
    a b except (KeyError, IOError): 
    38813881            C.delete_output()
    38823882
    38833883
    3884 __internal_test1 = '''
    3885 def foo(x):
    3886     "
    3887     EXAMPLES:
    3888         sage: 2+2
    3889         4
    3890     "
    3891     return x
    3892 '''.lstrip()
     3884__internal_test1 = '\ndef foo(x):\n    "\n    EXAMPLES:\n        sage: 2+2\n        4\n    "\n    return x'.lstrip()
    38933885
    38943886__internal_test2 = '''
    38953887sage: 2 + 2

I don't know if it's the best approach. Why does this use single double quotes, anyway? Shouldn't the EXAMPLES block be surrounded by """? Modifying the patch here to use """ still passes doctests.

comment:20 Changed 9 years ago by jhpalmieri

  • Dependencies changed from #9739 to #9739, #11871

The problem with library_cython.pyx should be dealt with at #11871, so I'm making that a dependency. I'll remove that part of the doctest patch.

Changed 9 years ago by jhpalmieri

Sage repository

comment:21 Changed 8 years ago by jhpalmieri

  • Status changed from needs_info to needs_review

I've submitted a patch for the flask notebook to deal with the issue with notebook/worksheet, and that should be merged soon. There is a patch at #11871 for interacts/library_cython.pyx. So I think this is now ready to go.

comment:22 Changed 8 years ago by jhpalmieri

  • Keywords sd41 added
  • Work issues Rebase on #9739 (and perhaps also 10952). deleted

comment:23 Changed 8 years ago by jdemeyer

  • Authors John Palmieri deleted
  • Dependencies #9739, #11871 deleted
  • Milestone changed from sage-5.8 to sage-duplicate/invalid/wontfix
  • Reviewers changed from Burcin Erocal to John Palmieri, Burcin Erocal
  • Status changed from needs_review to positive_review

Fixed by #12415.

comment:24 Changed 8 years ago by jdemeyer

  • Resolution set to duplicate
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.