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

unittest.mock does not wrap dunder methods (__getitem__ etc) #69783

Open
DarraghBailey mannequin opened this issue Nov 10, 2015 · 12 comments
Open

unittest.mock does not wrap dunder methods (__getitem__ etc) #69783

DarraghBailey mannequin opened this issue Nov 10, 2015 · 12 comments
Labels
3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@DarraghBailey
Copy link
Mannequin

DarraghBailey mannequin commented Nov 10, 2015

BPO 25597
Nosy @rbtcollins, @cjw296, @bitdancer, @voidspace, @phmc, @lisroach, @tonybaloney, @mariocj89, @tirkarthi, @akulakov
PRs
  • bpo-25597: Ensure wraps' return value is used for magic methods in MagicMock #16029
  • bpo-39966: Revert "bpo-25597: Ensure wraps' return value is used for magic methods in MagicMock (#16029)" #19734
  • Files
  • test-mock-wraps-dict.py
  • 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 = None
    created_at = <Date 2015-11-10.17:02:38.914>
    labels = ['type-bug', 'library', '3.9']
    title = 'unittest.mock does not wrap dunder methods (__getitem__ etc)'
    updated_at = <Date 2021-08-28.03:05:05.247>
    user = 'https://bugs.python.org/DarraghBailey'

    bugs.python.org fields:

    activity = <Date 2021-08-28.03:05:05.247>
    actor = 'andrei.avk'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2015-11-10.17:02:38.914>
    creator = 'Darragh Bailey'
    dependencies = []
    files = ['41001']
    hgrepos = []
    issue_num = 25597
    keywords = ['patch']
    message_count = 12.0
    messages = ['254453', '254466', '261835', '341615', '351469', '351824', '352124', '352125', '360739', '367559', '367623', '400457']
    nosy_count = 11.0
    nosy_names = ['rbcollins', 'cjw296', 'r.david.murray', 'michael.foord', 'pconnell', 'Darragh Bailey', 'lisroach', 'anthonypjshaw', 'mariocj89', 'xtreak', 'andrei.avk']
    pr_nums = ['16029', '19734']
    priority = 'normal'
    resolution = None
    stage = None
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue25597'
    versions = ['Python 3.9']

    @DarraghBailey
    Copy link
    Mannequin Author

    DarraghBailey mannequin commented Nov 10, 2015

    Both unittest.mock and the backported release for earlier pythons don't appear to support mocking of dictionary objects.

    Specifically I'm expecting that any of the methods used to test for membership, or get items from a mock object wrapping a dictionary should succeed. However it appears that MagicMock doesn't appear to support this.

    Attached file shows an attempt to use different methods with a wrapped dictionary object where only the '.get()' method appears to work as expected.

    @DarraghBailey DarraghBailey mannequin added the type-bug An unexpected behavior, bug, or error label Nov 10, 2015
    @bitdancer
    Copy link
    Member

    Looking at the source, it's not clear that wraps is supported for __ methods, despite what the documentation implies. That is, MagicProxy doesn't seem to look at the wraps information. I suspect it is doable, but it may be an enhancement request rather than a bug fix, I'm not sure.

    @rbtcollins
    Copy link
    Member

    I think this is a valid mock bug; it likely needs some thoughtful exhaustive testing, and obviously support added for it.

    @rbtcollins rbtcollins changed the title unittest.mock does not wrap dict objects correctly unittest.mock does not wrap dunder methods (__getitem__ etc) Mar 15, 2016
    @tonybaloney
    Copy link
    Mannequin

    tonybaloney mannequin commented May 6, 2019

    The assertions in the attached test still fail on master (3.8a3), so this still applies.

    Michael, are you able to look at this, the code hasn't changed since the original PEP-417 implementation, which doesn't specify if this behaviour should be supported.

    The documentation does not specify that this is supported also, so i suspect this is an enhancement request.

        elif result is None:
            wraps = None
            if self._mock_wraps is not None:
                # XXXX should we get the attribute without triggering code
                # execution?
                wraps = getattr(self._mock_wraps, name)
    
                result = self._get_child_mock(
                    parent=self, name=name, wraps=wraps, _new_name=name,
                    _new_parent=self
                )

    @tirkarthi
    Copy link
    Member

    This seems to a reasonable change to me since dict.get returns the value then making a contains check dict.__contains__ should return True instead of the fixed return value of False. Below is a patch where the mock_wraps attribute is set with the relevant method that would make sure in the report dict.get would used.

    diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py
    index 298b41e0d7..077d22d08e 100644
    --- a/Lib/unittest/mock.py
    +++ b/Lib/unittest/mock.py
    @@ -1935,6 +1935,12 @@ _side_effect_methods = {

     def _set_return_value(mock, method, name):
    +    # If _mock_wraps is present then attach it so that it's return
    +    # value is used when called.
    +    if mock._mock_wraps is not None:
    +        method._mock_wraps = getattr(mock._mock_wraps, name)
    +        return
    +
         fixed = _return_values.get(name, DEFAULT)
         if fixed is not DEFAULT:
             method.return_value = fixed

    @tirkarthi
    Copy link
    Member

    Michael, any thoughts on this? This more feels like an enhancement to me and I have marked it as 3.9 since I am not sure someone might be depending on the behavior where they have used wraps but still expect default values for magicmethods as they do now.

    @tirkarthi tirkarthi added stdlib Python modules in the Lib dir 3.9 only security fixes labels Sep 11, 2019
    @voidspace
    Copy link
    Contributor

    As discussed with Karthik, I think this is a nice feature enhancement for the wraps functionality and worth fixing. It has the great advantage that the fix is nice and isolated and simple.

    @voidspace
    Copy link
    Contributor

    The previous behaviour was unspecified and clearly due to missing functionality, so the advantages of fixing it outweigh any potential compatibility issues. But I'd see it as a feature enhancement for 3.9.

    @cjw296
    Copy link
    Contributor

    cjw296 commented Jan 27, 2020

    New changeset 72b1004 by Chris Withers (Karthikeyan Singaravelan) in branch 'master':
    bpo-25597: Ensure wraps' return value is used for magic methods in MagicMock (bpo-16029)
    72b1004

    @cjw296
    Copy link
    Contributor

    cjw296 commented Apr 28, 2020

    New changeset 521c8d6 by Karthikeyan Singaravelan in branch 'master':
    bpo-39966: Revert "bpo-25597: Ensure wraps' return value is used for magic methods in MagicMock" (GH-19734)
    521c8d6

    @tirkarthi
    Copy link
    Member

    The change has been reverted as per bpo-39966. I am reopening this for further discussion.

    @tirkarthi tirkarthi reopened this Apr 29, 2020
    @akulakov
    Copy link
    Contributor

    I went through dunder methods to check if any other operators or builtins work on objects without respective dunder methods:

    • del x works even though there is no object.__del__

    • operator.length_hint() => 0 when there is no object.__length_hint__

    So in addition to __bool__, these dunder methods would have to be special cased.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    Status: No status
    Development

    No branches or pull requests

    6 participants