bpo-33747: Avoid mutating the global sys.modules dict by onyb · Pull Request #8520 · python/cpython

onyb · 2018-07-28T11:28:56Z

The test method test_patch_imports_lazily was injecting a mocked module
to the sys.modules dict, which was reused in another test module called
test_patch_propogrates_exc_on_exit. Hence, the other test module failed
whenever it was launched independently.

Implemented a context manager to make sure modifications to sys.modules
are not persisted after the test methods finish execution.

https://bugs.python.org/issue33747

The test method test_patch_imports_lazily was injecting a mocked module to the sys.modules dict, which was reused in another test module called test_patch_propogrates_exc_on_exit. Hence, the other test module failed whenever it was launched independently. Implemented a context manager to make sure modifications to sys.modules are not persisted after the test methods finish execution.

pablogsal · 2018-07-28T13:06:33Z

 )
-
+from unittest.test.testmock import support
+from unittest.test.testmock.support import SomeClass, is_instance


Why are these lines changed?

I was rearranging the imports. Just pushed a commit to revert them.

encukou · 2018-07-28T13:00:37Z

 )
-
+from unittest.test.testmock import support
+from unittest.test.testmock.support import SomeClass, is_instance


Could you avoid rearranging the imports? It makes it hard to see what is being changed.

Sorry. I just fixed it in a recent commit.

encukou · 2018-07-28T13:07:36Z

+        with swap_attr(sys, 'modules', sys.modules.copy()):
+            squizz = Mock()
+            sys.modules['squizz'] = squizz
+            yield squizz


What exactly the method does is not very clear from the name.
There are two places that need to patch in a mocked squizz module, but it seems to me that moving this into a method is too much indirection: the testcases would be much easier to read if they included the three lines directly even though it means a little code duplication:

with swap_attr(sys, 'modules', sys.modules.copy()): squizz = Mock() sys.modules['squizz'] = squizz ...

serhiy-storchaka · 2018-07-28T16:00:55Z

-        p1.start()
-        p1.stop()
-        self.assertEqual(squizz.squozz, 3)
+        with swap_attr(sys, 'modules', sys.modules.copy()):


I have doubts about changing the reference to sys.modules. it can be cached in some places. It is safer to change sys.modules in-place. See for example sys_modules_context in Lib/test/test_importlib/test_namespace_pkgs.py. There are several other helpers in modules test.support and test.test_importlib.util.

Okay with your suggestion, but could you please explain to me what kind of risk is posed by a change in reference?

I like the sys_modules_context helper more. Where do you think I should move this function to, before using it in the tests?

I don't know if @brettcannon or any of the other importlib maintainers expect the tests to be standalone, but if not then you could move sys_modules_context into test.support. Alternatively, it could be copied into test.support and leave the importlib tests using their own copy.

Okay with your suggestion, but could you please explain to me what kind of risk is posed by a change in reference?

I don't remember details (and these details were changed in recent releases), but the comment to sys_modules_context implies that there is such risk.

Where do you think I should move this function to, before using it in the tests?

You can just import it.

from test.test_importlib.test_namespace_pkgs import sys_modules_context

There are many import-related helpers scattered through different files, and I think that once we should merge and standardize them, but this is a different issue.

Thanks. I have made the modifications.

…eference of sys.modules

onyb · 2018-07-30T10:39:04Z

I have made the requested changes; please review again

bedevere-bot · 2018-07-30T10:39:06Z

Thanks for making the requested changes!

: please review the changes made to this pull request.

brettcannon

Minor swap to another context manager which is supported for use by other test modules.

brettcannon · 2018-08-10T18:58:58Z

 from unittest.test.testmock import support
 from unittest.test.testmock.support import SomeClass, is_instance

+from test.test_importlib.test_namespace_pkgs import sys_modules_context


I know that @serhiy-storchaka recommended using this context manager, but since it isn't in test.test_importlib.util then I wouldn't consider it stable. Instead, I would use test.test_importlib.util.uncache() which will do what you need and is considered usable by other tests.

test.test_importlib.util.uncache() is less strong than test.test_importlib.test_namespace_pkgs.sys_modules_context(), but I think in this case it would be enough.

bedevere-bot · 2018-08-10T19:00:43Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

…d-sys-modules-mutation

the-knights-who-say-ni · 2018-12-06T16:24:16Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

onyb · 2018-12-07T09:11:57Z

I have made the requested changes; please review again

bedevere-bot · 2018-12-07T09:11:59Z

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

miss-islington · 2018-12-07T23:30:44Z

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

bedevere-bot · 2018-12-07T23:31:02Z

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

…k tests (pythonGH-8520) (cherry picked from commit 3cf7438) Co-authored-by: Anirudha Bose <ani07nov@gmail.com>

bedevere-bot · 2018-12-07T23:31:16Z

GH-11032 is a backport of this pull request to the 3.6 branch.

…k tests (pythonGH-8520) (cherry picked from commit 3cf7438) Co-authored-by: Anirudha Bose <ani07nov@gmail.com>

…st.mock tests (GH-8520) (GH-11031) (cherry picked from commit 3cf7438) Co-authored-by: Anirudha Bose <ani07nov@gmail.com> https://bugs.python.org/issue33747

…st.mock tests (GH-8520) (GH-11032) (cherry picked from commit 3cf7438) Co-authored-by: Anirudha Bose <ani07nov@gmail.com> https://bugs.python.org/issue33747

the-knights-who-say-ni added the CLA signed label Jul 28, 2018

bedevere-bot added the awaiting review label Jul 28, 2018

pablogsal reviewed Jul 28, 2018

View reviewed changes

encukou reviewed Jul 28, 2018

View reviewed changes

onyb added 3 commits July 28, 2018 16:40

Remove usage of a separate context manager for restoring sys.modules

db7a7ec

Remove unused import

2d0dcb2

Revert PEP8 changes

4ea8da5

serhiy-storchaka reviewed Jul 28, 2018

View reviewed changes

Use sys_modules_context instead of swap_attr to avoid modifying the r…

300614e

…eference of sys.modules

serhiy-storchaka added needs backport to 3.6 skip news tests Tests in the Lib/test dir labels Jul 29, 2018

bedevere-bot added awaiting change review and removed awaiting review labels Jul 30, 2018

brettcannon requested changes Aug 10, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Aug 10, 2018

onyb added 2 commits December 6, 2018 16:55

Merge remote-tracking branch 'origin/master' into fix-issue33747-avoi…

8612842

…d-sys-modules-mutation

Replace usage of sys_modules_context in testpatch.py with uncache

fce4126

the-knights-who-say-ni removed the CLA signed label Dec 6, 2018

the-knights-who-say-ni added CLA not signed and removed CLA not signed labels Dec 6, 2018

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 7, 2018

brettcannon approved these changes Dec 7, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Dec 7, 2018

brettcannon merged commit 3cf7438 into python:master Dec 7, 2018

bedevere-bot removed the awaiting merge label Dec 7, 2018

bedevere-bot removed the needs backport to 3.7 label Dec 7, 2018

bedevere-bot removed the needs backport to 3.6 label Dec 7, 2018

onyb deleted the fix-issue33747-avoid-sys-modules-mutation branch December 8, 2018 10:12

Uh oh!

Conversation

onyb commented Jul 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onyb commented Jul 30, 2018

Uh oh!

bedevere-bot commented Jul 30, 2018

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 10, 2018

Uh oh!

the-knights-who-say-ni commented Dec 6, 2018

Uh oh!

onyb commented Dec 7, 2018

Uh oh!

bedevere-bot commented Dec 7, 2018

Uh oh!

miss-islington commented Dec 7, 2018

Uh oh!

bedevere-bot commented Dec 7, 2018

Uh oh!

bedevere-bot commented Dec 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

onyb commented Jul 28, 2018 •

edited

Loading