patch.dict resolves in_dict eagerly (should be late resolved)

[jaraco]

BPO	35512
Nosy	@jaraco, @cjw296, @voidspace, @mariocj89, @tirkarthi
PRs	bpo-35512: Resolve string target to patch.dict decorator during function call #12000 [3.7] bpo-35512: Resolve string target to patch.dict decorator during function call GHGH-12000 #12021

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-02-26.03:20:26.371>
created_at = <Date 2018-12-16.20:43:27.377>
labels = ['3.8', 'type-bug', 'library']
title = 'patch.dict resolves in_dict eagerly (should be late resolved)'
updated_at = <Date 2019-04-04.18:25:29.105>
user = 'https://github.com/jaraco'

bugs.python.org fields:

activity = <Date 2019-04-04.18:25:29.105>
actor = 'jaraco'
assignee = 'none'
closed = True
closed_date = <Date 2019-02-26.03:20:26.371>
closer = 'xtreak'
components = ['Library (Lib)']
creation = <Date 2018-12-16.20:43:27.377>
creator = 'jaraco'
dependencies = []
files = []
hgrepos = []
issue_num = 35512
keywords = ['patch']
message_count = 11.0
messages = ['331937', '336300', '336307', '336369', '336385', '336389', '336390', '336480', '336555', '336600', '339459']
nosy_count = 5.0
nosy_names = ['jaraco', 'cjw296', 'michael.foord', 'mariocj89', 'xtreak']
pr_nums = ['12000', '12021']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35512'
versions = ['Python 3.8']

[jaraco]

Originally reported in testing-cabal/mock#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?

[tirkarthi]

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]

cpython/Lib/unittest/mock.py

Line 1624 in 3208880

in_dict = _importer(in_dict)

[jaraco]

I agree that’s a good reproducer. Thanks for looking into this!

[tirkarthi]

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}

[mariocj89]

Interesting, patch does resolve it when the patched function is called (see

cpython/Lib/unittest/mock.py

Line 1269 in 175421b

self.target = self.getter()

) vs patch.dict that resolves it at the time the patcher is created - when decorating - (see

cpython/Lib/unittest/mock.py

Line 1624 in 175421b

in_dict = _importer(in_dict)

).

An option might be to delay the resolution as done for patch, changing

cpython/Lib/unittest/mock.py

Line 1625 in 175421b

self.in_dict = in_dict

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.

[tirkarthi]

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?

[mariocj89]

Great, all yours :) I'll be happy to review.

[cjw296]

New changeset a875ea5 by Chris Withers (Xtreak) in branch 'master':
bpo-35512: Resolve string target to patch.dict decorator during function call GH#12000
a875ea5

[cjw296]

New changeset ea199b9 by Chris Withers (Miss Islington (bot)) in branch '3.7':
bpo-35512: Resolve string target to patch.dict decorator during function call GHGH-12000 (bpo-12021)
ea199b9

[tirkarthi]

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 :)

[jaraco]

Confirmed the fix. Thank you very much!