classification
Title: inspect.iscoroutinefunction raises TypeError when checks Mock of function or coroutinefunction
Type: behavior Stage: resolved
Components: asyncio Versions: Python 3.9, Python 3.8
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: asvetlov, lisroach, michael.foord, miyakogi, xtreak, yselivanov
Priority: normal Keywords: patch

Created on 2016-01-17 11:42 by miyakogi, last changed 2019-09-13 14:29 by asvetlov. This issue is now closed.

Files
File name Uploaded Description Edit
mock.patch miyakogi, 2016-01-17 11:42 review
mock2.patch miyakogi, 2016-01-20 03:46 review
Messages (10)
msg258464 - (view) Author: Hiroyuki Takagi (miyakogi) Date: 2016-01-17 11:42
inspect.iscoroutinefunction and asyncio.iscoroutinefunction with patch of issue25599 (https://bugs.python.org/issue25599) raise TypeError when used to check Mock object which mocks function or coroutinefunction.

How to reproduce:

- For the mock of function

>>> from unittest.mock import Mock
>>> import inspect
>>> def a():...
... 
>>> inspect.iscoroutinefunction(Mock(a))

Expected: False
Actual: 
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File ".../cpython/Lib/inspect.py", line 187, in iscoroutinefunction
        object.__code__.co_flags & CO_COROUTINE)
    TypeError: unsupported operand type(s) for &: 'Mock' and 'int'


- For the mock of coroutine-function

>>> async def b():...
... 
>>> inspect.iscoroutinefunction(Mock(b))

Expected: True
Actual:
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File ".../cpython/Lib/inspect.py", line 187, in iscoroutinefunction
        object.__code__.co_flags & CO_COROUTINE)
    TypeError: unsupported operand type(s) for &: 'Mock' and 'int'


Without the patch of issue25599, asyncio.iscoroutinefunction does not raise error and returns Mock object. But I don't think it is expected behavior, as discussed in that issue.

I wrote a patch to solve this problem.
msg258494 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-01-18 03:05
Why fix this in mock rather than in the iscoroutinefunction functions? (Not
a rhetorical question -- I really am not sure what the better place is to
fix this, or if we should even consider fixing it.)
msg258521 - (view) Author: Hiroyuki Takagi (miyakogi) Date: 2016-01-18 12:54
Thank you for review and comment.

Honestly speaking, I couldn't find any other good place to fix it.

One possible solution might be to use isinstance(mock, Mock) in iscoroutinefunction, but I don't think it's good for inspect module to add special check and depend on unittest.mock. Mocks are usually used only in debug/test, but iscoroutinefunction is used in production code. Adding some check to iscoroutinefunction only for test is not good for performance (though, actually its effect will be very small).

The reasons why I think this behavior should be fixed are,

    - Raising error and stopping test is not kind for mock users
    - After the patch (issue25599), no mock object can become `True` to iscoroutinefunction, which will make it impossible to test the block after if-iscoroutinefunction by using mock.


Now, I checked inspect module again, and found one more unexpected behavior (not error).

>>> def a(): yield 1
>>> inspect.isgeneratorfunction(a)
    True

>>> inspect.isgeneratorfunction(Mock(a))
    False

With the patch, inspect.isgeneratorfunction(Mock(a)) returns True.
msg258538 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-01-18 18:54
It looks like this should be fixed in the mock module, as special casing it in inspect doesn't look right.  Unfortunately, I can't review this patch, as I don't know the mock module internals and I don't think I understand all consequences of this patch.

Michael, what do you think?
msg258575 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2016-01-19 09:39
In inspect checking that __code__ is a code object, or that co_flags is an int, would be better than special casing mock. 

However, the patch to mock looks reasonable to me. It copies the whole code object from the original function to the mock object. The patch needs a test (I'd like to see a test for the Mock(func) case and the create_autospec(func) case.)
msg258638 - (view) Author: Hiroyuki Takagi (miyakogi) Date: 2016-01-20 03:46
Thank you for reviewing patch.

I wrote test and updated patch. To pass the test, both this patch and issue25599's patch are required.

Changes of the patch:
- copy __code__ not only functions but also methods
- add autospec (create_autospec) suppoort

I have completely missed about autospec, thank you for a mention about it.
For autospec, simply copying original __code__ to funcopy makes error on existing tests.
That's why I changed the src of exec, but it seems to be quite ad-hoc. It may be better to be improved, but I don't have any good idea, sorry.

On the tests of this patch, I wonder if it's better to use assertIs(.., True/False) instead of assertTrue/False, since it was one of the problem in issue25599.
To apply this change and pass test, need to change asyncio.iscoroutinefunction to return bool. The change would be very easy, just update issue25599's patch like `return_value = bool(getattr(func, ...`.
msg258834 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-01-22 19:28
Am I really still needed on this issue?

On Tue, Jan 19, 2016 at 7:46 PM, Hiroyuki Takagi <report@bugs.python.org>
wrote:

>
> Hiroyuki Takagi added the comment:
>
> Thank you for reviewing patch.
>
> I wrote test and updated patch. To pass the test, both this patch and
> issue25599's patch are required.
>
> Changes of the patch:
> - copy __code__ not only functions but also methods
> - add autospec (create_autospec) suppoort
>
> I have completely missed about autospec, thank you for a mention about it.
> For autospec, simply copying original __code__ to funcopy makes error on
> existing tests.
> That's why I changed the src of exec, but it seems to be quite ad-hoc. It
> may be better to be improved, but I don't have any good idea, sorry.
>
> On the tests of this patch, I wonder if it's better to use assertIs(..,
> True/False) instead of assertTrue/False, since it was one of the problem in
> issue25599.
> To apply this change and pass test, need to change
> asyncio.iscoroutinefunction to return bool. The change would be very easy,
> just update issue25599's patch like `return_value = bool(getattr(func, ...`.
>
> ----------
> Added file: http://bugs.python.org/file41664/mock2.patch
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue26140>
> _______________________________________
>
msg352323 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-09-13 13:29
I believe after implementing AsyncMock we can close the issue
msg352329 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-09-13 13:38
I believe it's still a problem with Mock though MagicMock and AsyncMock work but I am not sure what would be the correct behavior that whether Mock should behave like a coroutine or not as per the discussion here.

./python.exe
Python 3.9.0a0 (heads/master:375a3e2bdb, Sep 13 2019, 14:18:36)
[Clang 7.0.2 (clang-700.1.81)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from unittest.mock import Mock, MagicMock, AsyncMock
>>> import inspect
>>> def f(): pass
...
>>> for m in MagicMock, AsyncMock : print(inspect.iscoroutinefunction(m(f)))
...
True
True
>>> for m in Mock, MagicMock, AsyncMock : print(inspect.iscoroutinefunction(m(f)))
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/inspect.py", line 194, in iscoroutinefunction
    return _has_code_flag(obj, CO_COROUTINE)
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/inspect.py", line 180, in _has_code_flag
    return bool(f.__code__.co_flags & flag)
TypeError: unsupported operand type(s) for &: 'Mock' and 'int'
msg352341 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-09-13 14:29
I believe with AsyncMock implemented we can close the issue
History
Date User Action Args
2019-09-13 14:29:34asvetlovsetmessages: + msg352341
2019-09-13 13:38:49xtreaksetmessages: + msg352329
2019-09-13 13:29:04asvetlovsetstatus: open -> closed

nosy: + asvetlov
messages: + msg352323

resolution: out of date
stage: resolved
2019-06-12 19:32:10vstinnersetnosy: - vstinner
2019-06-12 18:18:55xtreaksetnosy: + lisroach, xtreak

versions: + Python 3.8, Python 3.9, - Python 3.5
2016-01-25 10:31:04michael.foordsetnosy: - gvanrossum
2016-01-22 19:28:59gvanrossumsetmessages: + msg258834
2016-01-20 03:46:14miyakogisetfiles: + mock2.patch

messages: + msg258638
2016-01-19 09:39:28michael.foordsetmessages: + msg258575
2016-01-18 18:54:08yselivanovsetnosy: + michael.foord
messages: + msg258538
2016-01-18 12:54:46miyakogisetmessages: + msg258521
2016-01-18 03:05:31gvanrossumsetmessages: + msg258494
2016-01-17 11:42:52miyakogicreate