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 create_autospec doesn't correctly replace mocksignature #61387

Closed
cjw296 opened this issue Feb 11, 2013 · 9 comments
Closed

unittest mock create_autospec doesn't correctly replace mocksignature #61387

cjw296 opened this issue Feb 11, 2013 · 9 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 Feb 11, 2013

BPO 17185
Nosy @cjw296, @voidspace, @mariocj89, @pablogsal, @tirkarthi
PRs
  • bpo-17185: Add __signature__ to mock that can be used by inspect for signature #11048
  • [3.7] bpo-17185: Add __signature__ to mock that can be used by inspect for signature (GH11048) #11125
  • 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-12.07:58:18.079>
    created_at = <Date 2013-02-11.22:31:40.009>
    labels = ['3.7', '3.8', 'type-bug', 'tests']
    title = "unittest mock create_autospec doesn't correctly replace mocksignature"
    updated_at = <Date 2018-12-12.08:58:40.134>
    user = 'https://github.com/cjw296'

    bugs.python.org fields:

    activity = <Date 2018-12-12.08:58:40.134>
    actor = 'cjw296'
    assignee = 'michael.foord'
    closed = True
    closed_date = <Date 2018-12-12.07:58:18.079>
    closer = 'cjw296'
    components = ['Tests']
    creation = <Date 2013-02-11.22:31:40.009>
    creator = 'cjw296'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 17185
    keywords = ['patch']
    message_count = 9.0
    messages = ['181934', '223380', '223405', '329345', '330987', '331149', '331344', '331673', '331682']
    nosy_count = 5.0
    nosy_names = ['cjw296', 'michael.foord', 'mariocj89', 'pablogsal', 'xtreak']
    pr_nums = ['11048', '11125']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue17185'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Feb 11, 2013

    Sticking an issue in at Michael's request...

    Older versions of mock had a helper called mocksignature.
    In newer versions, create_autospec replaces this, but doesn't get it right sometimes:

    >>> from inspect import getargspec
    >>> from mock import create_autospec
    >>> def myfunc(x, y): pass
    ...
    >>> getargspec(myfunc)
    ArgSpec(args=['x', 'y'], varargs=None, keywords=None, defaults=None)
    >>> getargspec(create_autospec(myfunc))
    ArgSpec(args=[], varargs='args', keywords='kwargs', defaults=None)

    mocksignature gets it right:

    >>> from mock import mocksignature
    >>> getargspec(mocksignature(myfunc))
    ArgSpec(args=['x', 'y'], varargs=None, keywords=None, defaults=None)

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 17, 2014

    Just curiosity why such a name change?

    @BreamoreBoy BreamoreBoy mannequin added the tests Tests in the Lib/test dir label Jul 17, 2014
    @BreamoreBoy BreamoreBoy mannequin changed the title create_autospec unittest mock create_autospec doesn't correctly replace mocksignature Jul 17, 2014
    @BreamoreBoy BreamoreBoy mannequin added the type-bug An unexpected behavior, bug, or error label Jul 17, 2014
    @voidspace
    Copy link
    Contributor

    It was a functionality change, not just a name change.

    @tirkarthi
    Copy link
    Member

    Is this being worked on or can I try fixing this?

    My analysis so far is as below :

    1. For functions : inspect.signature looks for attribute __signature__ for functions and while creating the mock for a function since we already have the signature we can set __signature__ attribute during _set_signature. I don't know why __signature__ was set. I downloaded mock from GitHub to look at the actual mocksignature implementation which uses lambda signature: _mock_signature to form a mock function which I hope is done here too with a function constructed and evald with sig.bind used for parameter checking. I am still new to the mocksignature internals so I couldn't understand how mocksignature worked.

    2. For class and partial functions _check_signature is called and __call__ is overriden in the mock class to check for signature during initialization acting like a constructor. The actual signature will have self along with rest of the parameters but _get_signature_object checks for __init__ and skips the self thus the constructor signature skips self to return a partial function which makes comparing actual constructor call with self and the partial function signature without self to fail.

    Attaching a sample patch with tests. Hope I am on the right track and guidance would help. I am changing the version to 3.8

    diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py
    index a9c82dcb5d..8cbef0e514 100644
    --- a/Lib/unittest/mock.py
    +++ b/Lib/unittest/mock.py
    @@ -103,6 +103,7 @@ def _check_signature(func, mock, skipfirst, instance=False):
             sig.bind(*args, **kwargs)
         _copy_func_details(func, checksig)
         type(mock)._mock_check_sig = checksig
    +    type(mock).__signature__ = sig
    
    
     def _copy_func_details(func, funcopy):
    @@ -172,11 +173,11 @@ def _set_signature(mock, original, instance=False):
         return mock(*args, **kwargs)""" % name
         exec (src, context)
         funcopy = context[name]
    -    _setup_func(funcopy, mock)
    +    _setup_func(funcopy, mock, sig)
         return funcopy

    -def _setup_func(funcopy, mock):
    +def _setup_func(funcopy, mock, sig):
    funcopy.mock = mock

         # can't use isinstance with mocks
    @@ -224,6 +225,7 @@ def _setup_func(funcopy, mock):
         funcopy.assert_called = assert_called
         funcopy.assert_not_called = assert_not_called
         funcopy.assert_called_once = assert_called_once
    +    funcopy.__signature__ = sig
         mock._mock_delegate = funcopy

    Initial set of tests where partial function and class test fails :

    def test_spec_inspect_signature(self):
        def foo(a: int, b: int=10, *, c:int) -> int:
            return a  b  c
    
        mock = create_autospec(foo)
        assert inspect.getfullargspec(mock) == inspect.getfullargspec(foo)
        self.assertRaises(TypeError, mock, 1)
    
    def test_spec_inspect_signature_partial(self):
        def foo(a: int, b: int=10, *, c:int) -> int:
            return a  b  c
    
        import functools
    
        partial_object = functools.partial(foo, 1)
        mock = create_autospec(partial_object)
        assert inspect.getfullargspec(mock) == inspect.getfullargspec(partial_object) # Fails
        self.assertRaises(TypeError, partial_object)
    
    def test_spec_inspect_signature_class(self):
        class Bar:
            def __init__(self, a: int):
                self.a = a
    
        mock = create_autospec(Bar)
        assert inspect.getfullargspec(mock) == inspect.getfullargspec(Bar) # Fails since mock signature has no self
        self.assertRaises(TypeError, mock)
        self._check_someclass_mock(mock)

    @tirkarthi tirkarthi added the 3.8 only security fixes label Nov 6, 2018
    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Dec 3, 2018

    xtreak - great to see action on this! First step would be to add a unit test for the failure case I reported. I like the tests you have too, but would be good to see the specific failure case covered too.

    Beyond that, if we can get all the the new tests passing, then I'd say you're there!

    @cjw296 cjw296 added the 3.7 (EOL) end of life label Dec 3, 2018
    @tirkarthi
    Copy link
    Member

    While working on partials test case failure I found two more cases along the way.

    1. When we call create_autospec it calls _get_signature_object that gets the signature for the given parameter. With functools.partial it returns a partial object and hence while getting the signature it returns the signature for the constructor of partial instead of the underlying function passed to functools.partial. I think a check needs to be added to make sure not to use func.__init__ when it's a partial object.

    2. When we call create_autospect on a class that has a partialmethod the self parameter is not skipped in the signature and thus it creates a signature with self causing error. The fix would be to handle partialmethod also in _must_skip that determines whether to skip self or not.

    3. It also seems that inspect.getfullargspec doesn't work for magic mocks which I hope is now fixed with __signature__ set in my patch

    To be honest I don't if my getting hacky at this point I will clean up the code in a couple of days and raise a PR for initial feedback. Current changes are at master...tirkarthi:bpo-17185. I am adding @mariocj89 to the issue.

    Sample reproducer :

    from functools import partial, partialmethod
    from unittest.mock import create_autospec
    import inspect
    
    def foo(a, b):
        pass
    
    p = partial(foo, 1)
    m = create_autospec(p)
    m(1, 2, 3) # passes since signature is set as (*args, **kwargs) the signature of functools.partial constructor. This should throw TypeError under autospec
    
    
    class A:
    
        def f(self, a, b):
            print(a, b)
    
        g = partialmethod(f, 1)
    
    m = create_autospec(A)
    m().g(1, 2) # passes since signature is set as (self, b) and self is not skipped in _must_skip thus self=1, b=2. This should throw TypeError under autospec since the valid call is m().g(2)

    @mariocj89
    Copy link
    Mannequin

    mariocj89 mannequin commented Dec 7, 2018

    Agree that it sounds reasonable to just set __signature__ to tell inspect where to look at (thanks PEP-362 :P). Not an expert here though.

    For the partials bug, I'll add Pablo as we were speaking today about something similar :) but that might be unrelated (though interesting and worth fixing!) from the issue Chris brought up.

    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Dec 12, 2018

    New changeset f7fa62e by Chris Withers (Xtreak) in branch 'master':
    bpo-17185: Add __signature__ to mock that can be used by inspect for signature (GH11048)
    f7fa62e

    @cjw296 cjw296 closed this as completed Dec 12, 2018
    @cjw296
    Copy link
    Contributor Author

    cjw296 commented Dec 12, 2018

    New changeset 6a12931 by Chris Withers (Miss Islington (bot)) in branch '3.7':
    bpo-17185: Add __signature__ to mock that can be used by inspect for signature (GH11125)
    6a12931

    @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

    3 participants