-
-
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 _load_testfile function -- newline handling seems incorrect #46137
Comments
When running doctest.testfile on a Linux machine, testing a txt file (1) When there is no package keyword argument given, or the package (2) When there is a package.__loader__.get_data method found, that return file_contents.replace(os.linesep, '\n') This does not seem to match what universal newline conversion does; for linesep in ('\r\n', '\r'):
file_contents = file_contents.replace(linesep, '\n') I have attached a diff against the current svn trunk showing my Peter Donis |
Edit: I should have said that the attached diff also includes changes |
I've uploaded a revised diff with two small improvements: (1) Removed a redundant os.isfile check in (2) Added doctest.master = None in three places in doctest_testfile.py *** DocTestRunner.merge: 'doctest_testfile.txt' in both testers; Changed the expected output in test_doctest.py to correspond. |
Mark L, This could use some shaking. Please take a look. |
I saw that this issue was bumped and re-tested against the current trunk (r82970). A further change in doctest_testfile.py was needed to make the test pass when called from regrtest.py: the test importer for the loader.get_data test case now stores the absolute path where doctest_testfile.txt is located, so that it can always find it. I've uploaded a 2nd revised diff with this change. |
Patched files work fine on Windows against the 2.7 maintainance branch. They fail on py3k, it's a unicode problem that's nothing to do with this patch. Could someone try this on a Linux box to confirm my findings? |
You'll probably want someone else to confirm, but for the record, my testing was on a Linux box (SuSE 11.2) using Python 2.7 built from the SVN trunk: peter@Powerspec:
|
I realized on comparing doctest-fixes2.diff with doctest-fixes1.diff that doctest-fixes2.diff doesn't capture the different newlines correctly, so patch on my machine wouldn't apply the diff (I had done testing from the modified svn checkout without reverting and then re-applying the patch). Uploaded doctest-fixes3.diff which is generated using the external diff command (svn diff --diff-cmd diff) so that patch will apply it cleanly. Not sure if this is something specific to my machine. |
I've again tried running the test against the 2.7 maintainance release and got:- ValueError: line 12 of the docstring for doctest_testfile.txt has inconsistent leading whitespace: 'This doctest verifies universal newline support; each command' Could another patch please be provided that corrects this? I suspect that I tried to edit the previous version to overcome this, screwed up the line endings and forgot to mention it, sorry about that. |
Have you tried the doctest-fixes3.diff I uploaded just a little while ago? You may have run up against the same issue I did when I tried to revert and re-apply the previous patch. See my msg110795. |
Uploaded a diff implementing the fix for the head of the py3k branch. Test passes on my Linux box: peter@Powerspec:~/.local/lib/python3.2/test> python3.2
|
Re msg110808, on thinking it over I realized it may not be so simple to get diff and patch to behave properly with a file like doctest_testfile.txt, where we want to intentionally mismatch newlines. We basically need to treat the file as binary rather than text (which also means that, since the file as I've uploaded it contains all Unix newlines except for the Windows and Mac specific lines, on a Windows or Mac system most of the file, including all the comment lines, ends up being part of the test). Maybe there's a way to set the svn:mime-type property to do that. In the meantime, I've uploaded the raw doctest_testfile.txt file, so if necessary it can just be copied into the appropriate directory for testing. |
Uploading py3k version of doctest_testfile.txt as well, in case it's needed for testing. |
Also, can someone please clear the spam flag on my msg110813? |
Re my msg110822, I think I have a better solution: |
Uploaded doctest-fixes5.diff with one minor correction: |
Uploaded doctest-fixes5-py3k.diff, diff against py3k |
The py3k stuff is fine on Windows but the 2.7 maintainance branch now fails.
|
I don't normally run Windows, so it will take a little time Output from testing on Windows: C:\Python27\Lib\test>python test_doctest.py C:\Python27\Lib\test>python Hopefully the improved patch will test OK on your box as well. If Improvements in doctest-fixes6.diff:
|
Uploaded revised diff against py3k branch, doctest-fixes6-py3k.diff, |
@peter again the py3k patch is fine and the 2.7 fails. Is this worth all your effort? |
@mark, I'm probably stubborn, yes. :-) Could you post verbose |
@peter apologies hereby offered, the 2.7 patch is fine, I'd screwed up doctest_testfile.txt. As the patch is ok and has been very thoroughly tested please can this be committed. |
I'll take a look. Mark, "commit review" is a stage after a patch was reviewed by a committer and is deemed appropriate. Usually this comes with an "accepted" resolution. Sometimes "commit review" is skipped for simple patches, but in most cases, at least two committers should approve a patch before it is committed. |
@mark, no problem, thanks for keeping up with all my patches. :-) |
@alexander: If I understand this correctly it means that there is effectively no distinction betwen "patch review" and "commit review". Hence it is perfectly possible that the work that myself and Peter have put in goes down the drain? This is not acceptable. Either the patch gets committed accepting the work that Peter and myself have put in or it gets rejected. Please make your mind up. Also note that I've several more issues in the pipeline that need a bit of TLC. I've a note of every issue number, and if they don't get committed within the next few days I will be asking why. |
can committers take a look please. |
I recently noticed that there has been a minor code change in the _load_testfile function in doctest, so I generated a new patch against the latest pull from Mercurial (cpython). No actual changes to the issue fix, but this patch should apply cleanly against a checkout from Mercurial. This patch also adds my name to the Misc/ACKS file. :-) I have not re-done patches in Mercurial's format against any other branches except the default. If this fix is still under consideration, I can generate patches against other branches and test them. |
Updated patch to ensure that tests pass when the -v flag is set running the test suite. This is done by having the helper script, doctest_testfile.py, call doctest.testfile with verbose=False to ensure there is no output if the test passes (which is what the master test_doctest script expects) even if the -v flag is set on the command line. |
Pinging as a reminder that there is a pull request for this issue awaiting review. |
Allow me to ask a maybe very naive question: Wouldn't it be possible to fix the problem at the source, that is, use (I get that the second one uses |
Ah, I got it wrong, get_data itself uses With Python 3’s rich IO system, isn’t there a quick and correct way to get a text file from a bytes stream? TextIOWrapper? |
12 years later, we finally landed your fix :). Thanks for the patch, and for bearing with us (and me in particular for the past couple months). As mentioned in the PR, there is probably opportunity to clean up the cleanup code in the new test a bit, and as Éric and Ammar mentioned here and in review, it could be interesting to use TextIOWrapper instead (though we'd probably only make that change in 3.9). If you're interesting in doing a follow-up PR, it can either be attached to this issue (in spite of it being closed) or a new issue opened. |
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: