bpo-42532: Check if NonCallableMock's spec_arg is not None instead of call its __bool__ function#23613
Uh oh!
There was an error while loading. Please reload this page.
Conversation
the-knights-who-say-ni commented Dec 2, 2020
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 resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Mario Corchero <mariocj89@gmail.com>
idanw206 commented Dec 4, 2020
Thanks! :) |
cjw296 left a comment
cjw296 left a comment There was a problem hiding this comment.
Nice spot, but please can you add a test that fails before your fix and passes after it.
bedevere-bot commented Dec 5, 2020
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! |
idanw206force-pushed the bpo-42532-dont-call-bool-on-noncallablemock-specarg branch from a4c2be1 to 6ac0e20Compareidanw206 commented Dec 5, 2020
I have made the requested changes; please review again. |
bedevere-bot commented Dec 5, 2020
Thanks for making the requested changes! @cjw296: please review the changes made to this pull request. |
idanw206force-pushed the bpo-42532-dont-call-bool-on-noncallablemock-specarg branch from 6ac0e20 to 71c4f0cCompareidanw206force-pushed the bpo-42532-dont-call-bool-on-noncallablemock-specarg branch from 3ad46ec to 190c9edCompare
miss-islington commented Dec 6, 2020
miss-islington commented Dec 6, 2020
miss-islington commented Dec 6, 2020
Sorry, @idanw206 and @cjw296, I could not cleanly backport this to |
miss-islington commented Dec 6, 2020
Sorry @idanw206 and @cjw296, I had trouble checking out the |
cjw296 commented Dec 6, 2020
@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? |
tirkarthi commented Dec 6, 2020
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 |
cjw296 commented Dec 6, 2020
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 |
cjw296 commented Dec 7, 2020
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... |
tirkarthi commented Dec 7, 2020
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. |
cjw296 commented Dec 10, 2020
@idanw206 - this has been released in the backport now: https://pypi.org/project/mock/4.0.3/ |
bedevere-bot commented Dec 14, 2020
|
7 participants
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.