Repository navigation
doctest _load_testfile function -- newline handling seems incorrect #46137
Description
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 2020-03-26.16:26:07.378>
created_at = <Date 2008-01-12.05:57:33.772>
labels = ['easy', 'type-bug', '3.8', '3.9', '3.7', 'tests']
title = 'doctest _load_testfile function -- newline handling seems incorrect'
updated_at = <Date 2021-03-02.17:06:28.028>
user = 'https://github.com/pdonis'bugs.python.org fields:
activity = <Date 2021-03-02.17:06:28.028>
actor = 'zach.ware'
assignee = 'none'
closed = True
closed_date = <Date 2020-03-26.16:26:07.378>
closer = 'zach.ware'
components = ['Tests']
creation = <Date 2008-01-12.05:57:33.772>
creator = 'pdonis'
dependencies = []
files = ['9138', '9142', '18063', '18066', '18070', '18071', '18072', '18117', '18118', '18119', '18133', '18134', '26505', '26508']
hgrepos = []
issue_num = 1812
keywords = ['patch', 'easy']
message_count = 38.0
messages = ['59801', '59802', '59834', '110706', '110755', '110767', '110779', '110795', '110808', '110813', '110815', '110822', '110824', '110825', '111143', '111144', '111148', '111180', '111215', '111218', '111299', '111304', '111314', '111351', '111361', '111392', '112980', '166348', '166352', '357468', '360436', '361926', '361927', '365086', '365090', '365091', '365092', '387940']
nosy_count = 5.0
nosy_names = ['belopolsky', 'pdonis', 'eric.araujo', 'zach.ware', 'miss-islington']
pr_nums = ['17385', '19175', '19176', '24359']
priority = 'low'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue1812'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']
Activity
pdonis commentedon Jan 12, 2008
When running doctest.testfile on a Linux machine, testing a txt file
saved on a Windows machine, doctest raised a SyntaxError exception for
each Windows newline in the txt file. On examining the code in the
_load_testfile function, it looks to me like there are actually two
issues:
(1) When there is no package keyword argument given, or the package
argument points to a package without a __loader__.get_data method, the
txt file data is retrieved by calling open(filename).read(); this is
the code path that my Windows-saved file triggered. However, since the
default file mode for open is 'r', not 'rU', there is no universal
newline conversion done on the file data. This was the issue I
initially observed.
(2) When there is a package.__loader__.get_data method found, that
method reads the data (using file mode 'rb'), and then newline
conversion is performed by this line:
return file_contents.replace(os.linesep, '\n')
This does not seem to match what universal newline conversion does;
that is supposed to convert both '\r\n' and '\r' to '\n', but running
on Linux, os.linesep is '\n', so the replace operation in the current
code is a no-op, even if the txt file was saved with Windows or Mac
newlines. It seems to me that the correct operation would be:
I have attached a diff against the current svn trunk showing my
suggested fix for both of the above issues.
Peter Donis
pdonis commentedon Jan 12, 2008
Edit: I should have said that the attached diff also includes changes
to test_doctest.py to test for the correct newline behavior. Because
the test setup is a little complex, I added an auxiliary script,
doctest_testfile.py, and an accompanying text file,
doctest_testfile.txt, which intentionally contains mismatched newlines
for use in the test.
pdonis commentedon Jan 12, 2008
I've uploaded a revised diff with two small improvements:
(1) Removed a redundant os.isfile check in
PackageLoaderTestImporter.get_data() in doctest_testfile.py. (The
open() builtin already raises IOError if the file can't be opened.)
(2) Added doctest.master = None in three places in doctest_testfile.py
so the expected output is now nothing instead of three lines of
*** DocTestRunner.merge: 'doctest_testfile.txt' in both testers;
summing outcomes.
Changed the expected output in test_doctest.py to correspond.
abalkin commentedon Jul 19, 2010
Mark L,
This could use some shaking. Please take a look.
pdonis commentedon Jul 19, 2010
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.
BreamoreBoy commentedon Jul 19, 2010
Patched files work fine on Windows against the 2.7 maintainance branch.
doctest (doctest) ... 66 tests with zero failures
doctest (test.test_doctest) ... 440 tests with zero failures
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?
pdonis commentedon Jul 19, 2010
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:
/.local/lib/python2.7/test> uname -a/.local/lib/python2.7/test> python2.7Linux Powerspec 2.6.31.12-0.2-desktop #1 SMP PREEMPT 2010-03-16 21:25:39 +0100 i686 i686 i386 GNU/Linux
peter@Powerspec:
Python 2.7 (trunk:82970M, Jul 19 2010, 12:44:35)
[GCC 4.4.1 [gcc-4_4-branch revision 150839]] on linux2
Type "help", "copyright", "credits" or "license" for more information.
pdonis commentedon Jul 19, 2010
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.
BreamoreBoy commentedon Jul 19, 2010
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.
pdonis commentedon Jul 19, 2010
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.
pdonis commentedon Jul 19, 2010
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
Python 3.2a0 (py3k:82984, Jul 19 2010, 16:03:06)
[GCC 4.4.1 [gcc-4_4-branch revision 150839]] on linux2
Type "help", "copyright", "credits" or "license" for more information.
pdonis commentedon Jul 19, 2010
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.
18 remaining items
pdonis commentedon Nov 26, 2019
I have submitted pull request bpo-17385 regarding this issue:
#17385
pdonis commentedon Jan 21, 2020
Pinging as a reminder that there is a pull request for this issue awaiting review.
merwok commentedon Feb 13, 2020
Allow me to ask a maybe very naive question:
Wouldn't it be possible to fix the problem at the source, that is, use
open(filename, "rU")in the two spots you found rather than doing ad-hoc line ending translation?(I get that the second one uses
rbat the moment, so switching from bytes to str is not just a one-liner there)merwok commentedon Feb 13, 2020
Ah, I got it wrong, get_data itself uses
rb, not defaultr.It makes sense given that package data could be any kind of file, not necessarily text. Too bad there isn’t a
get_text(encoding)method in the loader API!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?
zware commentedon Mar 26, 2020
New changeset e0b8101 by Peter Donis in branch 'master':
bpo-1812: Fix newline conversion when doctest.testfile loads from a package whose loader has a get_data method (GH-17385)
e0b8101
miss-islington commentedon Mar 26, 2020
New changeset b05fbe9 by Miss Islington (bot) in branch '3.8':
bpo-1812: Fix newline conversion when doctest.testfile loads from a package whose loader has a get_data method (GH-17385)
b05fbe9
miss-islington commentedon Mar 26, 2020
New changeset 9387678 by Miss Islington (bot) in branch '3.7':
bpo-1812: Fix newline conversion when doctest.testfile loads from a package whose loader has a get_data method (GH-17385)
9387678
zware commentedon Mar 26, 2020
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.
zware commentedon Mar 2, 2021
New changeset b36349a by Peter Donis in branch 'master':
bpo-43049: Use io.IncrementalNewlineDecoder for doctest newline conversion (GH-24359)
b36349a