Skip to content

/ cpython

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

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

Merged
merged 6 commits into from Jan 24, 2020
Merged

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

merged 6 commits into from Jan 24, 2020
+35 −0

Conversation

@tirkarthi
Copy link
Member

tirkarthi commented Oct 14, 2019 •
edited

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 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
tirkarthi reviewed Oct 14, 2019
View changes
Lib/unittest/mock.py Outdated
@@ -820,7 +820,16 @@ def _get_call_signature_from_name(self, name):
break
else:
children = child._mock_children

This comment has been minimized.

Sign in to view
Copy link
@tirkarthi

tirkarthi Oct 14, 2019

Author Member

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

This comment has been minimized.

Sign in to view
Copy link
@cjw296

cjw296 Jan 24, 2020

Contributor

What test cases could we add to verify that?
@tirkarthi 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 Oct 14, 2019
@tirkarthi
Use _spec_signature from inner mock for autospec
e9298eb
@tirkarthi
Add NEWS entry
bae3fac
@tirkarthi
Add more assertions and a test for function
f524ad9
@tirkarthi
Loop for the child mock from the mock object itself. Add a test for n…
Loading status checks…
ae0518c 
…ested children.
@tirkarthi tirkarthi force-pushed the tirkarthi:bpo38473 branch from 992ed04 to ae0518c Jan 21, 2020
@tirkarthi tirkarthi requested review from cjw296, voidspace and lisroach Jan 21, 2020
@tirkarthi

This comment has been minimized.

Sign in to view
Copy link
Member Author

tirkarthi commented Jan 21, 2020

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

Thanks
@tirkarthi tirkarthi closed this Jan 21, 2020
@tirkarthi tirkarthi reopened this Jan 21, 2020
@mariocj89
mariocj89 reviewed Jan 23, 2020
View changes
Lib/unittest/mock.py Outdated
sig = child._spec_signature
try:
sig = child._spec_signature
except AttributeError:

This comment has been minimized.

Sign in to view
Copy link
@mariocj89

mariocj89 Jan 23, 2020

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@tirkarthi

tirkarthi Jan 23, 2020

Author Member

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.

This comment has been minimized.

Sign in to view
Copy link
@mariocj89

mariocj89 Jan 23, 2020

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@cjw296

cjw296 Jan 24, 2020

Contributor

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.
@mariocj89
mariocj89 approved these changes Jan 23, 2020
View changes
@cjw296

This comment has been minimized.

Sign in to view
Copy link
Contributor

cjw296 commented Jan 24, 2020

I'm afraid I don't have a lot to offer here, since I've never used attach_mock.
@cjw296 cjw296 removed their request for review Jan 24, 2020
@tirkarthi
Remove try/catch block since _spec_signature is always present for mo…
Loading status checks…
7bdae66 
…ck object.
@tirkarthi

This comment has been minimized.

Sign in to view
Copy link
Member Author

tirkarthi commented Jan 24, 2020

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
mariocj89 approved these changes Jan 24, 2020
View changes
@mariocj89
mariocj89 reviewed Jan 24, 2020
View changes
Lib/unittest/mock.py Outdated
@@ -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

This comment has been minimized.

Sign in to view
Copy link
@mariocj89

mariocj89 Jan 24, 2020

Contributor

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

This comment has been minimized.

Sign in to view
Copy link
@tirkarthi

tirkarthi Jan 24, 2020

Author Member

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.
@tirkarthi
Update comment about fallback behavior removal.
Loading status checks…
7701a4e
@cjw296
cjw296 approved these changes Jan 24, 2020
View changes
@cjw296 cjw296 merged commit 66b00a9 into python:master Jan 24, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200124.32 succeeded
Details
bedevere/issue-number Issue number 38473 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Jan 24, 2020

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

This comment has been minimized.

Sign in to view
Copy link

miss-islington commented Jan 24, 2020

Thanks @tirkarthi for the PR, and @cjw296 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖
miss-islington added a commit to miss-islington/cpython that referenced this pull request Jan 24, 2020
@tirkarthi @miss-islington
bpo-38473: Handle autospecced functions and methods used with attach_…
516b885 
…mock (pythonGH-16784)

(cherry picked from commit 66b00a9)

Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Jan 24, 2020

GH-18166 is a backport of this pull request to the 3.7 branch.
miss-islington added a commit to miss-islington/cpython that referenced this pull request Jan 24, 2020
@tirkarthi @miss-islington
bpo-38473: Handle autospecced functions and methods used with attach_…
44ccf4f 
…mock (pythonGH-16784)

(cherry picked from commit 66b00a9)

Co-authored-by: Karthikeyan Singaravelan <tir.karthi@gmail.com>
@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

bedevere-bot commented Jan 24, 2020

GH-18167 is a backport of this pull request to the 3.8 branch.
tirkarthi added a commit that referenced this pull request Jan 25, 2020
@miss-islington @tirkarthi
bpo-38473: Handle autospecced functions and methods used with attach_…
Loading status checks…
a5906b2 
…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>
tirkarthi added a commit that referenced this pull request Jan 25, 2020
@miss-islington @tirkarthi
bpo-38473: Handle autospecced functions and methods used with attach_…
Loading status checks…
71d2b33 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariocj89 mariocj89

@cjw296 cjw296

@voidspace voidspace

@lisroach lisroach

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
6 participants
@tirkarthi @cjw296 @miss-islington @bedevere-bot @mariocj89 @the-knights-who-say-ni
You can’t perform that action at this time.