gh-85934: Use getattr_static when adding mock spec #22209
Conversation
|
Note: I have signed the CLA earlier today, so the records should be updated sometime next week. |
|
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 |
Yes, when you create a mock and provide a spec to it (a class or object) the mock will be created to only allow the interface of the class spec. That way, arbitrary calls upon your mock with invalid methods or attributes will fail in your test. In order to create the mock according to the spec, it needs to inspect the class or object but ideally it should not call any methods on the object or change its state (IMHO) while doing so. Prior to Python 3.8 it did not, but the more recent async mock support checks whether methods are coroutine functions and its use of getattr is now causing object properties to be called. Here's a section in the docs that explains speccing as part of an explanation of auto-speccing: https://docs.python.org/3/library/unittest.mock.html#autospeccing |
|
Gah, I have really messed this up by failing at using the UI here. Apologies, working on fixing it. |
|
Windows test suite failure appears unrelated [1]: [1] https://github.com/python/cpython/pull/22209/checks?check_run_id=1136737699#step:5:4914 |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @terryjreedy: please review the changes made to this pull request. |
|
I could not download logs, so when one can, better to briefly summarize the failure in a comment. Ah, you did, but I did not see on email. I triggered the minimum retest possible. |
terryjreedy
dismissed
their stale review
Changes made, but not ready to approve for merging.
Sorry about that, I'm a bit new to the github PR workflow for code review and am missing some things. After I wrote "appears unrelated" and posted it, I realized I should have included a paste of the error and a link to the log so I made an edit afterward. Thanks for your patience. |
melwitt
force-pushed
the
unittest.mock-getattr
branch
from
5722f37 to
4e3cb45
Compare
Since commit 77b3b77, class properties are being called when adding mock specs and this can introduce side effects for tests that are verifying state affected by code inside a @Property. This replaces the getattr call with a inspect.getattr_static call to avoid executing class code as part of the mock speccing process.
The goal of the property is provide access to the object and create it lazily on first access.
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
There was a problem hiding this comment.
The two possible concerns here are that getattr_static is slower than getattr, and backward-compatibility. But I think mock speccing executing properties is a bug, and fixing that bug outweighs those two concerns. Also, @AlexWaygood made getattr_static faster in 3.12. And this behavior of mock spec executing properties was introduced relatively recently (with AsyncMock); that change was a back-compat break from previous longstanding behavior, and this is the fix for it.
Because this is a bug, I think the fix should also be backported.
Misc/NEWS.d/next/Library/2020-09-16-16-53-06.bpo-41768.8_fWkC.rst
Outdated
Show resolved
Hide resolved
arhadthedev
changed the title
Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
There was a problem hiding this comment.
Currently async methods decorated with @classmethod or @staticmethod are detected as async attributes. This PR causes them not to be anymore. I think we need to preserve this behavior (and add tests for it.) We'll have to follow __wrapped__ to the decorated method, and test it with iscoroutinefunction. We can do this using inspect.unwrap.
|
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 (per bedevere-bot) |
|
Thanks for making the requested changes! @carljm: please review the changes made to this pull request. |
|
Update: looks like this is already documented and is being worked on: #104341 AFAICT the Full traceback |
|
Thanks @melwitt for the patch! |
|
When async magic method support was added to
unittest.mock.Mockto address issue #26467, it introduced agetattrcall [1] that causes class properties to be called when the class is used as a mock spec.This caused a problem for a test in my project when running with Python 3.8 where previously the test worked OK with Python 3.6.
The test aims to verify that a class instance is not created if the called code path does not access the class property and thus the class will not create a heavy object unless it's needed (lazy create on access via
@property).As of Python 3.8, the
@propertyis always called and is called by the mock spec process itself, even though the code path being tested does not access the class@property.Here is a code snippet that illustrates the
@propertycalling from the mock spec alone:In summary, since commit 77b3b77, class properties are being called when adding mock specs and this can introduce side effects for tests that are verifying state affected by code inside a
@property.This replaces the
getattrcall with ainspect.getattr_staticcall to avoid executing class code as part of the mock speccing process.[1]
cpython/Lib/unittest/mock.py
Line 492 in fb27187
https://bugs.python.org/issue41768