gh-76273: Fix autospec mocking of instance and class methods

[stephenfin]

Currently, when patching instance / class methods with autospec, their self / cls arguments are not consumed, causing call asserts to fail (they expect an instance / class reference as the first argument).
Example:

from unittest import mock

class Something(object):
    def foo(self, a, b, c, d):
        pass

with mock.patch.object(Something, 'foo', autospec=True):
    s = Something()
    s.foo()

Fix this by skipping the first argument when presented with a method.

Based on #4476 with the changes to the Mock signature stripped out and left for a separate, follow-up PR.

• Issue: mock.patch with autospec does not consume self / cls argument #76273

[johnslavik]

Hi! Nice to see some work on this. Btw, we generally avoid force pushes.

[stephenfin]

Btw, we generally avoid force pushes.

Understood. I've got a small bit more work to do this on to resolve the test_hmac_constructor_uses_builtin failure (we need to handle calling patch.object(cls, name, autospec=True, wraps=wraps) where wraps is unbound). If I push a patch will it be squashed on merge? I can't see anything in the contributor docs in relation to this.

[johnslavik]

If I push a patch will it be squashed on merge?

Yes, all PRs are squashed on merge.

I can't see anything in the contributor docs in relation to this.

I see! It's in https://devguide.python.org/getting-started/pull-request-lifecycle/#don-t-force-push. Maybe we can make it more visible elsewhere, what do you think? Let me know what you had read before you created the PR, maybe we can add it there :-) Thanks for saying that you didn't see that note.

[stephenfin]

Just noting that I do plan to get back to this. I just need to find time to do so.

If I push a patch will it be squashed on merge?

Yes, all PRs are squashed on merge.

Ack, thanks 👌

I can't see anything in the contributor docs in relation to this.

I see! It's in devguide.python.org/getting-started/pull-request-lifecycle#don-t-force-push. Maybe we can make it more visible elsewhere, what do you think? Let me know what you had read before you created the PR, maybe we can add it there :-) Thanks for saying that you didn't see that note.

To be fair, I just looked the Makefile and README in Tools. I hadn't gone as far as looking at external docs.

[github-actions]

This PR is stale because it has been open for 30 days with no activity.

[SeanMooney]

as an aside, i really like using autospec in general but there is a related edge case to what stephen is tryign to fix here that has bit me when backporting chagnes to branches that still support python 3.9

def _writer(fn):
    # oslo.db uses functools.wraps which sets __wrapped__. Python 3.9's
    # mock.autospec follows __wrapped__ and then fails to strip `self` for
    # the bound-method case, shifting positional args by one. Removing
    # __wrapped__ makes autospec see (*args, **kwargs) instead.
    wrapped = main_context_manager.writer(fn)
    del wrapped.__wrapped__
    return wrapped


def _reader(fn):
    wrapped = main_context_manager.reader(fn)
    del wrapped.__wrapped__
    return wrapped 

on 3.9 i had to do that hack to make functions that had multiple layers of decorators

    @oslo_db_api.retry_on_deadlock
    @_writer
    def _do_update_device(self, context, uuid, values):
        query = model_query(context, models.Device)
        query = add_identity_filter(query, uuid)
        try:
            ref = query.with_for_update().one()
        except NoResultFound:
            raise exception.ResourceNotFound(
                resource='Device',
                msg='with uuid=%s' % uuid)

        ref.update(values)
        return ref

actully work properly with autospec in a version independent way.

so it would be good to test with a function that has 2+ levels fo decortors that internally use functools.wraps to ensure that we do not regress the 3.10 behvior adn that the self/cls parmater are properly handeled

[claudiubelu]

Thanks Stephen for following up on the issue on my behalf. Quite a few things changed since my original implementation, especially with async mocks, wraps, etc. It seems that there are quite a few problems left here, as seen in the failing tests.

Since this PR is stale, I've continued it myself too here, and also resolved some more cases (which were causing tests to fail): #154080 and #4476 . I have also sent a few autospec-related PRs as wel, as I've seen these issues in some tests: #153582 and #153580 . Still waiting for reviews on them.

as an aside, i really like using autospec in general but there is a related edge case to what stephen is tryign to fix here that has bit me when backporting chagnes to branches that still support python 3.9

def _writer(fn):
    # oslo.db uses functools.wraps which sets __wrapped__. Python 3.9's
    # mock.autospec follows __wrapped__ and then fails to strip `self` for
    # the bound-method case, shifting positional args by one. Removing
    # __wrapped__ makes autospec see (*args, **kwargs) instead.
    wrapped = main_context_manager.writer(fn)
    del wrapped.__wrapped__
    return wrapped


def _reader(fn):
    wrapped = main_context_manager.reader(fn)
    del wrapped.__wrapped__
    return wrapped 

on 3.9 i had to do that hack to make functions that had multiple layers of decorators

    @oslo_db_api.retry_on_deadlock
    @_writer
    def _do_update_device(self, context, uuid, values):
        query = model_query(context, models.Device)
        query = add_identity_filter(query, uuid)
        try:
            ref = query.with_for_update().one()
        except NoResultFound:
            raise exception.ResourceNotFound(
                resource='Device',
                msg='with uuid=%s' % uuid)

        ref.update(values)
        return ref

actully work properly with autospec in a version independent way.

so it would be good to test with a function that has 2+ levels fo decortors that internally use functools.wraps to ensure that we do not regress the 3.10 behvior adn that the self/cls parmater are properly handeled

Right. That's an unfortunate issue, and it seems to be because inspect module changed between Python 3.9 and Python 3.10 (e6f066a, the fix was not cherry-picked to 3.9), and the change (or lack of it) impacted mock autospecs as well, since it's a direct dependency. This could probably would have been caught with a few more tests, trying those edge cases, but from my experience, it's not like something it's ever going to be ever perfectly tested / gated. FWIW, I've added a test for a similar scenario you've described here: #4476 Nonetheless, autospecs are still a net positive.