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

mock.call equality surprisingly broken #79407

Closed
cjw296 opened this issue Nov 13, 2018 · 13 comments
Closed

mock.call equality surprisingly broken #79407

cjw296 opened this issue Nov 13, 2018 · 13 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@cjw296
Copy link
Contributor

cjw296 commented Nov 13, 2018

BPO 35226
Nosy @cjw296, @merwok, @voidspace, @asqui, @mariocj89, @tirkarthi
PRs
  • bpo-35226: Fix equality for nested unittest.mock.call objects. #10555
  • [3.6] bpo-35226: Fix equality for nested unittest.mock.call objects. (GH-10555) #10878
  • [3.7] bpo-35226: Fix equality for nested unittest.mock.call objects. (GH-10555) #10879
  • 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 = 'https://github.com/voidspace'
    closed_at = <Date 2018-12-03.22:01:20.649>
    created_at = <Date 2018-11-13.08:05:32.070>
    labels = ['3.7', '3.8', 'type-bug', 'tests']
    title = 'mock.call equality surprisingly broken'
    updated_at = <Date 2018-12-03.22:01:20.648>
    user = 'https://github.com/cjw296'

    bugs.python.org fields:

    activity = <Date 2018-12-03.22:01:20.648>
    actor = 'cjw296'
    assignee = 'michael.foord'
    closed = True
    closed_date = <Date 2018-12-03.22:01:20.649>
    closer = 'cjw296'
    components = ['Tests']
    creation = <Date 2018-11-13.08:05:32.070>
    creator = 'cjw296'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 35226
    keywords = ['patch']
    message_count = 13.0
    messages = ['329813', '329820', '329821', '329823', '329825', '329826', '329827', '329945', '330008', '330577', '330983', '330984', '330985']
    nosy_count = 6.0
    nosy_names = ['cjw296', 'eric.araujo', 'michael.foord', 'dfortunov', 'mariocj89', 'xtreak']
    pr_nums = ['10555', '10878', '10879']
    priority = 'normal'
    resolution = None
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue35226'
    versions = ['Python 2.7', 'Python 3.7', 'Python 3.8']

    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Nov 13, 2018

    $ python3
    Python 3.7.0 (v3.7.0:1bf9cc5093, Jun 26 2018, 23:26:24) 
    [Clang 6.0 (clang-600.0.57)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from unittest.mock import call
    >>> call(x=1) == call(x=2)
    False

    Good so far?

    >>> call(x=1).foo == call(x=2).foo
    True

    Eep, I think a lot of people might find that quite surprising!

    @cjw296 cjw296 added 3.7 (EOL) end of life 3.8 only security fixes labels Nov 13, 2018
    @cjw296 cjw296 added tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error labels Nov 13, 2018
    @tirkarthi
    Copy link
    Member

    I think this is due to the fact that when an attribute of a call object is accessed a new call object is returned with the parent as self.parent [0] but the original information regarding the parent is lost during representation and no check for self.parent equality while the call objects are compared [1]

    ./python.exe
    Python 3.8.0a0 (heads/master:0dc1e45dfd, Nov 13 2018, 11:19:49)
    [Clang 7.0.2 (clang-700.1.81)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from unittest.mock import call
    >>> call(x=2).foo
    call().foo
    >>> call(x=1).foo
    call().foo
    >>> # Comparison returns true since it's similar to call().foo == call().foo though parents are set
    >>> call(x=1).foo == call(x=2).foo
    True
    >>> call(x=1).foo.parent
    call(x=1)
    >>> call(x=2).foo.parent
    call(x=2)
    >>> call(x=2).foo(x=2).bar
    call().foo().bar
    >>> call(x=4).foo(x=3).bar
    call().foo().bar
    >>> # Comparison returns true since it's similar to call().foo().bar == call().foo().bar
    >>> call(x=2).foo(x=2).bar == call(x=4).foo(x=3).bar 
    True
    >>> call(x=4).foo(x=3).bar.parent
    call().foo(x=3)

    Maybe we can add a check for parent to be equal if present?

    diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py
    index a9c82dcb5d..a7565d5f60 100644
    --- a/Lib/unittest/mock.py
    +++ b/Lib/unittest/mock.py
    @@ -2054,6 +2054,10 @@ class _Call(tuple):
    else:
    self_name, self_args, self_kwargs = self

    + if (getattr(self, 'parent', None) and getattr(other, 'parent', None)
    + and self.parent != other.parent):
    + return False
    +
    other_name = ''
    if len_other == 0:
    other_args, other_kwargs = (), {}

    With patch the checks return False and there is no test suite failure but I am not sure if I need to put this inside a while loop to check for nested parents and there maybe other cases that my code doesn't handle.

    ➜ cpython git:(master) ✗ ./python.exe -c 'from unittest.mock import call; print(call(x=2).foo.bar == call(x=1).foo.bar)'
    False
    ➜ cpython git:(master) ✗ ./python.exe -c 'from unittest.mock import call; print(call(x=2).foo == call(x=1).foo)'
    False

    [0]

    return _Call(name=name, parent=self, from_kall=False)

    [1]
    def __eq__(self, other):

    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Nov 13, 2018

    I don't think the getattr(self, 'parent', None) is necessary, the attribute is always there and None == None ;-)

    I reckon this will work:

    if self.parent != other.parent):
        return True

    ...but obviously, we'd add some more tests to the suite to make sure this keeps working.

    If this is the way to go, lemme know and I'll work up a PR.

    @tirkarthi
    Copy link
    Member

    Yes, for call objects it's always there but some of the tests use a tuple during self.assertEquals and hence during equality check other fails with the attribute error for parent like below. Also getattr(self, 'parent', None) != getattr(other, 'parent', None) will skip the attribute error but still fail for tuples in tests I guess with tuple.parent returned as None and checked against a valid parent. Hence the patch with both the checks of getattr and self.parent that passes the test suite. I will wait for input from maintainers if my approach is correct since I am little new to mock :)

    ======================================================================
    ERROR: test_explicit_mock (unittest.test.testmock.testwith.TestMockOpen)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/test/testmock/testwith.py", line 177, in test_explicit_mock
        mock.assert_called_once_with('foo')
      File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 838, in assert_called_once_with
        return self.assert_called_with(*args, **kwargs)
      File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 823, in assert_called_with
        if expected != actual:
      File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 2057, in __eq__
        if self.parent != other.parent:
    AttributeError: 'tuple' object has no attribute 'parent'

    @mariocj89
    Copy link
    Mannequin

    mariocj89 mannequin commented Nov 13, 2018

    If this is to be done we should not change those tests, I am sure there is code validating calls relying on its "tupleness". Example:

    
    >>> import unittest.mock
    >>> m = unittest.mock.Mock()
    >>> m(1)
    >>> m(2, a=1) 
    >>> m.assert_has_calls([
    ...   ((1,), {}),
    ...   ((2,), {'a':1}),
    ... ])
    
    

    This is documented here:

    _Call(('name', (), {})) == ('name',)

    On the addition in general,
    I cannot really comment on the "call(x=1).foo == call(x=2).foo" or how that is supposed to work, I've used those "nesting calls" minimally. I would try to get a hold of Michael Ford.

    As a note, I think nesting calls in unittest.mock.call is supposed to be used only via the https://docs.python.org/3/library/unittest.mock.html#unittest.mock.call.call_list method and for calls only.

    See:

    >>> call(x=1).foo(z=1).call_list() == call(x=1).foo(z=1).call_list()
    True
    >>> call(x=2).foo(z=1).call_list() == call(x=1).foo(z=1).call_list()
    False
    

    which "works as expected".

    Having support for:

    call(x=1).foo == call(x=2).foo is kind of trying to validate an attribute has been accessed, which is a different thing from what mock.calls seems to be doing at the moment. It could be added I guess, but I don't see how do can you use that in mock.assert_has_calls for example. What is the "real life" issue that you faced with this issue? Because creating and comparing calls yourself is not something that is usually done, I guess you compared it to a mock call.

    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Nov 13, 2018

    I'm chatting with Michael already (on Facebook of all places), and I use this feature heavily and deeply, being one of the people who originally requested it.

    I'm both happy and capable of seeing this through, so no need for anyone else to dive in :-)

    Hopefully Michael will follow up when he gets a chance...

    @voidspace
    Copy link
    Contributor

    Parents comparing upwards sounds like the right (and simple) fix. Not breaking the tuple tests would be good. I'm happy for Chris to produce the patch.

    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Nov 15, 2018

    Assuming the PR is merged, what do I need to do for 3.7 and 3.6 backports?
    There's no doubt a doc for this somewhere, so please just point me at it :-)

    @merwok
    Copy link
    Member

    merwok commented Nov 16, 2018

    Add the right «needs backport to X.Y» labels to the PR and a bot will take care of it.

    (This info does not seem to be in the devguide!)

    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Nov 28, 2018

    Éric, doesn't look like I can add labels, what needs to happen for me to do so? (I was/am a core developer, just an extremely inactive one ;-), but happy to jump through whatever hoops necessary... )

    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Dec 3, 2018

    New changeset 8ca0fa9 by Chris Withers in branch 'master':
    bpo-35226: Fix equality for nested unittest.mock.call objects. (bpo-10555)
    8ca0fa9

    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Dec 3, 2018

    New changeset 67e6136 by Chris Withers (Miss Islington (bot)) in branch '3.6':
    bpo-35226: Fix equality for nested unittest.mock.call objects. (GH-10555)
    67e6136

    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Dec 3, 2018

    New changeset e8f9e47 by Chris Withers (Miss Islington (bot)) in branch '3.7':
    bpo-35226: Fix equality for nested unittest.mock.call objects. (GH-10555)
    e8f9e47

    @cjw296 cjw296 closed this as completed Dec 3, 2018
    @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.7 (EOL) end of life 3.8 only security fixes tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants