Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doctest.DocTestSuite error misleading when module has no docstrings #58854

Closed
cjerdonek opened this issue Apr 23, 2012 · 27 comments
Closed

doctest.DocTestSuite error misleading when module has no docstrings #58854

cjerdonek opened this issue Apr 23, 2012 · 27 comments
Labels
easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@cjerdonek
Copy link
Member

BPO 14649
Nosy @bitdancer, @asvetlov, @cjerdonek
Files
  • issue-14649-1.patch
  • issue-14649-2.patch
  • issue-14649-3.patch: Improving main code comment.
  • issue-14649-4.patch
  • issue-14649-5.patch
  • issue-14649-6.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2012-09-10.14:19:39.620>
    created_at = <Date 2012-04-23.14:54:21.332>
    labels = ['easy', 'type-bug', 'library']
    title = 'doctest.DocTestSuite error misleading when module has no docstrings'
    updated_at = <Date 2012-09-10.16:08:55.340>
    user = 'https://github.com/cjerdonek'

    bugs.python.org fields:

    activity = <Date 2012-09-10.16:08:55.340>
    actor = 'chris.jerdonek'
    assignee = 'none'
    closed = True
    closed_date = <Date 2012-09-10.14:19:39.620>
    closer = 'r.david.murray'
    components = ['Library (Lib)']
    creation = <Date 2012-04-23.14:54:21.332>
    creator = 'chris.jerdonek'
    dependencies = []
    files = ['26269', '26990', '26991', '27000', '27027', '27040']
    hgrepos = []
    issue_num = 14649
    keywords = ['patch', 'easy']
    message_count = 27.0
    messages = ['159022', '164721', '168212', '168214', '168228', '168232', '168233', '168246', '168659', '169097', '169104', '169147', '169152', '169244', '169315', '169317', '169323', '170077', '170078', '170079', '170102', '170110', '170123', '170189', '170190', '170197', '170199']
    nosy_count = 4.0
    nosy_names = ['r.david.murray', 'asvetlov', 'chris.jerdonek', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue14649'
    versions = ['Python 2.7', 'Python 3.2', 'Python 3.3']

    @cjerdonek
    Copy link
    Member Author

    When invoking doctest.DocTestSuite's constructor with a module that has no docstrings, doctest raises the following exception:

    ...
    File "/opt/local/Library/Frameworks/Python.framework/Versions/3.2/lib/python3.2/doctest.py", line 2280, in DocTestSuite
    raise ValueError(module, "has no tests")
    ValueError: (<module '[snip]' from '[snip]'>, 'has no tests')

    The error message is misleading because the exception is not raised for modules that have docstrings but no doctests, only for modules that have no docstrings.

    To be accurate, the message should be something like 'has no docstrings'.

    @cjerdonek cjerdonek added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Apr 23, 2012
    @cjerdonek
    Copy link
    Member Author

    I have attached a patch with tests.

    @cjerdonek cjerdonek added the easy label Jul 6, 2012
    @asvetlov
    Copy link
    Contributor

    I think current behavior is correct.
    Every doctest is docstring from tested module, but not every docstring is a valid doctest.

    @bitdancer
    Copy link
    Member

    Chris isn't suggesting changing the behavior, just the error message. I agree with his change for exactly the reason you say: currently doctest doesn't complain if a module does have docstrings but none of those docstrings are tests. So a module could have no tests without the error being raised.

    However, I suspect that the current behavior is a bug. I suspect the intent was to raise an error if there were no *tests*, not if there were no docstrings. I don't think it is a bug that should be fixed, though. I have a couple of test runners in various projects that run doctest on *all* modules, the goal being to pick up any doctests that there are or that I may add, even though not all modules have docstrings that contain doctests. The doctest version of TestCase-based discovery.

    So if anything I think this exception should be dropped. If the report is that no tests are run, that should be enough of an indication that something is wrong, without raising an error, as it has been for the case where there are docstrings but no tests (we do not as far as I know have a bug or feature request that the current behavior is inadequate).

    @cjerdonek
    Copy link
    Member Author

    Every doctest is docstring from tested module, but not every docstring is a valid doctest.

    Actually, I'm not sure even this is correct. doctest will form a unittest *TestSuite* from a module if it has docstrings, but it will not necessarily create a test for each docstring. You can see this by running code like the following on a target module that contains an empty docstring:

    suite = doctest.DocTestSuite('doctest_target')
    print(repr(suite))
    print(suite.countTestCases())

    which outputs--

    <unittest.suite.TestSuite tests=[]>
    0

    So while "tests" (in the doctest code) evaluates to True (because it is a TestSuite instance), it still "has no tests." If it wants to check for tests, it should probably be evaluating tests.countTestCases(), as David suggested.

    @cjerdonek
    Copy link
    Member Author

    I suspect the intent was to raise an error if there were no *tests*, not if there were no docstrings.

    That, or the implementor thought that if no docstrings were found, then that might indicate something went wrong with the parsing.

    For background purposes, here is the justification in the code comment:

    elif not tests:
        # Why do we want to do this? Because it reveals a bug that might
        # otherwise be hidden.
        raise ValueError(module, "has no tests")
    

    I have a couple of test runners in various projects that run doctest on *all* modules

    Same here, which is how I noticed the issue.

    So if anything I think this exception should be dropped. If the report is that no tests are run, that should be enough of an indication that something is wrong

    I would be in favor of this. The unittest module's test discovery, for example, does not raise an exception if it finds no tests (to my knowledge). In what versions would we be able to make this change?

    I could prepare another patch.

    @bitdancer
    Copy link
    Member

    That's a good question. Perhaps you could argue for it as a bug fix since it doesn't seem to be documented either way...except for the 'exclude_empty' argument of testmod. If testmod throws an error when there are no docstrings instead of returning a 0 test count or (if exclude_empty is true) skipping the module, then I think we can treat it as a bug and fix it in all active versions.

    @cjerdonek
    Copy link
    Member Author

    The problem seems to be restricted only to DocTestSuite. testmod and DocTestFinder both seem to work fine. DocTestSuite calls DocTestFinder but not the other way around: nothing calls DocTestSuite.

    If in certain versions we do not treat raising the exception as a bug (e.g. in already-released versions), then I am hoping we can at least document the behavior as a known issue in those versions and still change the exception message to be more accurate and helpful.

    Aside from ensuring that docstrings are present, another way to work around the issue is to pass a DocTestFinder to DocTestSuite with exclude_empty set to False, as in the following code:

    finder = DocTestFinder(exclude_empty=False)
    suite = DocTestSuite(module, test_finder=finder)

    Depending on what we decide for which versions, I am willing to create two patches: one for the documentation and exception message change (e.g. for stable versions), and another to drop raising the exception (e.g. for future versions).

    @asvetlov
    Copy link
    Contributor

    Ok, I'm +0 for that.

    @cjerdonek
    Copy link
    Member Author

    Here is an updated patch for review (just for the default branch for now).

    The main change from the previous patch is that the ValueError exception is now documented.

    A few additional comments/questions:

    Is there a better way to signify in the documentation that certain behavior is an unfixable bug as opposed to desired behavior? Would that be appropriate here?

    I created a new folder inside Lib/test for supporting test files specific to doctest. In addition to the two new files this patch adds, I would also like to move the eight existing support files into this new directory:

    doctest_aliases.py
    test_doctest.txt
    test_doctest3.txt
    sample_doctest.py
    test_doctest2.py
    test_doctest4.txt
    test_doctest.py
    test_doctest2.txt

    Can that be done as a separate change after the patch for this issue, or should it be done before?

    Assuming people are okay with keeping this ValueError behavior the same in the maintenance releases (i.e. not handling this as a bug fix), would people be open to changing the behavior in future versions, or should we keep the behavior the same in future versions as well?

    @bitdancer
    Copy link
    Member

    Personally I prefer to have the test case create the file(s) used in the test dynamically, writing them to the temporary working directory. Since these are Python modules, you could use the helpers from script_helpers for this.

    Otherwise I think your patch is OK. I don't think we have a doc convention for "unfortunate implementation detail" :( We have in the past put notes in the doc that say "this may change in the future", though that is usually about desired enhancements. But perhaps it could apply here as well.

    @cjerdonek
    Copy link
    Member Author

    Personally I prefer to have the test case create the file(s) used in the test dynamically, writing them to the temporary working directory.

    I prefer that too, but when I approached this issue, I found that test_doctest doesn't use unittest. It uses only doctest tests to test itself, and it uses checked-in files to test things like DocTestSuite and DocFileSuite. For example, test_DocFileSuite has this:

    >>> suite = doctest.DocFileSuite('test_doctest.txt',
    ...                              'test_doctest2.txt',
    ...                              'test_doctest4.txt')

    (Hence the eight supporting files I mentioned in my previous comment, of which these are three.)

    Should I break the current way of doing things for this module and introduce unittest and the first dynamically created files?

    We have in the past put notes in the doc that say "this may change in the future", though that is usually about desired enhancements.

    That's interesting. It's like a "Will change in version ..." counterpart to "Changed in version ...." Come to think of it, that could be a useful practice in general because it would let people future-proof their code more easily and give them incentives to upgrade.

    To simplify things for the purposes of this issue, we can discuss adding such wording to earlier versions only if and when we actually change the future behavior. I can create a separate issue to discuss changing future behavior once this issue is complete.

    @cjerdonek
    Copy link
    Member Author

    Small tweak to a code comment in the patch (tests is not itself a unittest.TestSuite instance).

    @cjerdonek
    Copy link
    Member Author

    Updating the patch after discussing with David on IRC.

    The two new files are now added to Lib/test instead of to a subdirectory of Lib/test. Moving the doctest files to a subdirectory can be discussed and possibly addressed as part of a separate issue.

    @bitdancer
    Copy link
    Member

    With your patch 5 applied, test_zipimport_support fails. I took a quick look at this and it looks like it is because we've added a dependency on an external data file to the test for DocTestSuite.

    Note that with patch 4 applied, test_pyclbr fails...I found that out by accident. Not sure why test_zipimport_support didn't fail with that one. But regardless we'll have to deal with pyclbr if/when we isolate the doctest test files.

    @cjerdonek
    Copy link
    Member Author

    With your patch 5 applied, test_zipimport_support fails.

    You're right. It looks like test_zipimport_support is tightly coupled to test_doctest. For example--

    http://hg.python.org/cpython/file/786d9516663e/Lib/test/test_zipimport_support.py#l94

    My apologies for not running the full test suite before uploading those patches. I'll take a closer look.

    @cjerdonek
    Copy link
    Member Author

    I updated the patch to fix the test_zipimport_support tests. All tests now pass. The only changes were to Lib/test/test_zipimport_support.py.

    I'll make a note about test_pyclbr and test_zipimport_support when I create the issue to move the supporting doctest files into a subdirectory. Thanks again for the catch, David.

    @bitdancer
    Copy link
    Member

    OK, 3.2 passes now, but when I merge to 3.3 I get failures. test_zipimport_support doesn't work on 2.7 either, but there we could just not backport the tests.

    @cjerdonek
    Copy link
    Member Author

    I prepared the patch against 3.3 (default), so I wouldn't have expected it to work against 3.2 without further changes. The behavior in 3.3 after merging is also surprising given that it worked in 3.2 (and for me with direct application).

    I will look into it and report back -- providing 3.2 and a "delta" patch, if necessary. I just wanted to be sure you were okay with the patch for the default branch before doing that extra work.

    @cjerdonek
    Copy link
    Member Author

    I tried applying the latest patch (patch #6) to 3.2, merging to default, and then running all tests in default, and the tests seem to all pass for me (in both default and 3.2). Also, the "diff" for the change in default after that process seems to be identical to the uploaded patch.

    Is it possible that you applied the wrong patch at some point in the process? What test failure were you getting?

    Can you try applying the patch as is to the default branch (i.e. without going through 3.2) and see if you still get test failures?

    @bitdancer
    Copy link
    Member

    Ah, my mistake. I forgot to do the hg add for the new files.

    @cjerdonek
    Copy link
    Member Author

    What workflow/command are you using to apply and then merge the patch? I didn't need to run "hg add" at any point to go from 3.2 to default, and run the tests, etc.

    @bitdancer
    Copy link
    Member

    I have separate working directories for 3.2 vs 3.3. If you have a single working directory, the files would continue to be present even without the add. The add is definitely needed for a correctly committed changeset.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 10, 2012

    New changeset d012f645b8b2 by R David Murray in branch '3.2':
    bpo-14649: clarify DocTestSuite error when there are no docstrings.
    http://hg.python.org/cpython/rev/d012f645b8b2

    New changeset 6544fc92b528 by R David Murray in branch 'default':
    Merge bpo-14649: clarify DocTestSuite error when there are no docstrings.
    http://hg.python.org/cpython/rev/6544fc92b528

    New changeset b48ef168d8c5 by R David Murray in branch '2.7':
    bpo-14649: clarify DocTestSuite error when there are no docstrings.
    http://hg.python.org/cpython/rev/b48ef168d8c5

    @bitdancer
    Copy link
    Member

    Thanks, Chris.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 10, 2012

    New changeset c5fc49bc7a5f by R David Murray in branch '2.7':
    bpo-14649: add sample files omitted from previous checkin.
    http://hg.python.org/cpython/rev/c5fc49bc7a5f

    @cjerdonek
    Copy link
    Member Author

    Thanks a lot, David.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants