-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
gh-75988: Fix issues with autospec ignoring wrapped object #115223
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
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
1 similar comment
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
@infohash - your description is a bit terse. Is I'm probably not going to have a chance to look at this any time soon, @mariocj89 / @tirkarthi do you folks have any time? |
|
Hi! Thanks for looking into this.
While this argument works as intended to be, it is not respected by See Details for full explanation. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Thanks for catching the behavior change with my patch. PR looks good to me. Please add a NEWS entry for CI. At some point it I guess it would be good to have precedence of return_value, wraps and side_effect documented when all or combinations of them are set. I would recommend waiting for comments from @cjw296 and @mariocj89 since it's been sometime when I last worked on mock module and your explanation was of good help. Thanks. |
|
Sorry, @infohash and @cjw296, I could not cleanly backport this to |
|
Sorry, @infohash and @cjw296, I could not cleanly backport this to |
|
|
This should be backported, but I don't have time to manually drive the cherry-picker :'( |
|
@cjw296 I will do it using cherry picker. I thought backport was an automated process. |
|
It is, but as you can see in the comments above, it failed this time due to conflicts. |
…hon#115223) * set default return value of functional types as _mock_return_value * added test of wrapping child attributes * added backward compatibility with explicit return * added docs on the order of precedence * added test to check default return_value (cherry picked from commit 735fc2c)
|
GH-117119 is a backport of this pull request to the 3.12 branch. |
…hon#115223) * set default return value of functional types as _mock_return_value * added test of wrapping child attributes * added backward compatibility with explicit return * added docs on the order of precedence * added test to check default return_value (cherry picked from commit 735fc2c)
|
GH-117124 is a backport of this pull request to the 3.11 branch. |
…-115223) (#117119) gh-75988: Fix issues with autospec ignoring wrapped object (#115223) * set default return value of functional types as _mock_return_value * added test of wrapping child attributes * added backward compatibility with explicit return * added docs on the order of precedence * added test to check default return_value (cherry picked from commit 735fc2c)
…-115223) (#117124) gh-75988: Fix issues with autospec ignoring wrapped object (#115223) * set default return value of functional types as _mock_return_value * added test of wrapping child attributes * added backward compatibility with explicit return * added docs on the order of precedence * added test to check default return_value (cherry picked from commit 735fc2c)
…hon#115223) * set default return value of functional types as _mock_return_value * added test of wrapping child attributes * added backward compatibility with explicit return * added docs on the order of precedence * added test to check default return_value
…hon#115223) * set default return value of functional types as _mock_return_value * added test of wrapping child attributes * added backward compatibility with explicit return * added docs on the order of precedence * added test to check default return_value
Fix for:
Description
wrapsis an argument defined inunittest.mock.Mock. Here's how official documentation defines it:While this argument works as intended, it is not respected by
create_autospec. This is the signature ofcreate_autospec:cpython/Lib/unittest/mock.py
Lines 2704 to 2705 in b104360
kwargstakeswrapsas a keyword argument.Tests
Below are the tests that are currently failing because of the issue and will pass after merging the fix.
Details
There are 3 issues that are making
autospecto ignorewraps.Issue 1
When autospec is used on a function/method, autospec replaces the original function/method with its own hardcoded mock function and mock attributes. This can be seen here 👇 and it is intended by design.
cpython/Lib/unittest/mock.py
Lines 2775 to 2781 in 17689e3
This if condition becomes true when you pass a function or a method to
specargument. This is whatFunctionTypesis 👇:cpython/Lib/unittest/mock.py
Lines 2886 to 2891 in 17689e3
On condition true,
_set_signatureor_setup_async_mockwill return a hardcoded mock function 👇:cpython/Lib/unittest/mock.py
Lines 201 to 207 in 17689e3
Now the question is how this leads to ignoring
wraps? See the above line 206 👆_setup_func, it is patching the return value of the mock function 👇:cpython/Lib/unittest/mock.py
Line 264 in 17689e3
mock.return_valueis a property 👇:cpython/Lib/unittest/mock.py
Lines 592 to 593 in fdb2d90
So, accessing its value triggers the
getterdescriptor 👇:cpython/Lib/unittest/mock.py
Lines 571 to 582 in fdb2d90
At line 580 👆, it reassigns a new value to
mock.return_valuewhich triggers thesetterdescriptor 👇:cpython/Lib/unittest/mock.py
Lines 584 to 590 in 17689e3
At line 588 👆
self._mock_return_valueis assigned a value replacing the default valuesentinel.DEFAULT.self._mock_return_valuewill come to the picture soon.Now the original function has been replaced with the mock and its return value is patched, when it is called, these are the calls made 👇:
cpython/Lib/unittest/mock.py
Lines 1159 to 1164 in 17689e3
In the above 👆 return statement
self._mock_call(*args, **kwargs)executes 👇:cpython/Lib/unittest/mock.py
Lines 1167 to 1168 in 17689e3
Inside
self._execute_mock_call(*args, **kwargs)👇:cpython/Lib/unittest/mock.py
Lines 1234 to 1240 in 17689e3
self._mock_return_valueis no longersentinel.DEFAULT. Because of that, the line 1234 👆 becomes true and you are returned mocked return value ignoring yourwrapswhich is just below this condition.Solution
Instead of this:
cpython/Lib/unittest/mock.py
Line 264 in 5a173ef
the default return value should be
mock._mock_return_valueIssue 2
cpython/Lib/unittest/mock.py
Lines 2788 to 2790 in 17689e3
If
wrapsis provided by the user, it should be passed here so that the instance of the mock can also wrap those child attributes that are also the attribute on the parent (like methods).Solution
Issue 3
Inside this for loop:
cpython/Lib/unittest/mock.py
Line 2792 in 17689e3
cpython/Lib/unittest/mock.py
Line 2811 in 17689e3
Child attributes are not being wrapped by the mock.
Solution
It should be like this:
Tests for Issue 2 & Issue 3
Without patch
With patch
This fix ensures that
create_autospecmust respect this argument when creating mock objects.