bpo-17185: Add __signature__ to mock that can be used by inspect for signature by tirkarthi · Pull Request #11048 · python/cpython

tirkarthi · 2018-12-09T11:36:59Z

Set __signature__ that helps inspect module to detect the signature of the mock.

https://bugs.python.org/issue17185

Edit : Removed partial bug into a separate issue

mariocj89 · 2018-12-09T13:10:01Z

+            pass
+
+        mock = create_autospec(myfunc)
+        assert inspect.getfullargspec(mock) == inspect.getfullargspec(myfunc)


Let’s change all these asserts into ‘self.asserEquals’ it will give a better error report when the test fails.

My bad. I didn't put much thought into refactoring the tests. I think you meant assertEqual since assertEquals is deprecated.

Changed assert statement to use assertEqual. Thanks.

mariocj89 · 2018-12-09T13:11:39Z

-            func = func.__call__
-        except AttributeError:
-            return None
+    if not isinstance(func, functools.partial):


Just a suggestion, to follow the way the code is written, what do you think about:

If is partial:
Pass
Elif is a class:
...

I wrote it this way initially but I will be simply adding pass or func = func like a no op inside the clause and adding if not this way seemed reasonable but I too felt while writing since it kind of seems to make the diff large indicating larger change as in skipping a lot of code.

if isinstance(func, functools.partial): pass elif isinstance(func, type) and not as_instance: # If it's a type and should be modelled as a type, use __init__.

As you wish, it was just a suggestion as that way it could read as each branch is a different type.

I have changed to the suggested branching. Yours looks more inline with the flow 👍 I think adding a comment on no-op would help. How about "For partial objects use the underlying function signature instead of using constructor's signature as below"?

mariocj89 · 2018-12-09T13:13:43Z

+
+            baz = functools.partialmethod(foo, 1)
+
+        mock = create_autospec(Bar)


Can you also validate the signature output for this one?

The signatures are different since inspect includes self and mock skips self as per the patch which I think is the correct way to approach partialmethods?

mariocj89 · 2018-12-09T13:16:23Z

@@ -0,0 +1,2 @@
+Set ``__signature__`` on mock for `inspect` to get signature. Fix signature


Can you use rst markup to reference inspect module?

I think it is :module: just before the backtick.

Add :mod: that references inspect and verified it locally. Thanks 👍

pablogsal · 2018-12-09T23:17:44Z

@@ -0,0 +1,2 @@
+Set ``__signature__`` on mock for `inspect` to get signature. Fix signature
+check for partial and partialmethods. Patch by Karthikeyan Singaravelan.


Please, cross reference with functools.partial and functools.partialmethod using rst markup.

pablogsal · 2018-12-09T23:18:44Z

+        self.assertRaises(TypeError, mock, 1)
+        self.assertRaises(TypeError, mock, 1, 2, 3, c=4)
+
+    def test_spec_inspect_signature_partial(self):


Can we check also against a combination of partial and partialmethod?

Sorry, by combination do you mean a partialmethod taking a partial function as an argument? An example would help here.

Ideally, I'd prefer to see this split out into its own bug and PR. I'm much less certain about the situation with partial, could this be a bug on the partial side that needs fixing there instead? (are partials being created with an incorrect __signature__ or is this something special about what Mock is doing?)

cjw296 · 2018-12-11T06:16:09Z

    """
-    if isinstance(func, type) and not as_instance:
+    if isinstance(func, functools.partial):
+        pass


What's special about functools.partial? It's a bit scary to special case one particular type, when there may be other that behave like it. (also curious how this relates to my original bug report?)

functools.partial with a signature of (*args, **kwargs) returns a partial object and thus when mock checks for signature it checks if it's an object and sets func = func.__init__ which sets the partial function spec as that of the constructor instead of the function. So to skip this I check for func to be a partial.

inspect can detect partial objects but since we set func = func.__init__ it returns the constructor's signature. So this is a case with mock being incorrect with partial.

cjw296 · 2018-12-11T06:18:49Z

            # (if looked up on instance, self is already skipped)
            return is_type
+        elif isinstance(result, functools.partialmethod):
+            return True


Again, concerned about the special casing here...

cjw296 · 2018-12-11T06:19:32Z

+        mock = create_autospec(myfunc)
+        self.assertEqual(inspect.getfullargspec(mock), inspect.getfullargspec(myfunc))
+        mock(1, 2)
+        mock(x=1, y=2)


Would be good to see some assertions about mock_calls here.

cjw296 · 2018-12-11T06:19:43Z

+        self.assertRaises(TypeError, mock, 1)
+
+        def foo(a: int, b: int=10, *, c:int) -> int:
+            return a + b + c


Please split this out into its own test case.

cjw296 · 2018-12-11T06:20:01Z

+        mock = create_autospec(foo)
+        self.assertEqual(inspect.getfullargspec(mock), inspect.getfullargspec(foo))
+        mock(1, 2, c=3)
+        mock(1, c=3)


mock_calls assertions here too please.

cjw296 · 2018-12-11T06:21:42Z

+        self.assertRaises(TypeError, mock, 1)
+        self.assertRaises(TypeError, mock, 1, 2, 3, c=4)
+
+    def test_spec_inspect_signature_partial(self):


Ideally, I'd prefer to see this split out into its own bug and PR. I'm much less certain about the situation with partial, could this be a bug on the partial side that needs fixing there instead? (are partials being created with an incorrect __signature__ or is this something special about what Mock is doing?)

bedevere-bot · 2018-12-11T06:22:07Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

tirkarthi · 2018-12-11T06:47:08Z

@cjw296 Thanks for the review. I stumbled upon the partial and partialmethod bug while working on the issue and adding signature checks helped me catch them so I fixed them here. I agree with you to spin it up as a separate PR and have this PR fix only the original bug report and refactor tests adding mock_calls.

partial and partialmethod bug description is at https://bugs.python.org/issue17185#msg331149. I will create a new issue for it.

tirkarthi · 2018-12-11T14:52:21Z

I have created https://bugs.python.org/issue35463 for partial function issue and reverted the changes from this PR. I have added made the changes suggested to tests and NEWS entry.

@cjw296 I have made the requested changes; please review again.

bedevere-bot · 2018-12-11T14:52:24Z

Thanks for making the requested changes!

@cjw296: please review the changes made to this pull request.

cjw296 · 2018-12-12T07:54:43Z

Perfect thanks!

miss-islington · 2018-12-12T07:55:10Z

Thanks @tirkarthi for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

tirkarthi added 3 commits December 6, 2018 01:06

Fix partial and partial method signatures in mock

ce36d17

Add more calls

e73a549

Add NEWS entry

bf5289b

the-knights-who-say-ni added the CLA signed label Dec 9, 2018

bedevere-bot added the awaiting review label Dec 9, 2018

mariocj89 reviewed Dec 9, 2018

View reviewed changes

Use assertEquals and fix markup in NEWS

3b3f805

pablogsal requested a review from cjw296 December 9, 2018 23:15

pablogsal reviewed Dec 9, 2018

View reviewed changes

Refactor branching and add markup reference for functools

21bfc2c

cjw296 requested changes Dec 11, 2018

View reviewed changes

bedevere-bot removed the awaiting review label Dec 11, 2018

bedevere-bot added the awaiting changes label Dec 11, 2018

Revert partial object related changes and fix pr comments

68bdf07

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 11, 2018

tirkarthi changed the title ~~bpo-17185: Fix partial and partial method signatures in mock~~ bpo-17185: Add __signature__ to mock that can be used by inspect for signature Dec 11, 2018

cjw296 approved these changes Dec 12, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Dec 12, 2018

cjw296 merged commit f7fa62e into python:master Dec 12, 2018

bedevere-bot removed the awaiting merge label Dec 12, 2018

cjw296 added the needs backport to 3.7 label Dec 12, 2018


		baz = functools.partialmethod(foo, 1)

		mock = create_autospec(Bar)

		@@ -0,0 +1,2 @@
		Set ``__signature__`` on mock for `inspect` to get signature. Fix signature

		@@ -0,0 +1,2 @@
		Set ``__signature__`` on mock for `inspect` to get signature. Fix signature
		check for partial and partialmethods. Patch by Karthikeyan Singaravelan.

Uh oh!

Uh oh!

Conversation

tirkarthi commented Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tirkarthi Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 11, 2018

Uh oh!

tirkarthi commented Dec 11, 2018

Uh oh!

tirkarthi commented Dec 11, 2018

Uh oh!

bedevere-bot commented Dec 11, 2018

Uh oh!

cjw296 commented Dec 12, 2018

Uh oh!

miss-islington commented Dec 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tirkarthi commented Dec 9, 2018 •

edited

Loading

tirkarthi Dec 9, 2018 •

edited

Loading