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

[Jason R. Coombs]

Jason R. Coombs  added the comment:

Confirmed the fix. Thank you very much!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Karthikeyan Singaravelan]

Change by Karthikeyan Singaravelan :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Chris Withers]

Chris Withers  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[miss-islington]

Change by miss-islington :


--
pull_requests: +12053

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Chris Withers]

Chris Withers  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Karthikeyan Singaravelan]

Change by Karthikeyan Singaravelan :


--
keywords: +patch
pull_requests: +12030
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Mario Corchero]

Mario Corchero  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Mario Corchero]

Mario Corchero  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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}

--
nosy: +cjw296, mariocj89, michael.foord

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Jason R. Coombs]

Jason R. Coombs  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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

--
nosy: +xtreak
type:  -> behavior
versions: +Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Jason R. Coombs]

New submission from Jason R. Coombs :

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)
Eassert {'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/v7hnmq453xb6p2dbz1gqc6rrgn/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:
: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/v7hnmq453xb6p2dbz1gqc6rrgn/T/rwt-pcm3552g/mock/mock.py:1708:
 in __init__
in_dict = _importer(in_dict)
/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rrgn/T/rwt-pcm3552g/mock/mock.py:1210:
 in _importer
thing = _dot_lookup(thing, comp, import_path)
/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rrgn/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?

--
components: Library (Lib)
messages: 331937
nosy: jason.coombs
priority: normal
severity: normal
status: open
title: patch.dict resolves in_dict eagerly (should be late resolved)

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com