bpo-1812: Fix newline conversion when doctest.testfile loads from a package whose loader has a get_data method. #17385

pdonis · 2019-11-26T00:41:19Z

bpo-1812: Fix newline conversion when doctest.testfile loads from a package whose loader has a get_data method.

This pull request fixes the newline conversion bug originally reported in issue 1812. When that issue was originally submitted, the open builtin did not default to universal newline mode; now it does, which makes the issue fix simpler, since the only code path that needs to be changed is the one in doctest._load_testfile where the file is loaded from a package whose loader has a get_data method. Commit b6ad8ee contains the change that implements the fix.

This pull request also adds a test that verifies that doctest.testfile does the newline conversion correctly when loading from a package whose loader has a get_data method. This test fails (a ValueError is raised for inconsistent whitespace) unless the change in commit b6ad8ee is applied. Commit dd02845 contains the changes that add the test; the test is added to the test_lineendings docstring along with supporting comments.

Also, the test_lineendings docstring is missing a test to check for correct handling of the old-style Mac newline \r. This pull request adds that case to the original test_lineendings docstring. Commit da35562 contains the changes that add this test and edit the surrounding comments accordingly.

Finally, this pull request contains commit 22094c8 that adds my name to the MISC/Acks file.

https://bugs.python.org/issue1812

…ings. The test_lineendings test in test_doctest did not include a test with old-style Mac line endings (\r). This commit adds one, and edits the introductory discussion in test_lineendings accordingly.

…le from package with loader.get_data. The previous test_doctest.test_lineendings did not cover the case where a testfile for doctest.testfile is loaded from a package whose loader has a get_data method. This commit adds a test for that case.

…ackage with loader.get_data. This commit corrects the newline conversion that is done in doctest._load_testfile when the file is being loaded from a package whose loader has a get_data method.

zware

Overall, this is an incredibly clean patch, and you have the patience of a saint to have shepherded this along for 12 years! I have a style nit and a cleanup suggestion, but this is probably acceptable as is.

Thanks for the patch, and hopefully we'll finally get this landed!

Lib/test/test_doctest.py

aeros · 2020-02-12T06:43:56Z

This seems like it should be backported to 3.6+, no? It's fixing an internal function and making some changes to the tests, so I don't think there's any backwards compatibility concerns to worry about. I'll add the backport labels for now, but feel free to remove them if needed.

zware · 2020-02-12T07:01:32Z

3.6 is in security-fix-only mode, so this definitely will not be backported that far.

aeros · 2020-02-12T08:40:37Z

Oh oops, for some reason I had forgotten that 3.6 was a security-only branch. Thanks for clarifying.

Break up long line to improve readability. Co-Authored-By: Zachary Ware <[email protected]>

This commit wraps the doctest.testfile test with a package loader that has a get_data method in a context manager to ensure proper removal of the extra package hook.

pdonis · 2020-02-21T04:36:43Z

Any update on this? Should I be looking at the 3.7 and 3.8 branches for backporting?

aeros · 2020-02-21T06:30:59Z

Any update on this?

I believe at this point we're just waiting for a follow-up review from @zware.

Should I be looking at the 3.7 and 3.8 branches for backporting?

The labels I added will trigger @miss-islington to attempt to open backport PRs to the 3.8 and 3.7 branches after this one is merged. It typically works without conflicts, but if there are any, you may have to resolve them manually with git cherry-pick (or the tool on PyPI, cherry-picker).

Lib/doctest.py

ammaraskar · 2020-02-21T19:50:55Z

Lib/test/test_doctest.py

+        sys.modules.update(self.modules_before)
+
+
+@contextlib.contextmanager


I think some of this and the code above can be folded to use stuff from https://github.com/python/cpython/blob/master/Lib/test/test_importlib/fixtures.py like:

cpython/Lib/test/test_importlib/fixtures.py

Lines 71 to 83 in fd5116c

class OnSysPath(Fixtures):

@staticmethod

@contextlib.contextmanager

def add_sys_path(dir):

sys.path[:0] = [str(dir)]

try:

yield

finally:

sys.path.remove(str(dir))

def setUp(self):

super(OnSysPath, self).setUp()

self.fixtures.enter_context(self.add_sys_path(self.site_dir))

cpython/Lib/test/test_importlib/fixtures.py

Lines 50 to 56 in fd5116c

@contextlib.contextmanager

def install_finder(finder):

sys.meta_path.append(finder)

try:

yield

finally:

sys.meta_path.remove(finder)

The OnSysPath fixture appears to be designed for a unittest test case, not doctest. A standalone version of the add_sys_path context manager might work if combined with install_finder; however, I notice that neither of those clears the path importer cache, and neither one removes the imported test module from sys.modules when the test is done. I don't know enough about the internals of the test framework to know if leaving out those last two items is OK; I put them in to make sure that everything that could possibly be mutated by this particular test was restored afterwards.

pdonis · 2020-02-22T08:07:41Z

@aeros Thanks for the update and for explaining how the backport process works.

pdonis · 2020-03-01T01:13:30Z

@zware is there any update on this?

brettcannon · 2020-03-13T20:33:14Z

Lib/test/test_doctest.py

@@ -2659,12 +2663,52 @@ def test_testfile(): r"""
    >>> sys.argv = save_argv
 """

+class TestLoader(importlib.abc.MetaPathFinder, importlib.abc.ResourceLoader):


This is misnamed as this is implementing a finder and a loader, so call it an importer.

I'll push an update changing the name to TestImporter.

Commit bb836d7 does the name change.

Change the name of the class used to test the case of a package with a loader.get_data method from TestLoader to TestImporter.

pdonis · 2020-03-24T03:07:18Z

Any update on merging this?

brettcannon · 2020-03-24T19:07:15Z

I refreshed the review request for @zware to see if he has time to review again.

zware

I wouldn't mind seeing the cleanup code cleaned up a bit in a separate PR, but as nobody has pointed out any actual problems with it as is, let's go ahead.

Lib/doctest.py

Lib/test/test_doctest.py

miss-islington · 2020-03-26T15:53:20Z

Thanks @pdonis for the PR, and @zware for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

…ackage whose loader has a get_data method (pythonGH-17385) This pull request fixes the newline conversion bug originally reported in bpo-1812. When that issue was originally submitted, the open builtin did not default to universal newline mode; now it does, which makes the issue fix simpler, since the only code path that needs to be changed is the one in doctest._load_testfile where the file is loaded from a package whose loader has a get_data method. (cherry picked from commit e0b8101) Co-authored-by: Peter Donis <[email protected]>

bedevere-bot · 2020-03-26T15:53:33Z

GH-19175 is a backport of this pull request to the 3.8 branch.

…ackage whose loader has a get_data method (pythonGH-17385) This pull request fixes the newline conversion bug originally reported in bpo-1812. When that issue was originally submitted, the open builtin did not default to universal newline mode; now it does, which makes the issue fix simpler, since the only code path that needs to be changed is the one in doctest._load_testfile where the file is loaded from a package whose loader has a get_data method. (cherry picked from commit e0b8101) Co-authored-by: Peter Donis <[email protected]>

bedevere-bot · 2020-03-26T15:53:41Z

GH-19176 is a backport of this pull request to the 3.7 branch.

…ackage whose loader has a get_data method (GH-17385) This pull request fixes the newline conversion bug originally reported in bpo-1812. When that issue was originally submitted, the open builtin did not default to universal newline mode; now it does, which makes the issue fix simpler, since the only code path that needs to be changed is the one in doctest._load_testfile where the file is loaded from a package whose loader has a get_data method. (cherry picked from commit e0b8101) Co-authored-by: Peter Donis <[email protected]>

pdonis · 2020-03-26T20:04:00Z

@zware @brettcannon Thanks so much for getting this merged!

…rsion (GH-24359) Followup to bpo-1812 and GH-17385.

pdonis added 7 commits November 23, 2019 23:35

bpo-1812: Add old-style Mac newline test to test_doctest.test_lineend…

da35562

…ings. The test_lineendings test in test_doctest did not include a test with old-style Mac line endings (\r). This commit adds one, and edits the introductory discussion in test_lineendings accordingly.

Merge branch 'master' into fix-issue-1812

59370c5

bpo-1812: Fix newline conversion when loading doctest testfile from p…

b6ad8ee

…ackage with loader.get_data. This commit corrects the newline conversion that is done in doctest._load_testfile when the file is being loaded from a package whose loader has a get_data method.

Add Peter Donis to Misc/ACKS.

22094c8

Merge branch 'master' into fix-issue-1812

80977d7

Merge branch 'master' into fix-issue-1812

74471ec

the-knights-who-say-ni added the CLA signed label Nov 26, 2019

bedevere-bot added the awaiting review label Nov 26, 2019

pdonis changed the title ~~Fix issue 1812~~ bpo-1812: Fix newline conversion when doctest.testfile loads from a package whose loader has a get_data method. Nov 26, 2019

Add MISC/NEWS.d blurb.

83c03d8

zware reviewed Feb 12, 2020

View reviewed changes

Lib/test/test_doctest.py Outdated Show resolved Hide resolved

Lib/test/test_doctest.py Show resolved Hide resolved

gpshead added the type-bug An unexpected behavior, bug, or error label Feb 12, 2020

aeros added needs backport to 3.6 labels Feb 12, 2020

zware removed the needs backport to 3.6 label Feb 12, 2020

pdonis and others added 3 commits February 12, 2020 19:51

Update Lib/test/test_doctest.py

efabd42

Break up long line to improve readability. Co-Authored-By: Zachary Ware <[email protected]>

Fix typo in commit efabd42.

1d56c05

bpo-1812: Wrap package hook test in context manager.

e1f0a2f

This commit wraps the doctest.testfile test with a package loader that has a get_data method in a context manager to ensure proper removal of the extra package hook.

ammaraskar reviewed Feb 21, 2020

View reviewed changes

Lib/doctest.py Show resolved Hide resolved

ammaraskar reviewed Feb 21, 2020

View reviewed changes

Merge branch 'master' into fix-issue-1812

4664f1e

brettcannon reviewed Mar 13, 2020

View reviewed changes

pdonis added 2 commits March 16, 2020 14:17

Merge branch 'master' into fix-issue-1812

860f835

bpo-1812: Change TestLoader to TestImporter.

bb836d7

Change the name of the class used to test the case of a package with a loader.get_data method from TestLoader to TestImporter.

brettcannon requested a review from zware March 24, 2020 19:06

zware approved these changes Mar 26, 2020

View reviewed changes

Lib/doctest.py Show resolved Hide resolved

Lib/test/test_doctest.py Show resolved Hide resolved

bedevere-bot added awaiting merge and removed awaiting review labels Mar 26, 2020

zware merged commit e0b8101 into python:master Mar 26, 2020

bedevere-bot removed the awaiting merge label Mar 26, 2020

bedevere-bot removed the needs backport to 3.8 label Mar 26, 2020

bedevere-bot removed the needs backport to 3.7 label Mar 26, 2020

pdonis deleted the fix-issue-1812 branch March 28, 2020 03:11

merwok mentioned this pull request Jan 28, 2021

bpo-43049: Use io.IncrementalNewlineDecoder for doctest newline conversion #24359

Merged

zware pushed a commit that referenced this pull request Mar 2, 2021

bpo-43049: Use io.IncrementalNewlineDecoder for doctest newline conve…

b36349a

…rsion (GH-24359) Followup to bpo-1812 and GH-17385.

sthagen mentioned this pull request Mar 2, 2021

bpo-43049: Use io.IncrementalNewlineDecoder for doctest newline conve… sthagen/python-cpython#412

Merged

		sys.modules.update(self.modules_before)


		@contextlib.contextmanager

	class OnSysPath(Fixtures):
	@staticmethod
	@contextlib.contextmanager
	def add_sys_path(dir):
	sys.path[:0] = [str(dir)]
	try:
	yield
	finally:
	sys.path.remove(str(dir))

	def setUp(self):
	super(OnSysPath, self).setUp()
	self.fixtures.enter_context(self.add_sys_path(self.site_dir))

	@contextlib.contextmanager
	def install_finder(finder):
	sys.meta_path.append(finder)
	try:
	yield
	finally:
	sys.meta_path.remove(finder)

Uh oh!

bpo-1812: Fix newline conversion when doctest.testfile loads from a package whose loader has a get_data method. #17385

bpo-1812: Fix newline conversion when doctest.testfile loads from a package whose loader has a get_data method. #17385

Uh oh!

Conversation

pdonis commented Nov 26, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zware left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aeros commented Feb 12, 2020

Uh oh!

zware commented Feb 12, 2020

Uh oh!

aeros commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pdonis commented Feb 21, 2020

Uh oh!

aeros commented Feb 21, 2020

Uh oh!

Uh oh!

ammaraskar Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

pdonis Feb 22, 2020

Choose a reason for hiding this comment

Uh oh!

pdonis commented Feb 22, 2020

Uh oh!

pdonis commented Mar 1, 2020

Uh oh!

brettcannon Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

pdonis Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

pdonis Mar 16, 2020

Choose a reason for hiding this comment

Uh oh!

pdonis commented Mar 24, 2020

Uh oh!

brettcannon commented Mar 24, 2020

Uh oh!

zware left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miss-islington commented Mar 26, 2020

Uh oh!

bedevere-bot commented Mar 26, 2020

Uh oh!

bedevere-bot commented Mar 26, 2020

Uh oh!

pdonis commented Mar 26, 2020

Uh oh!

Uh oh!

pdonis commented Nov 26, 2019 •

edited by bedevere-bot

Loading

aeros commented Feb 12, 2020 •

edited

Loading