classification
Title: patch.dict resolves in_dict eagerly (should be late resolved)
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: cjw296, jaraco, mariocj89, michael.foord, xtreak
Priority: normal Keywords: patch

Created on 2018-12-16 20:43 by jaraco, last changed 2019-04-04 18:25 by jaraco. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 12000 merged xtreak, 2019-02-23 18:59
PR 12021 merged miss-islington, 2019-02-24 19:01
Messages (11)
msg331937 - (view) Author: Jason R. Coombs (jaraco) * (Python committer) 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 <module>
    @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) * (Python triager) 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) * (Python committer) Date: 2019-02-22 13:43
I agree that’s a good reproducer. Thanks for looking into this!
msg336369 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) 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) * (Python triager) 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) * (Python triager) Date: 2019-02-23 16:49
Thanks Mario for the details. I had almost the same patch while writing msg336300 :) 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) * (Python triager) Date: 2019-02-23 16:52
Great, all yours :) I'll be happy to review.
msg336480 - (view) Author: Chris Withers (cjw296) * (Python committer) 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) * (Python committer) 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) * (Python triager) 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) * (Python committer) Date: 2019-04-04 18:25
Confirmed the fix. Thank you very much!
History
Date User Action Args
2019-04-04 18:25:29jaracosetmessages: + msg339459
2019-02-26 03:20:26xtreaksetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-02-26 03:20:03xtreaksetmessages: + msg336600
2019-02-25 21:17:20cjw296setmessages: + msg336555
2019-02-24 19:01:46miss-islingtonsetpull_requests: + pull_request12053
2019-02-24 18:54:52cjw296setmessages: + msg336480
2019-02-23 18:59:26xtreaksetkeywords: + patch
stage: patch review
pull_requests: + pull_request12030
2019-02-23 16:52:24mariocj89setmessages: + msg336390
2019-02-23 16:49:05xtreaksetmessages: + msg336389
2019-02-23 16:29:43mariocj89setmessages: + msg336385
2019-02-23 06:08:37xtreaksetnosy: + cjw296, michael.foord, mariocj89
messages: + msg336369
2019-02-22 13:43:17jaracosetmessages: + msg336307
2019-02-22 12:32:29xtreaksetversions: + Python 3.8
nosy: + xtreak

messages: + msg336300

type: behavior
2018-12-16 20:43:27jaracocreate