bpo-42532: Check if NonCallableMock's spec_arg is not None instead of call its __bool__ function #23613
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
Misc/NEWS.d/next/Library/2020-12-02-07-37-59.bpo-42532.ObNep_.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Mario Corchero <mariocj89@gmail.com>
Thanks! :) |
|
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 And if you don't make the requested changes, you will be poked with soft cushions! |
idanw206
force-pushed
the
bpo-42532-dont-call-bool-on-noncallablemock-specarg
branch
from
a4c2be1 to
6ac0e20
Compare
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @cjw296: please review the changes made to this pull request. |
idanw206
force-pushed
the
bpo-42532-dont-call-bool-on-noncallablemock-specarg
branch
from
6ac0e20 to
71c4f0c
Compare
idanw206
force-pushed
the
bpo-42532-dont-call-bool-on-noncallablemock-specarg
branch
from
3ad46ec to
190c9ed
Compare
|
Sorry, @idanw206 and @cjw296, I could not cleanly backport this to |
|
Sorry @idanw206 and @cjw296, I had trouble checking out the |
|
@tirkarthi - I've taken off the backport labels since neither of the ones I added appear to have worked. Where should this get backported and how do we do that nowadays? |
|
There is a tool called cherry_picker that needs to be installed and can be run locally to cherry pick the commits. It seems bot cannot do it automatically due to conflicts. I will raise manual backports. Dev guide : https://devguide.python.org/committing/?highlight=Backport#backporting-changes-to-an-older-version |
|
I guess I'm concerned that there are conflicts, it doesn't feel like there should be? |
|
Yes, it might affect mock backport. The change is simple enough so not sure if conflict is in mock.py or the tests. I will update here once I try the backport to 3.9 and 3.8. |
|
@cjw296 I get below patch as conflict while trying to cherry pick. I guess 3.8 doesn't need this since it seems to iterate through all args and check for spec which has been simplified in 3.9 and above with #16099. The simplification has a backport to 3.8 in #16137 but was not applied. Hence backporting this PR would need #16137 also to be applied. diff --cc Lib/unittest/mock.py
index 3629cf6109,4db1bacf4b..0000000000
--- a/Lib/unittest/mock.py
+++ b/Lib/unittest/mock.py
@@@ -403,18 -402,12 +403,25 @@@ class NonCallableMock(Base)
# so we can create magic methods on the
# class without stomping on other mocks
bases = (cls,)
- if not issubclass(cls, AsyncMockMixin):
+ if not issubclass(cls, AsyncMock):
# Check if spec is an async object or function
++<<<<<<< HEAD
+ sig = inspect.signature(NonCallableMock.__init__)
+ bound_args = sig.bind_partial(cls, *args, **kw).arguments
+ spec_arg = [
+ arg for arg in bound_args.keys()
+ if arg.startswith('spec')
+ ]
+ if spec_arg:
+ # what if spec_set is different than spec?
+ if _is_async_obj(bound_args[spec_arg[0]]):
+ bases = (AsyncMockMixin, cls,)
++=======
+ bound_args = _MOCK_SIG.bind_partial(cls, *args, **kw).arguments
+ spec_arg = bound_args.get('spec_set', bound_args.get('spec'))
+ if spec_arg is not None and _is_async_obj(spec_arg):
+ bases = (AsyncMockMixin, cls)
++>>>>>>> c598a04dd2... [bpo-42532](https://bugs.python.org/issue42532): Check if NonCallableMock's spec_arg is not None instead of call its __bool__ function (GH23613)
new = type(cls.__name__, bases, {'__doc__': cls.__doc__})
instance = _safe_super(NonCallableMock, cls).__new__(new)
return instance |
|
It's unclear to me why #16099 doesn't have any tests as part of the commit. That would help us figure out whether both or neither of these PRs should end up in 3.8... |
|
My guess is that it's a refactoring PR where all bound_args are iterated and checking for starting with "spec" might give others starting with spec like "specfoo" which could have been neither supported/tested. The PR probably tries to be explicit about checking for only spec_set and spec. |
|
@idanw206 - this has been released in the backport now: https://pypi.org/project/mock/4.0.3/ |
|
https://bugs.python.org/issue42532
If an object has logic in its
__bool__function, it'll be called here, but it shouldn't be called.