Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jaraco opened this issue Dec 16, 2018 · 11 comments
Closed

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

jaraco opened this issue Dec 16, 2018 · 11 comments
Labels
3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jaraco
Copy link
Member

jaraco commented Dec 16, 2018

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
    Copy link
    Member Author

    jaraco commented Dec 16, 2018

    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?

    @jaraco jaraco added the stdlib Python modules in the Lib dir label Dec 16, 2018
    @tirkarthi
    Copy link
    Member

    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]

    in_dict = _importer(in_dict)

    @tirkarthi tirkarthi added 3.8 only security fixes type-bug An unexpected behavior, bug, or error labels Feb 22, 2019
    @jaraco
    Copy link
    Member Author

    jaraco commented Feb 22, 2019

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

    @tirkarthi
    Copy link
    Member

    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
    Copy link
    Mannequin

    mariocj89 mannequin commented Feb 23, 2019

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

    self.target = self.getter()
    ) vs patch.dict that resolves it at the time the patcher is created - when decorating - (see
    in_dict = _importer(in_dict)
    ).

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

    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
    Copy link
    Member

    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
    Copy link
    Mannequin

    mariocj89 mannequin commented Feb 23, 2019

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

    @cjw296
    Copy link
    Contributor

    cjw296 commented Feb 24, 2019

    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
    Copy link
    Contributor

    cjw296 commented Feb 25, 2019

    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
    Copy link
    Member

    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
    Copy link
    Member Author

    jaraco commented Apr 4, 2019

    Confirmed the fix. Thank you very much!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants