gh-81432: Make create_autospec generate AsyncMocks for awaitable classes #25347
Conversation
|
@tirkarthi / @lisroach - thoughts? |
@lindacorbett - not sure this is ready to merge, and it's a tricky PR related to a problem that cropped up elsewhere. I don't have time to look until later this week, but I'd like someone with a lot of familiarity with the mock module to think this through...
|
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 |
|
Alright this is in a functional state. I'm not too familiar with AsyncMock but I think these changes achieve the intent of the original bug+test. @lisroach PTAL |
|
I must admit the change initially looks a little complicated to review since we had several discussion over how to detect async/coroutine properly, when to return AsyncMock, etc. and just want to understand if there is any behavior change though tests pass. I will review this PR a little later as I am working on few other things. Thanks @msuozzo . |
| @@ -24,14 +24,15 @@ | |||
|
|
|||
|
|
|||
| import asyncio | |||
| import collections | |||
There was a problem hiding this comment.
Please use import collections.abc as it is more explicit.
| def test_create_autospec_awaitable_class(self): | ||
| self.assertIsInstance(create_autospec(AwaitableClass), AsyncMock) |
There was a problem hiding this comment.
If I understand the changes correctly this will still fail and return MagicMock
create_autospec(AwaitableClass) will return MagicMock but instantiating returned mock will return an AsyncMock. If so can we modify this to be a test self.assertIsInstance(create_autospec(AwaitableClass), MagicMock) ?
| @@ -811,12 +828,10 @@ def test_assert_awaited_but_not_called(self): | |||
| self.mock.assert_awaited() | |||
| with self.assertRaises(AssertionError): | |||
| self.mock.assert_called() | |||
| with self.assertRaises(TypeError): | |||
| # You cannot await an AsyncMock, it must be a coroutine | |||
There was a problem hiding this comment.
As per this comment I guess not implementing __await__ was a conscious decision. This PR adds a behavior change making AsyncMock awaitable. I will leave this to @lisroach since I forgot the reason why this was decision was made. Perhaps I would suggest opening a new issue to track this and revert changes that make AsyncMock awaitable. For reference this comment was added in ef04851 and discussion at bpo-38136 .
|
This PR is stale because it has been open for 30 days with no activity. |
github-actions
bot
removed
the
stale
|
@lisroach - ping? |
gpshead
changed the title
Remove skipped test. See discussion on python/cpython#25326. Fix is apparently here, but no-one is confident to review and land: python/cpython#25347. Backports: 984894a9a25c0f8298565b0c0c2e1f41917e4f88 Signed-off-by: Chris Withers <chris@simplistix.co.uk>
Remove skipped test. See discussion on python/cpython#25326. Fix is apparently here, but no-one is confident to review and land: python/cpython#25347. Backports: 984894a9a25c0f8298565b0c0c2e1f41917e4f88 Signed-off-by: Chris Withers <chris@simplistix.co.uk>
#81432