| msg331937 - (view) |
Author: Jason R. Coombs (jaraco) *  |
Date: 2018-12-16 20:43 |
| Originally [reported in testing-cabal/mock#405](https://github.com/testing-cabal/mock/issues/405), I believe I've discovered an inconsistency that manifests as a flaw: `patch` and `patch.object` allow the target to be specified as string referring to the target object and this object is resolved at the time the patch effected, not when the patch is declared. `patch.dict` contrarily seems to resolve the dict eagerly, when the patch is declared. Observe with this pytest: ``` import mock target = dict(a=1) @mock.patch.dict('test_patch_dict.target', dict(b=2)) def test_after_patch(): assert target == dict(a=2, b=2) target = dict(a=2) ``` Here's the output: ``` $ rwt mock pytest -- -m pytest test_patch_dict.py Collecting mock Using cached mock-2.0.0-py2.py3-none-any.whl Collecting pbr>=0.11 (from mock) Using cached pbr-3.0.0-py2.py3-none-any.whl Collecting six>=1.9 (from mock) Using cached six-1.10.0-py2.py3-none-any.whl Installing collected packages: pbr, six, mock Successfully installed mock-2.0.0 pbr-3.0.0 six-1.10.0 ====================================== test session starts ======================================= platform darwin -- Python 3.6.1, pytest-3.0.5, py-1.4.33, pluggy-0.4.0 rootdir: /Users/jaraco, inifile: collected 1 items test_patch_dict.py F ============================================ FAILURES ============================================ ________________________________________ test_after_patch ________________________________________ @mock.patch.dict('test_patch_dict.target', dict(b=2)) def test_after_patch(): > assert target == dict(a=2, b=2) E assert {'a': 2} == {'a': 2, 'b': 2} E Omitting 1 identical items, use -v to show E Right contains more items: E {'b': 2} E Use -v to get the full diff test_patch_dict.py:8: AssertionError ==================================== 1 failed in 0.05 seconds ==================================== ``` The target is unpatched because `test_patch_dict.target` was resolved during decoration rather than during test run. Removing the initial assignment of `target = dict(a=1)`, the failure is thus: ``` ______________________________ ERROR collecting test_patch_dict.py _______________________________ ImportError while importing test module '/Users/jaraco/test_patch_dict.py'. Hint: make sure your test modules/packages have valid Python names. Traceback: /var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-pcm3552g/mock/mock.py:1197: in _dot_lookup return getattr(thing, comp) E AttributeError: module 'test_patch_dict' has no attribute 'target' During handling of the above exception, another exception occurred: <frozen importlib._bootstrap>:942: in _find_and_load_unlocked ??? E AttributeError: module 'test_patch_dict' has no attribute '__path__' During handling of the above exception, another exception occurred: test_patch_dict.py:4: in @mock.patch.dict('test_patch_dict.target', dict(b=2)) /var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-pcm3552g/mock/mock.py:1708: in __init__ in_dict = _importer(in_dict) /var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-pcm3552g/mock/mock.py:1210: in _importer thing = _dot_lookup(thing, comp, import_path) /var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/rwt-pcm3552g/mock/mock.py:1199: in _dot_lookup __import__(import_path) E ModuleNotFoundError: No module named 'test_patch_dict.target'; 'test_patch_dict' is not a package !!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!! ==================================== 1 error in 0.41 seconds ===================================== ``` Is there any reason `patch.dict` doesn't have a similar deferred resolution behavior as its sister methods? |
|
|
| msg336300 - (view) |
Author: Karthikeyan Singaravelan (xtreak) * |
Date: 2019-02-22 12:32 |
| If I understand the issue correctly it's as below is a simple reproducer where target is resolved with {'a': 1} during the construction of the decorator [0] though it's redefined later in the program as target = dict(a=2). Also here due to this since target is reassigned the decorator just stores a reference to {'a': 1} and updates it with {'b': 2} leaving the reference to dict object {'a': 2} unpatched in test_with_decorator. Meanwhile in case of the context manager (test_with_context_manager) it's created and resolved at the time it's executed hence updating the object {'a': 2} correctly. A possible fix would be to store the reference to the string path of the patch '__main__.target' and try it again with importer function. I will take a further look into this. It would be helpful if you can confirm this reproducer is good enough and resembles the original report. from unittest import mock target = dict(a=1) @mock.patch.dict('__main__.target', dict(b=2)) def test_with_decorator(): print(f"target inside decorator : {target}") def test_with_context_manager(): with mock.patch.dict('__main__.target', dict(b=2)): print(f"target inside context : {target}") target = dict(a=2) test_with_decorator() test_with_context_manager() $ python3 test_foo.py target inside decorator : {'a': 2} target inside context : {'a': 2, 'b': 2} [0] https://github.com/python/cpython/blob/3208880f1c72800bacf94a2045fcb0436702c7a1/Lib/unittest/mock.py#L1624 |
|
|
| msg336307 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2019-02-22 13:43 |
| I agree that’s a good reproducer. Thanks for looking into this! |
|
|
| msg336369 - (view) |
Author: Karthikeyan Singaravelan (xtreak) * |
Date: 2019-02-23 06:08 |
| Looking further this can be solved for a string target in patch.dict which can be resolved again while calling the decorated function. There could be a case where the actual target is specified and in that case mock could only updates the reference and cannot track if the variable has been redefined to reference a different dict object. In the below case also it's resolved with {'a': 1} in the decorator and later redefining target to {'a': 2} whose reference is not updated. I can propose a PR for string target but I am not sure if this case can be solved or it's expected. This seems to be not a problem with patch.object where redefining a class later like dict seems to work correctly and maybe it's due to creating a new class itself that updates the local to reference new class? Any thoughts would be helpful. # script with dict target passed from unittest import mock target = dict(a=1) @mock.patch.dict(target, dict(b=2)) def test_with_decorator(): print(f"target inside decorator : {target}") def test_with_context_manager(): with mock.patch.dict(target, dict(b=2)): print(f"target inside context : {target}") target = dict(a=2) test_with_decorator() test_with_context_manager() $ ./python.exe test_foo.py target inside decorator : {'a': 2} target inside context : {'a': 2, 'b': 2} |
|
|
| msg336385 - (view) |
Author: Mario Corchero (mariocj89) *  |
Date: 2019-02-23 16:29 |
| Interesting, `patch` does resolve it when the patched function is called (see https://github.com/python/cpython/blob/175421b58cc97a2555e474f479f30a6c5d2250b0/Lib/unittest/mock.py#L1269) vs patch.dict that resolves it at the time the patcher is created - when decorating - (see https://github.com/python/cpython/blob/175421b58cc97a2555e474f479f30a6c5d2250b0/Lib/unittest/mock.py#L1624). An option might be to delay the resolution as done for patch, changing https://github.com/python/cpython/blob/175421b58cc97a2555e474f479f30a6c5d2250b0/Lib/unittest/mock.py#L1625 to `self.in_dict_name = in_dict` Example untested patch: ``` diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py index 8f46050462..5328fda417 100644 --- a/Lib/unittest/mock.py +++ b/Lib/unittest/mock.py @@ -1620,9 +1620,7 @@ class _patch_dict(object): """ def __init__(self, in_dict, values=(), clear=False, **kwargs): - if isinstance(in_dict, str): - in_dict = _importer(in_dict) - self.in_dict = in_dict + self.in_dict_name = in_dict # support any argument supported by dict(...) constructor self.values = dict(values) self.values.update(kwargs) @@ -1649,7 +1647,7 @@ class _patch_dict(object): attr_value = getattr(klass, attr) if (attr.startswith(patch.TEST_PREFIX) and hasattr(attr_value, "__call__")): - decorator = _patch_dict(self.in_dict, self.values, self.clear) + decorator = _patch_dict(self.in_dict_name, self.values, self.clear) decorated = decorator(attr_value) setattr(klass, attr, decorated) return klass @@ -1662,7 +1660,11 @@ class _patch_dict(object): def _patch_dict(self): values = self.values - in_dict = self.in_dict + if isinstance(self.in_dict_name, str): + in_dict = _importer(self.in_dict_name) + else: + in_dict = self.in_dict_name + self.in_dict = in_dict ``` > This seems to be not a problem with patch.object where redefining a class later like dict seems to work correctly and maybe it's due to creating a new class itself that updates the local to reference new class? For patch, when you create a new class, the new one is patched as the name is resolved at the time the decorated function is executed, not when it is decorated. See: ``` $ cat t.py from unittest import mock import c target = dict(a=1) @mock.patch("c.A", "target", "updated") def test_with_decorator(): print(f"target inside decorator : {A.target}") def test_with_context_manager(): with mock.patch("c.A", "target", "updated"): print(f"target inside context : {A.target}") class A: target = "changed" c.A = A test_with_decorator() test_with_context_manager() xarmariocj89 at DESKTOP-9B6VH3A in ~/workspace/cpython on master* $ cat c.py class A: target = "original" mariocj89 at DESKTOP-9B6VH3A in ~/workspace/cpython on master* $ ./python ./t.py target inside decorator : changed target inside context : changed ``` If `patch` was implemented like `patch.dict`, you would see the first as "changed" as the reference to `c.A` would have been resolved when the decorator was run (before the re-definition of `A`). About `patch.object`, it cannot be compared, as it grabs the name at the time you execute the decorator because you are not passing a string, but the actual object to patch. |
|
|
| msg336389 - (view) |
Author: Karthikeyan Singaravelan (xtreak) * |
Date: 2019-02-23 16:49 |
| Thanks Mario for the details. I had almost the same patch while writing :) There were no test case failures except that I had resolved it in the constructor storing the string representation as a separate variable and also while calling the function which could be avoided as per your approach to just store the string and resolve once during function call. I think this is a good way to do this. Are you planning to create a PR or shall I go ahead with this? |
|
|
| msg336390 - (view) |
Author: Mario Corchero (mariocj89) * |
Date: 2019-02-23 16:52 |
| Great, all yours :) I'll be happy to review. |
|
|
| msg336480 - (view) |
Author: Chris Withers (cjw296) * |
Date: 2019-02-24 18:54 |
| New changeset a875ea58b29fbf510f9790ae1653eeaa47dc0de8 by Chris Withers (Xtreak) in branch 'master': bpo-35512: Resolve string target to patch.dict decorator during function call GH#12000 https://github.com/python/cpython/commit/a875ea58b29fbf510f9790ae1653eeaa47dc0de8 |
|
|
| msg336555 - (view) |
Author: Chris Withers (cjw296) * |
Date: 2019-02-25 21:17 |
| New changeset ea199b90bb61866cd3c2f154341d1eb0d5c4a710 by Chris Withers (Miss Islington (bot)) in branch '3.7': bpo-35512: Resolve string target to patch.dict decorator during function call GHGH-12000 (#12021) https://github.com/python/cpython/commit/ea199b90bb61866cd3c2f154341d1eb0d5c4a710 |
|
|
| msg336600 - (view) |
Author: Karthikeyan Singaravelan (xtreak) * |
Date: 2019-02-26 03:20 |
| Closing this as resolved. @jaraco this just made it to 3.8.0 alpha 2, feel free to reopen this if needed. Thanks Mario and Chris for review and merge :) |
|
|
| msg339459 - (view) |
Author: Jason R. Coombs (jaraco) * |
Date: 2019-04-04 18:25 |
| Confirmed the fix. Thank you very much! |
|
|