Bug in mock running on PyPy3

[cfbolz]

BPO	39485
Nosy	@cfbolz, @cjw296
PRs	bpo-39485: fix corner-case in method-detection of mock #18252 [3.8] bpo-39485: fix corner-case in method-detection of mock (GH-18252) #18255 [3.7] bpo-39485: fix corner-case in method-detection of mock (GH-18252) #18256

^{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/cjw296'
closed_at = <Date 2020-01-29.16:20:19.067>
created_at = <Date 2020-01-29.13:49:55.621>
labels = []
title = 'Bug in mock running on PyPy3'
updated_at = <Date 2020-01-29.16:20:19.066>
user = 'https://github.com/cfbolz'

bugs.python.org fields:

activity = <Date 2020-01-29.16:20:19.066>
actor = 'cjw296'
assignee = 'cjw296'
closed = True
closed_date = <Date 2020-01-29.16:20:19.067>
closer = 'cjw296'
components = []
creation = <Date 2020-01-29.13:49:55.621>
creator = 'Carl.Friedrich.Bolz'
dependencies = []
files = []
hgrepos = []
issue_num = 39485
keywords = ['patch']
message_count = 5.0
messages = ['360961', '360968', '360969', '360970', '360971']
nosy_count = 2.0
nosy_names = ['Carl.Friedrich.Bolz', 'cjw296']
pr_nums = ['18252', '18255', '18256']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue39485'
versions = []

[cfbolz]

One of the new-in-3.8 tests for unittest.mock, test_spec_has_descriptor_returning_function, is failing on PyPy. This exposes a bug in unittest.mock. The bug is most noticeable on PyPy, where it can be triggered by simply writing a slightly weird descriptor (CrazyDescriptor in the test). Getting it to trigger on CPython would be possible too, by implementing the same descriptor in C, but I did not actually do that.

The relevant part of the test looks like this:

from unittest.mock import create_autospec

class CrazyDescriptor(object):
    def __get__(self, obj, type_):
        if obj is None:
            return lambda x: None

class MyClass(object):

    some_attr = CrazyDescriptor()

mock = create_autospec(MyClass)
mock.some_attr(1)

On CPython this just works, on PyPy it fails with:

Traceback (most recent call last):
  File "x.py", line 13, in <module>
    mock.some_attr(1)
  File "/home/cfbolz/bin/.pyenv/versions/pypy3.6-7.2.0/lib-python/3/unittest/mock.py", line 938, in __call__
    _mock_self._mock_check_sig(*args, **kwargs)
  File "/home/cfbolz/bin/.pyenv/versions/pypy3.6-7.2.0/lib-python/3/unittest/mock.py", line 101, in checksig
    sig.bind(*args, **kwargs)
  File "/home/cfbolz/bin/.pyenv/versions/pypy3.6-7.2.0/lib-python/3/inspect.py", line 3034, in bind
    return args[0]._bind(args[1:], kwargs)
  File "/home/cfbolz/bin/.pyenv/versions/pypy3.6-7.2.0/lib-python/3/inspect.py", line 2955, in _bind
    raise TypeError('too many positional arguments') from None
TypeError: too many positional arguments

The reason for this problem is that mock deduced that MyClass.some_attr is a method on PyPy. Since mock thinks the lambda returned by the descriptor is a method, it adds self as an argument, which leads to the TypeError.

Checking whether something is a method is done by _must_skip in mock.py. The relevant condition is this one:

    elif isinstance(getattr(result, '__get__', None), MethodWrapperTypes):
        # Normal method => skip if looked up on type
        # (if looked up on instance, self is already skipped)
        return is_type
    else:
        return False

MethodWrapperTypes is defined as:

MethodWrapperTypes = (
    type(ANY.__eq__.__get__),
)

which is just types.MethodType on PyPy, because there is no such thing as a method wrapper (the builtin types look pretty much like python-defined types in PyPy).

On PyPy the condition isinstance(getattr...) is thus True for all descriptors! so as soon as result has a __get__, it counts as a method, even in the above case where it's a custom descriptor.

Now even on CPython the condition makes no sense to me. It would be True for a C-defined version of CrazyDescriptor, it's just not a good way to check whether result is a method.

I would propose to replace the condition with the much more straightforward check:

    elif isinstance(result, FunctionTypes):
        ...

something is a method if it's a function on the class. Doing that change makes the test pass on PyPy, and doesn't introduce any test failures on CPython either.

Will open a pull request.

[cjw296]

New changeset a327677 by Carl Friedrich Bolz-Tereick in branch 'master':
bpo-39485: fix corner-case in method-detection of mock (GH-18252)
a327677

[cjw296]

New changeset cf0645a by Miss Islington (bot) in branch '3.7':
bpo-39485: fix corner-case in method-detection of mock (GH-18256)
cf0645a

[cjw296]

New changeset 696d232 by Miss Islington (bot) in branch '3.8':
bpo-39485: fix corner-case in method-detection of mock (GH-18255)
696d232

[cjw296]

Thank you very much for this, that was a really good catch!