Emit a warning when a coroutine test function is encountered #4830
Conversation
nicoddemus
force-pushed
the
warn-on-coroutine
branch
from
abf01da
to
112b4a2
Compare
Codecov Report
@@ Coverage Diff @@
## features #4830 +/- ##
============================================
+ Coverage 95.82% 95.85% +0.02%
============================================
Files 113 113
Lines 25213 25229 +16
Branches 2490 2491 +1
============================================
+ Hits 24161 24182 +21
+ Misses 736 734 -2
+ Partials 316 313 -3
Continue to review full report at Codecov.
|
nicoddemus
force-pushed
the
warn-on-coroutine
branch
2 times, most recently
from
3e61e24
to
2e340e7
Compare
|
Ready for review. |
src/_pytest/python.py
Outdated
| @@ -156,6 +156,15 @@ def pytest_configure(config): | |||
| @hookimpl(trylast=True) | |||
| def pytest_pyfunc_call(pyfuncitem): | |||
| testfunction = pyfuncitem.obj | |||
| iscoroutinefunction = getattr(inspect, "iscoroutinefunction", None) | |||
| if iscoroutinefunction is not None and iscoroutinefunction(testfunction): | |||
| msg = "test function {} is a coroutine and not natively supported.\n" | |||
There was a problem hiding this comment.
shouldn't this be a format table warning as constant in the warnigns module
There was a problem hiding this comment.
We don't do that for PytestWarnings in general.
IIRC the objective of concentrating PytestDeprecationWarnings in deprecated.py is to have all of those type of warnings in a single place as a reminder of what we need to remove when the time for a major release comes around.
We don't have the same need for PytestWarning's, because we don't intend to ever remove them, so I don't think it is worth concentrating them in a single place.
Let me know what you think.
There was a problem hiding this comment.
We should address this in a separate PR if we want to move this forward, as it will affect other warnings as well. 👍
testing/acceptance_test.py
Outdated
| result.stdout.fnmatch_lines( | ||
| [ | ||
| "*test function test_async.py::test_1 is a coroutine*", | ||
| "*1 passed, 1 warnings in*", |
There was a problem hiding this comment.
Should it be counted as "passed" after all?
There was a problem hiding this comment.
Hmmm you are right.
I suppose the ideal behavior would be to not collect that test at all, but that's not possible, after all the async plugins need that collected.
Should we skip or xfail it, perhaps?
There was a problem hiding this comment.
Should we skip or xfail it, perhaps?
Skipping sounds good to me - but there should probably only be a single skip message (grouped) with -ra - might be the case automatically, when skipping from the same location here, but should have a test.
There was a problem hiding this comment.
When we skip, should we still show the warning? The warning is a lot more verbose, which doesn't fit with "reason" which is usually a single line...
There was a problem hiding this comment.
IMHO yes. The skip message could state to look at the warning for more details.
Also, having a subclass of PytestWarning for this might make sense, for easier ignoring?!
There was a problem hiding this comment.
Done, here's the full output of the test:
======================================== warnings summary =========================================
test_async.py::test_1
test_async.py::test_2
C:\pytest\src\_pytest\python.py:166: PytestWarning: Coroutine functions are not natively supported and have been skipped.
You need to install a suitable plugin for your async framework, for example:
- pytest-asyncio
- pytest-trio
- pytest-tornasync
warnings.warn(PytestWarning(msg.format(pyfuncitem.nodeid)))
-- Docs: https://docs.pytest.org/en/latest/warnings.html
============================== 2 skipped, 2 warnings in 0.02 seconds ==============================
Also, having a subclass of PytestWarning for this might make sense, for easier ignoring?!
Indeed, but let's wait to see if there's user requests for that. If we go down that route, it might make sense to make a subclass for each warning emitted by pytest.
nicoddemus
force-pushed
the
warn-on-coroutine
branch
from
2e340e7
to
92ae13d
Compare
nicoddemus
force-pushed
the
warn-on-coroutine
branch
from
92ae13d
to
40072b9
Compare
Fix #2224