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. |
Sorry, something went wrong.
terryjreedy
left a comment
There was a problem hiding this comment.
I do not know the mock specification well enough to be sure of what it should do, except that modifying the underlying object is likely wrong, and that the mock should actually mock it. So the code patch is likely correct and an additional test seems needed.
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
Gah, I have really messed this up by failing at using the UI here. Apologies, working on fixing it. |
Sorry, something went wrong.
|
Windows test suite failure appears unrelated [1]: [1] https://github.com/python/cpython/pull/22209/checks?check_run_id=1136737699#step:5:4914 |
Sorry, something went wrong.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
|
Thanks for making the requested changes! @terryjreedy: please review the changes made to this pull request. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
terryjreedy
left a comment
There was a problem hiding this comment.
Changes match those requested. I am hoping someone else reviews this, but I will read spec reference if not.
Sorry, something went wrong.
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. |
Sorry, something went wrong.
5722f37 to
4e3cb45
Compare
August 19, 2021 15:38
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.
Sorry, something went wrong.
Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>
carljm
left a comment
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.
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
I have made the requested changes; please review again (per bedevere-bot) |
Sorry, something went wrong.
|
Thanks for making the requested changes! @carljm: please review the changes made to this pull request. |
Sorry, something went wrong.
|
Update: looks like this is already documented and is being worked on: #104341 AFAICT the Full traceback |
Sorry, something went wrong.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot aarch64 Debian Clang LTO + PGO 3.x has failed when building commit 2e09310. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/all/#builders/1084/builds/905 Summary of the results of the build (if available): == Tests result: ENV CHANGED == 446 tests OK. 10 slowest tests:
1 test altered the execution environment: 18 tests skipped: Total duration: 4 min 9 sec Click to see traceback logsremote: Enumerating objects: 26, done.
remote: Counting objects: 3% (1/26)
remote: Counting objects: 7% (2/26)
remote: Counting objects: 11% (3/26)
remote: Counting objects: 15% (4/26)
remote: Counting objects: 19% (5/26)
remote: Counting objects: 23% (6/26)
remote: Counting objects: 26% (7/26)
remote: Counting objects: 30% (8/26)
remote: Counting objects: 34% (9/26)
remote: Counting objects: 38% (10/26)
remote: Counting objects: 42% (11/26)
remote: Counting objects: 46% (12/26)
remote: Counting objects: 50% (13/26)
remote: Counting objects: 53% (14/26)
remote: Counting objects: 57% (15/26)
remote: Counting objects: 61% (16/26)
remote: Counting objects: 65% (17/26)
remote: Counting objects: 69% (18/26)
remote: Counting objects: 73% (19/26)
remote: Counting objects: 76% (20/26)
remote: Counting objects: 80% (21/26)
remote: Counting objects: 84% (22/26)
remote: Counting objects: 88% (23/26)
remote: Counting objects: 92% (24/26)
remote: Counting objects: 96% (25/26)
remote: Counting objects: 100% (26/26)
remote: Counting objects: 100% (26/26), done.
remote: Compressing objects: 7% (1/14)
remote: Compressing objects: 14% (2/14)
remote: Compressing objects: 21% (3/14)
remote: Compressing objects: 28% (4/14)
remote: Compressing objects: 35% (5/14)
remote: Compressing objects: 42% (6/14)
remote: Compressing objects: 50% (7/14)
remote: Compressing objects: 57% (8/14)
remote: Compressing objects: 64% (9/14)
remote: Compressing objects: 71% (10/14)
remote: Compressing objects: 78% (11/14)
remote: Compressing objects: 85% (12/14)
remote: Compressing objects: 92% (13/14)
remote: Compressing objects: 100% (14/14)
remote: Compressing objects: 100% (14/14), done.
remote: Total 14 (delta 12), reused 1 (delta 0), pack-reused 0
From https://github.com/python/cpython
* branch main -> FETCH_HEAD
Note: switching to '2e0931046dcc200fd6abb2cdfaf57d8b99117c57'.
You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.
If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:
git switch -c <new-branch-name>
Or undo this operation with:
git switch -
Turn off this advice by setting config variable advice.detachedHead to false
HEAD is now at 2e0931046d gh-85934: Use getattr_static when adding mock spec (#22209)
Switched to and reset branch 'main'
./configure: line 8401: -reorder-blocks=ext-tsp: command not found
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
make[2]: [Makefile:2785: clean-retain-profile] Error 1 (ignored)
./Modules/expat/xmlparse.c:3116:9: warning: code will never be executed [-Wunreachable-code]
parser->m_characterDataHandler(parser->m_handlerArg, parser->m_dataBuf,
^~~~~~
./Modules/expat/xmlparse.c:3115:16: note: silence by adding parentheses to mark code as explicitly dead
else if (0 && parser->m_characterDataHandler)
^
/* DISABLES CODE */ ( )
./Modules/expat/xmlparse.c:4059:9: warning: code will never be executed [-Wunreachable-code]
parser->m_characterDataHandler(parser->m_handlerArg, parser->m_dataBuf,
^~~~~~
./Modules/expat/xmlparse.c:4058:16: note: silence by adding parentheses to mark code as explicitly dead
else if (0 && parser->m_characterDataHandler)
^
/* DISABLES CODE */ ( )
2 warnings generated.
warning: no profile data available for file "getpath_noop.c" [-Wprofile-instr-unprofiled]
warning: profile data may be out of date: of 14 functions, 1 has mismatched data that will be ignored [-Wprofile-instr-out-of-date]
1 warning generated.
1 warning generated.
warning: no profile data available for file "_csv.c" [-Wprofile-instr-unprofiled]
warning: no profile data available for file "rotatingtree.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "_lsprof.c" [-Wprofile-instr-unprofiled]
1 warning generated.
1 warning generated.
warning: no profile data available for file "_xxinterpchannelsmodule.c" [-Wprofile-instr-unprofiled]
warning: no profile data available for file "audioop.c" [-Wprofile-instr-unprofiled]
warning: no profile data available for file "_xxsubinterpretersmodule.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "_zoneinfo.c" [-Wprofile-instr-unprofiled]
1 warning generated.
1 warning generated.
1 warning generated.
./Modules/expat/xmlparse.c:3116:9: warning: code will never be executed [-Wunreachable-code]
parser->m_characterDataHandler(parser->m_handlerArg, parser->m_dataBuf,
^~~~~~
./Modules/expat/xmlparse.c:3115:16: note: silence by adding parentheses to mark code as explicitly dead
else if (0 && parser->m_characterDataHandler)
^
/* DISABLES CODE */ ( )
./Modules/expat/xmlparse.c:4059:9: warning: code will never be executed [-Wunreachable-code]
parser->m_characterDataHandler(parser->m_handlerArg, parser->m_dataBuf,
^~~~~~
./Modules/expat/xmlparse.c:4058:16: note: silence by adding parentheses to mark code as explicitly dead
else if (0 && parser->m_characterDataHandler)
^
/* DISABLES CODE */ ( )
2 warnings generated.
warning: no profile data available for file "_cryptmodule.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "grpmodule.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "mmapmodule.c" [-Wprofile-instr-unprofiled]
warning: no profile data available for file "nismodule.c" [-Wprofile-instr-unprofiled]
1 warning generated.
1 warning generated.
warning: no profile data available for file "ossaudiodev.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "syslogmodule.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "spwdmodule.c" [-Wprofile-instr-unprofiled]
1warning: no profile data available for file "posixshmem.c" [-Wprofile-instr-unprofiled]
warning generated.
1 warning generated.
warning: no profile data available for file "_curses_panel.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "_cursesmodule.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "_uuidmodule.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "_xxtestfuzz.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "xxsubtype.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "fuzzer.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "_testimportmultiple.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "_testsinglephase.c" [-Wprofile-instr-unprofiled]
1 warning generated.
warning: no profile data available for file "_testmultiphase.c" [-Wprofile-instr-unprofiled]
warning: no profile data available for file "_testclinic.c" [-Wprofile-instr-unprofiled]
warning: no profile data available for file "_ctypes_test.c" [-Wprofile-instr-unprofiled]
warning: no profile data available for file "xxlimited_35.c" [-Wprofile-instr-unprofiled]
warning: no profile data available for file "xxlimited.c" [-Wprofile-instr-unprofiled]
1 warning generated.
1 warning generated.
1 warning generated.
1 warning generated.
1 warning generated.
warning: profile data may be out of date: of 3 functions, 2 have mismatched data that will be ignored [-Wprofile-instr-out-of-date]
1 warning generated.
make: *** [Makefile:2015: buildbottest] Error 3 |
Sorry, something went wrong.
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