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

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

		pass

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed assert statement to use assertEqual. Thanks.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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__.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?


		baz = functools.partialmethod(foo, 1)

		mock = create_autospec(Bar)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also validate the signature output for this one?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use rst markup to reference inspect module?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

		self.assertRaises(TypeError, mock, 1)
		self.assertRaises(TypeError, mock, 1, 2, 3, c=4)

		def test_spec_inspect_signature_partial(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, concerned about the special casing here...

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to see some assertions about mock_calls here.

		self.assertRaises(TypeError, mock, 1)

		def foo(a: int, b: int=10, *, c:int) -> int:
		return a + b + c

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split this out into its own test case.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mock_calls assertions here too please.

		self.assertRaises(TypeError, mock, 1)
		self.assertRaises(TypeError, mock, 1, 2, 3, c=4)

		def test_spec_inspect_signature_partial(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

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.

@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.

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.

Thanks for making the requested changes!

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

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

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