Skip to content

bpo-37473: Don't import importlib ASAP in tests#14661

Merged
vstinner merged 1 commit intopython:masterfrom
vstinner:remove_importlib_hack
Jul 14, 2019
Merged

bpo-37473: Don't import importlib ASAP in tests#14661
vstinner merged 1 commit intopython:masterfrom
vstinner:remove_importlib_hack

Conversation

@vstinner
Copy link
Copy Markdown
Member

@vstinner vstinner commented Jul 9, 2019 •
edited by bedevere-bot
Loading

bpo-15386, bpo-37473: test_import, regrtest and libregrtest no longer
import importlib as soon as possible, as the first import, "to test
bpo-15386".

Sort test_import imports.

https://bugs.python.org/issue37473

@vstinner
bpo-37473: Don't import importlib ASAP in tests
f20e4c3 
bpo-15386, bpo-37473: test_import, regrtest and libregrtest no longer
import importlib as soon as possible, as the first import, "to test
bpo-15386".

Sort test_import imports.
@vstinner vstinner requested review from brettcannon and ncoghlan July 9, 2019 09:55
@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Jul 9, 2019

@brettcannon, @ncoghlan: Are you ok to remove these hacks?

It seems like https://bugs.python.org/issue15386 is already tested by test_import.test_there_can_be_only_one(). "import imp" was replaced with "import _imp" incommit be7e49f:

commit be7e49fd820318509cd8b4dbde479c552f74ef62
Author: Nick Coghlan <[email protected]>
Date:   Fri Jul 20 23:40:09 2012 +1000

    Close #15386: There was a loophole that meant importlib.machinery and imp would sometimes reference an uninitialised copy of importlib._bootstrap

At least, I can say that importlib breaks quickly with the following change:

diff --git a/Lib/importlib/__init__.py b/Lib/importlib/__init__.py
index 0c73c505f9..130bc19537 100644
--- a/Lib/importlib/__init__.py
+++ b/Lib/importlib/__init__.py
@@ -9,7 +9,7 @@ __all__ = ['__import__', 'import_module', 'invalidate_caches', 'reload']
 # modules would get an uninitialised copy of the source version, instead
 # of a fully initialised version (either the frozen one or the one
 # initialised below if the frozen one is not available).
-import _imp  # Just the builtin component, NOT the full Python module
+import imp  # Just the builtin component, NOT the full Python module
 import sys
 
 try:

@brettcannon
Copy link
Copy Markdown
Member

brettcannon commented Jul 9, 2019

As long as there's another test then I'm fine with the change.

@brettcannon brettcannon added the tests Tests in the Lib/test dir label Jul 9, 2019
aeros
aeros reviewed Jul 11, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@aeros aeros left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading through the issues and doing some testing on the latest version, I checked to see if the issue is still occurring:

I haven't worked out how yet, but importlib.machinery is managing to bypass the replacement of importlib._bootstrap with _frozen_importlib:

Since FileFinder was moved from _bootstrap to _bootstrap_external I replaced the reference when I ran through the process in the interpreter:

image

Based upon the outputs, it looks like the issue was solved at some point by having importlib._bootstrap_external.FileFinder and importlib.machinery.FileFinder refer to the same class, _frozen_importlib_external.FileFinder. To ensure this is happening, an assertion such as this could be used:

# ...
self.assertEqual(importlib.machinery.FileFinder, 
importlib._bootstrap_external.FileFinder, 
msg = "Both locations should refer to the same class.")

It might be a bit bandage solution still since it does not explain why the override was occurring in the first place, but at least it no longer happens with FileFinder.

@vstinner vstinner merged commit 8b7db5a into python:master Jul 14, 2019
@vstinner vstinner deleted the remove_importlib_hack branch July 14, 2019 17:31
@vstinner
Copy link
Copy Markdown
Member Author

vstinner commented Jul 14, 2019

I merged your PR. If someone considers than test_there_can_be_only_one() is not enough, please add a new test.

@ncoghlan
Copy link
Copy Markdown
Contributor

ncoghlan commented Jul 20, 2019

importlib initialisation changed quite a bit when the external import bootstrapping was split out to a separate step, so it's quite plausible that the previous odd behaviour was a result of FileFinder being defined in the core import system bootstrap module.

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@vstinner @lisroach
bpo-37473: Don't import importlib ASAP in tests (pythonGH-14661)
457beea 
bpo-15386, bpo-37473: test_import, regrtest and libregrtest no longer
import importlib as soon as possible, as the first import, "to test
bpo-15386".

It is tested by test_import.test_there_can_be_only_one().

Sort test_import imports.
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@vstinner @DinoV
bpo-37473: Don't import importlib ASAP in tests (pythonGH-14661)
9a20d11 
bpo-15386, bpo-37473: test_import, regrtest and libregrtest no longer
import importlib as soon as possible, as the first import, "to test
bpo-15386".

It is tested by test_import.test_there_can_be_only_one().

Sort test_import imports.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon Awaiting requested review from brettcannon

@ncoghlan ncoghlan Awaiting requested review from ncoghlan

1 more reviewer

@aeros aeros aeros left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vstinner @brettcannon @ncoghlan @aeros @the-knights-who-say-ni @bedevere-bot