bpo-38473: Handle autospecced functions and methods used with attach_mock #16784

tirkarthi · 2019-10-14T15:53:58Z

Extract signature from the inner mock object for methods/functions autospecced and attached to a mock object using attach_mock.

https://bugs.python.org/issue38473

tirkarthi · 2019-10-14T16:00:25Z

Lib/unittest/mock.py

@@ -820,7 +820,16 @@ def _get_call_signature_from_name(self, name):
                break
            else:
                children = child._mock_children


Note to self, reviewer : Do we need to default to empty dictionary too guarding against AttributeError for _mock_children?

What test cases could we add to verify that?

…ested children.

tirkarthi · 2020-01-21T16:36:19Z

Sorry, I somehow forgot this issue in my list. Would appreciate a review of this since it caused a regression. cc: @mariocj89

Thanks

mariocj89 · 2020-01-23T15:18:21Z

Lib/unittest/mock.py

-                sig = child._spec_signature
+                try:
+                    sig = child._spec_signature
+                except AttributeError:


In which situation will we fall here? Can you add a tests that exercises this?

I couldn't think of a situation that would get to this as autospecced ones should have this. But given that this was a regression of the previous change without tests catching it I added it as a fallback to have signature as None in case there are scenarios not tested here or try to use getattr with None as default.

I personally don't like the idea of having code "just if", but you wrote this function initially, so... 🤷‍♂ 😛

If we can't prove it's needed, let's drop it until we can. The backport caught up now, just want to get as many upstream PRs merged first before releasing, so we should get good feedback if this code can be reached.

cjw296 · 2020-01-24T07:59:28Z

I'm afraid I don't have a lot to offer here, since I've never used attach_mock.

…ck object.

tirkarthi · 2020-01-24T12:36:35Z

I have removed the try/catch block. From

cpython/Lib/unittest/mock.py

Line 434 in 161e7b3

self._mock_add_spec(spec, spec_set, _spec_as_instance, _eat_self)

_spec_signature seems to be always initialized with None on mock object. The attribute error was due to inner mock object not being extracted which is fixed. I was slightly concerned since the tests didn't catch it and caused a regression. I understand your points on having an untested block as a fallback. I will revisit this if there is a report of it in future. Thanks for your inputs.

mariocj89 · 2020-01-24T12:48:37Z

Lib/unittest/mock.py

@@ -817,6 +817,12 @@ def _get_call_signature_from_name(self, name):
            if child is None or isinstance(child, _SpecState):
                break
            else:
+                # If an autospecced object is attached using attach_mock the
+                # child would be a function with mock object as attribute from
+                # which signature has to be derived. If there is no signature


You might want to review this comment (maybe remove it as well and include it in the commit message).

Thanks for catching this. I prefer to keep it here with the part about fallback updated since it will be good to know why extract_mock is needed. I will add this in the comment over this change.

miss-islington · 2020-01-24T13:20:46Z

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

miss-islington · 2020-01-24T13:20:46Z

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

…mock (pythonGH-16784) (cherry picked from commit 66b00a9) Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>

bedevere-bot · 2020-01-24T13:20:58Z

GH-18166 is a backport of this pull request to the 3.7 branch.

…mock (pythonGH-16784) (cherry picked from commit 66b00a9) Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>

bedevere-bot · 2020-01-24T13:21:05Z

GH-18167 is a backport of this pull request to the 3.8 branch.

…mock (GH-16784) (GH-18167) If an autospecced object is attached using attach_mock the child would be a function with mock object as attribute from which signature has to be derived. (cherry picked from commit 66b00a9) Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>

…mock (GH-16784) (#18166) If an autospecced object is attached using attach_mock the child would be a function with mock object as attribute from which signature has to be derived. (cherry picked from commit 66b00a9) Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>

…mock (pythonGH-16784)

the-knights-who-say-ni added the CLA signed label Oct 14, 2019

bedevere-bot added the awaiting review label Oct 14, 2019

tirkarthi changed the title ~~bpo38473: Handle autospecced method's signature used with attach_mock~~ bpo-38473: Handle autospecced method's signature used with attach_mock Oct 14, 2019

tirkarthi commented Oct 14, 2019

View reviewed changes

tirkarthi changed the title ~~bpo-38473: Handle autospecced method's signature used with attach_mock~~ bpo-38473: Handle autospecced functions and methods used with attach_mock Oct 14, 2019

tirkarthi added 4 commits January 21, 2020 21:53

Use _spec_signature from inner mock for autospec

e9298eb

Add NEWS entry

bae3fac

Add more assertions and a test for function

f524ad9

Loop for the child mock from the mock object itself. Add a test for n…

ae0518c

…ested children.

tirkarthi force-pushed the bpo38473 branch from 992ed04 to ae0518c Compare January 21, 2020 16:34

tirkarthi requested review from cjw296, voidspace and lisroach January 21, 2020 16:34

tirkarthi closed this Jan 21, 2020

tirkarthi reopened this Jan 21, 2020

mariocj89 reviewed Jan 23, 2020

View reviewed changes

mariocj89 approved these changes Jan 23, 2020

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jan 23, 2020

cjw296 removed their request for review January 24, 2020 07:59

Remove try/catch block since _spec_signature is always present for mo…

7bdae66

…ck object.

mariocj89 approved these changes Jan 24, 2020

View reviewed changes

mariocj89 reviewed Jan 24, 2020

View reviewed changes

Update comment about fallback behavior removal.

7701a4e

cjw296 approved these changes Jan 24, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jan 24, 2020

cjw296 merged commit 66b00a9 into python:master Jan 24, 2020

bedevere-bot removed the awaiting merge label Jan 24, 2020

tirkarthi added needs backport to 3.7 labels Jan 24, 2020

bedevere-bot removed the needs backport to 3.7 label Jan 24, 2020

bedevere-bot removed the needs backport to 3.8 label Jan 24, 2020

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-38473: Handle autospecced functions and methods used with attach_…

18786b1

…mock (pythonGH-16784)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-38473: Handle autospecced functions and methods used with attach_mock #16784

bpo-38473: Handle autospecced functions and methods used with attach_mock #16784

tirkarthi commented Oct 14, 2019 •

edited

Loading

tirkarthi Oct 14, 2019

cjw296 Jan 24, 2020

tirkarthi commented Jan 21, 2020

mariocj89 Jan 23, 2020

tirkarthi Jan 23, 2020

mariocj89 Jan 23, 2020

cjw296 Jan 24, 2020

cjw296 commented Jan 24, 2020

tirkarthi commented Jan 24, 2020

mariocj89 Jan 24, 2020

tirkarthi Jan 24, 2020

miss-islington commented Jan 24, 2020

miss-islington commented Jan 24, 2020

bedevere-bot commented Jan 24, 2020

bedevere-bot commented Jan 24, 2020

bpo-38473: Handle autospecced functions and methods used with attach_mock #16784

bpo-38473: Handle autospecced functions and methods used with attach_mock #16784

Conversation

tirkarthi commented Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tirkarthi commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjw296 commented Jan 24, 2020

tirkarthi commented Jan 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miss-islington commented Jan 24, 2020

miss-islington commented Jan 24, 2020

bedevere-bot commented Jan 24, 2020

bedevere-bot commented Jan 24, 2020

tirkarthi commented Oct 14, 2019 •

edited

Loading