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.

Sign up for GitHub

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

Jump to bottom

doctest 'fancy diff' formats incorrectly strip trailing whitespace #68934

Closed
bitdancer opened this issue Jul 29, 2015 · 26 comments
Closed

doctest 'fancy diff' formats incorrectly strip trailing whitespace #68934

bitdancer opened this issue Jul 29, 2015 · 26 comments
Assignees
@orsenthil
Labels
3.7 (EOL) end of life 3.8 only security fixes easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@bitdancer
Copy link
Member

BPO 24746
Nosy @tim-one, @orsenthil, @vstinner, @ned-deily, @bitdancer, @CuriousLearner, @pablogsal, @miss-islington
PRs
  • bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff #10639
  • bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff #10639
  • [3.7] bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639) #11476
  • [3.7] bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639) #11476
  • [3.7] bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639) #11476
  • [3.6] bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639) #11477
  • [3.6] bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639) #11477
  • [3.6] bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639) #11477
  • [2.7] bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff #11482
  • [2.7] bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff #11482
  • Revert "bpo-24746: Avoid stripping trailing whitespace in doctest fan… #11496
  • Revert "bpo-24746: Avoid stripping trailing whitespace in doctest fan… #11496
  • Revert "bpo-24746: Avoid stripping trailing whitespace in doctest fan… #11496
  • [3.7] Revert "bpo-24746: Avoid stripping trailing whitespace in doctest fan… #11498
  • [3.7] Revert "bpo-24746: Avoid stripping trailing whitespace in doctest fan… #11498
  • [3.7] Revert "bpo-24746: Avoid stripping trailing whitespace in doctest fan… #11498
  • [3.6] Revert "bpo-24746: Avoid stripping trailing whitespace in doctest fan… #11499
  • [3.6] Revert "bpo-24746: Avoid stripping trailing whitespace in doctest fan… #11499
  • [3.6] Revert "bpo-24746: Avoid stripping trailing whitespace in doctest fan… #11499
  • bpo-24746: Fix doctest failures when running the testsuite with -R #11501
  • bpo-24746: Fix doctest failures when running the testsuite with -R #11501
  • bpo-24746: Fix doctest failures when running the testsuite with -R #11501
  • [3.7] bpo-24746: Fix doctest failures when running the testsuite with -R (GH-11501) #11505
  • [3.7] bpo-24746: Fix doctest failures when running the testsuite with -R (GH-11501) #11505
  • [3.7] bpo-24746: Fix doctest failures when running the testsuite with -R (GH-11501) #11505
  • [3.6] - "Revert bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639)" #11509
  • [3.6] - "Revert bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639)" #11509
  • [3.6] - "Revert bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639)" #11509
  • [2.7] bpo-24746: Fix doctest failures when running the testsuite with -R #11512
  • [2.7] bpo-24746: Fix doctest failures when running the testsuite with -R #11512
  • [2.7] bpo-24746: Fix doctest failures when running the testsuite with -R #11512
  • [2.7] bpo-24746: Fix doctest failures when running the testsuite with -R #11512
    		•
    Files
  • doctest_fancy_diff_trailing_whitespace.diff
  • issue24746.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 = 'https://github.com/orsenthil'
closed_at = <Date 2019-01-11.01:18:33.446>
created_at = <Date 2015-07-29.01:26:52.557>
labels = ['3.7', '3.8', 'type-bug', 'library', 'easy']
title = "doctest 'fancy diff' formats incorrectly strip trailing whitespace"
updated_at = <Date 2019-01-11.13:58:34.268>
user = 'https://github.com/bitdancer'

    bugs.python.org fields:

    activity = <Date 2019-01-11.13:58:34.268>
actor = 'vstinner'
assignee = 'orsenthil'
closed = True
closed_date = <Date 2019-01-11.01:18:33.446>
closer = 'orsenthil'
components = ['Library (Lib)']
creation = <Date 2015-07-29.01:26:52.557>
creator = 'r.david.murray'
dependencies = []
files = ['40049', '40191']
hgrepos = []
issue_num = 24746
keywords = ['patch', 'easy']
message_count = 26.0
messages = ['247552', '248695', '248701', '248726', '248727', '330214', '330220', '330240', '333319', '333320', '333322', '333341', '333360', '333361', '333362', '333363', '333367', '333372', '333382', '333388', '333390', '333392', '333415', '333417', '333431', '333468']
nosy_count = 9.0
nosy_names = ['tim.peters', 'orsenthil', 'vstinner', 'ned.deily', 'r.david.murray', 'jairotrad', 'CuriousLearner', 'pablogsal', 'miss-islington']
pr_nums = ['10639', '10639', '11476', '11476', '11476', '11477', '11477', '11477', '11482', '11482', '11496', '11496', '11496', '11498', '11498', '11498', '11499', '11499', '11499', '11501', '11501', '11501', '11505', '11505', '11505', '11509', '11509', '11509', '11512', '11512', '11512', '11512']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue24746'
versions = ['Python 2.7', 'Python 3.7', 'Python 3.8']
    @bitdancer
    Copy link
    Member Author

    I got a doctest failure that when I wrote the output to a file showed two exactly identical lines reported as being different. Turning off the fancy diff, I could see trailing whitespace on one of the lines. It turns out that when a fancy diff is requested, doctest explicitly goes through and strips trailing whitespace from the diff lines returned by difflib. This seems to me to be obviously incorrect. There is no clue in the changelog why this was done...this goes back to a massive refactoring of doctest that was done for python 2.4, and the fancy diff was introduced at that point, complete with this strange behavior.

    I tried to write a test for this but couldn't get it working in the time I was willing to devote to this (I've switched to NDIFF format, which shows the whitespace error even when the actual whitespace is stripped). Perhaps testing this via doctest isn't the best idea anyway, since it will be far to easy for the trailing whitespace in the test to get accidentally stripped. I've attached my work as a diff for reference if someone wants to work on this.

    @bitdancer bitdancer added stdlib Python modules in the Lib dir easy type-bug An unexpected behavior, bug, or error labels Jul 29, 2015
    @jairotrad
    Copy link
    Mannequin

    jairotrad mannequin commented Aug 17, 2015

    I have patched this as explained by David. Also the tests are working. Another test broke because a missing trailing space. I fixed that too.

    This is my first patch :D

    @bitdancer
    Copy link
    Member Author

    Unfortunately I was reminded a few days ago that there is a commit hook that prevents patches containing trailing whitespace from being committed to the repository. So using doctest to test this isn't going to work.

    The alternatives are to write a unit test, or to figure out how to write the doctest such there is no trailing whitespace...which would mean capturing the output of doctest.DoctTestRunner instead of letting it go the console, and then comparing it to an explicitly embedded string.

    @jairotrad
    Copy link
    Mannequin

    jairotrad mannequin commented Aug 17, 2015

    I can write a unittest for this, where should I write it? a new test file?

    @bitdancer
    Copy link
    Member Author

    I believe you can put it in the test_doctest file, and call it using the appropriate runner from test.support from the test_main function. I haven't tried it though.

    @CuriousLearner
    Copy link
    Member

    Unfortunately I was reminded a few days ago that there is a commit hook that prevents patches containing trailing whitespace from being committed to the repository.

    Is this still the case?

    I tried to figure out writing the test case, but I can't wrap my head around showing the trailing whitespace without putting it anywhere in the code.

    Also, can you have a look at the PR, please?

    @CuriousLearner CuriousLearner added 3.7 (EOL) end of life 3.8 only security fixes labels Nov 21, 2018
    @tim-one
    Copy link
    Member

    tim-one commented Nov 21, 2018

    To include trailing whitespace on a line in a doctest, _don't_ use raw strings. Use a regular string, and include add a (one or more) trailing \x20 instead of a space (for example). For example:

    r"""
>>> print("a ")
a 
"""

    where there's a space at the end of the output line is no good. It's visually impossible to tell what's intended, and the commit hook should reject it. But this works fine (a regular string and an escape code):

    """
>>> print("a ")
a\x20
"""

    @CuriousLearner
    Copy link
    Member

    Thanks for your suggestion, Tim.

    I've fixed the patch and it is ready for review at #10639

    @CuriousLearner CuriousLearner self-assigned this Nov 22, 2018
    @orsenthil
    Copy link
    Member

    New changeset cbb1645 by Senthil Kumaran (Sanyam Khurana) in branch 'master':
    bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (bpo-10639)
    cbb1645

    @miss-islington
    Copy link
    Contributor

    New changeset 53cf5f0 by Miss Islington (bot) in branch '3.7':
    bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639)
    53cf5f0

    @orsenthil
    Copy link
    Member

    New changeset 5d9ae8b by Senthil Kumaran (Miss Islington (bot)) in branch '3.6':
    bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639) (bpo-11477)
    5d9ae8b

    @orsenthil
    Copy link
    Member

    New changeset 02e33d9 by Senthil Kumaran (Sanyam Khurana) in branch '2.7':
    [2.7] bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (bpo-11482)
    02e33d9

    @vstinner
    Copy link
    Member

    test_doctest starts to fail on AMD64 Windows8.1 Refleaks 2.7:

    https://buildbot.python.org/all/#/builders/33/builds/471

    It may be related to:

    New changeset 02e33d9 by Senthil Kumaran (Sanyam Khurana) in branch '2.7':
    [2.7] bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (bpo-11482)
    02e33d9

    @vstinner
    Copy link
    Member

    Yeah, I confirm that it's a regression caused by commit 02e33d9:

    "./python -m test test_doctest test_doctest" fails after this commit, but pass before this commit.

    @CuriousLearner
    Copy link
    Member

    Hi Victor,

    Senthil reverted the backport commit on 2.7 branch.

    Is there a way I can test reference leaks in CPython? Is it using valgrind or some other tool? Can you please point me to that?

    I can then see if I can fix this on 2.7

    Thanks!

    @vstinner
    Copy link
    Member

    In short, this buildbot runs "./python -m test -R 3:3 test_doctest". But
    here the bug is just that running the test twice in a row fails.

    @vstinner
    Copy link
    Member

    Oh. The 3.6, 3.7 and master changes broke the Refleaks buildbots... By the way, why was such bugfix merged into the 3.6 branch which is now in security-only mode?

    Please fix this bug ASAP, or I will revert the change.

    @pablogsal
    Copy link
    Member

    I made #11501 to fix this problem. After my patch:

    ❯ ./python.exe -m test test_doctest test_doctest
    Run tests sequentially
    0:00:00 load avg: 1.82 [1/2] test_doctest
    0:00:02 load avg: 1.82 [2/2] test_doctest

    == Tests result: SUCCESS ==

    All 2 tests OK.

    Total duration: 5 sec 286 ms
    Tests result: SUCCESS

    @orsenthil
    Copy link
    Member

    New changeset c5dc60e by Senthil Kumaran (Pablo Galindo) in branch 'master':
    bpo-24746: Fix doctest failures when running the testsuite with -R (bpo-11501)
    c5dc60e

    @orsenthil
    Copy link
    Member

    Hi All,

    It was my mistake to merge this in into 3.6, I didn't realize 3.6 was in bugfix mode now. Also I went by the versions (previously) set in this bpo issue.

    For the regression caused in refleaks, I think, Pablo's patch will fix it, and I am verifying it here - https://buildbot.python.org/all/#/builders/1/builds/468

    This will be backported to 3.7 ( I notice viktor triggered already: #11505) and I will cherrpick this to 2.7.

    Finally, since the the port to 3.6 was a mistake, I will revert that.

    This will make sure that bug is fixed all the supported versions, target branches tests are successful and 3.6 remains unaffected.

    @vstinner
    Copy link
    Member

    Finally, since the the port to 3.6 was a mistake, I will revert that.

    I wrote PR 11499 but I closed it because PR bpo-11501 has been merged, but yeah, maybe a revert is the best option for 3.6.

    No problem, the status change of the 3.6 branch is very new, and not everybody got the memo ;-)
    https://devguide.python.org/#status-of-python-branches

    See this discussion:
    https://discuss.python.org/t/remove-needs-backport-to-3-6-label/563

    @miss-islington
    Copy link
    Contributor

    New changeset 1cbd17c by Miss Islington (bot) in branch '3.7':
    bpo-24746: Fix doctest failures when running the testsuite with -R (GH-11501)
    1cbd17c

    @ned-deily
    Copy link
    Member

    New changeset d09e8ce by Ned Deily (Senthil Kumaran) in branch '3.6':
    Revert "bpo-24746: Avoid stripping trailing whitespace in doctest fancy diff (GH-10639) (GH-11477)" (GH-11509)
    d09e8ce

    @orsenthil
    Copy link
    Member

    New changeset 0167c08 by Senthil Kumaran in branch '2.7':
    bpo-24746: Fix doctest failures when running the testsuite with -R (bpo-11501) (bpo-11512)
    0167c08

    @orsenthil
    Copy link
    Member

    All changes related to this issue are done.

    Thanks for your contribution and engagement everyone!

    @vstinner
    Copy link
    Member

    All Refleaks buildots are back to green, thanks ;-)

    @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
    Assignees

    @orsenthil orsenthil

    Labels
    3.7 (EOL) end of life 3.8 only security fixes easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    8 participants
    @vstinner @orsenthil @bitdancer @ned-deily @CuriousLearner @pablogsal @tim-one @miss-islington