-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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 'fancy diff' formats incorrectly strip trailing whitespace #68934
Comments
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. |
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 |
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. |
I can write a unittest for this, where should I write it? a new test file? |
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. |
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? |
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
""" |
Thanks for your suggestion, Tim. I've fixed the patch and it is ready for review at #10639 |
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': |
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. |
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! |
In short, this buildbot runs "./python -m test -R 3:3 test_doctest". But |
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. |
I made #11501 to fix this problem. After my patch: ❯ ./python.exe -m test test_doctest test_doctest == Tests result: SUCCESS == All 2 tests OK. Total duration: 5 sec 286 ms |
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. |
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 ;-) See this discussion: |
All changes related to this issue are done. Thanks for your contribution and engagement everyone! |
All Refleaks buildots are back to green, thanks ;-) |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: