Fix duplicated imports with importlib mode and doctest-modules #11148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hmm not sure why pypy3 build failed, it broke during tox setup. |
bluetech
approved these changes
Jun 29, 2023
jaraco
added a commit
to pmxbot/pmxbot
that referenced
this pull request
Jun 29, 2023
|
Good news is this does work to fix most of the test failures in pmxbot, so it's a good fix. Bad news is there's still two import-related failures that may have a different root cause. I'll investigate those, put together another reproducer and possibly file a new issue (if distinct). Regardless, this fix here seems solid, so feel free to merge. Update: the other issue appears to be a manifestation of #10337. |
jaraco
approved these changes
Jun 29, 2023
nicoddemus
force-pushed
the
import-mode-10811
branch
from
e8a6c76
to
7335b76
Compare
The initial implementation (in pytest-dev#7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. Not adding modules to `sys.modules` proved problematic, so we began to add the imported module to `sys.modules` in pytest-dev#7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then pytest-dev#10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix pytest-dev#10811, pytest-dev#10341
nicoddemus
force-pushed
the
import-mode-10811
branch
from
7335b76
to
d2ae7d3
Compare
|
Thanks for the bug fix! Is there a plan to backport this to 7.4? If not, would this be part of a minor release or a major release? |
|
Thanks @akhilramkee! Yes backporting makes sense, on it now. |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The initial implementation (in #7246) introduced the
importlibmode, which never added the imported module tosys.modules, so it included a test to ensure callingimport_pathtwice would yield different modules.Not adding modules to
sys.modulesproved problematic, so we began to add the imported module tosys.modulesin #7870, but failed to realize that given we are now changingsys.modules, we might as well avoid importing it more than once.Then #10088 came along, passing
importlibalso when importing application modules to collect docstrings (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once.With this PR,
import_pathreturns the module immediately if already insys.modules.Fix #10811
Fix #10341