Use standard warnings for internal pytest warnings#3931
Use standard warnings for internal pytest warnings#3931nicoddemus merged 44 commits intopytest-dev:featuresfrom
Conversation
…nmanager It seems this has no effect since `pluggy` was developed as a separate library.
pytest_logwarning is no longer emitted by the warnings plugin, only ever emitted from .warn() functions in config and item
Once we can capture warnings during the config stage, we can then get rid of this function Related to pytest-dev#2891
Standard warnings already contain the proper location, so we don't need to also print the node id
nicoddemus
changed the title
asottile
left a comment
asottile
left a comment
There was a problem hiding this comment.
code looks good -- how does this interact with capwarn?
src/_pytest/cacheprovider.py
Outdated
|
|
||
| def warn(self, fmt, **args): | ||
| self._warn(code="I9", message=fmt.format(**args) if args else fmt) | ||
| import warnings |
There was a problem hiding this comment.
lets please make the imports toplevel and introduce a PytestCacheWarning which expresses the warnings of this subsystem
There was a problem hiding this comment.
@nicoddemus i beleive as a followup we should inline this method and adapt stacklevels, so people see the warnings located in their code
There was a problem hiding this comment.
lets please make the imports toplevel and introduce a PytestCacheWarning which expresses the warnings of this subsystem
This is worth discussing a little: should we in general create a new warning class for each plugin, or just using PytestWarning is enough?
My take is that just having a few warning classes is enough, I don't see much reason to have a ton of warning classes unless we have a good use case for that.
I ask because there's only 2 calls to Config.warn:
self.warn("could not create cache path {path}", path=path)
...
self.warn("cache could not write path {path}", path=path)Is Config.warn something that we meant to be public so others use it? It feels like it was made public without much thought behind it, mostly seems like an accident to me actually.
There was a problem hiding this comment.
@nicoddemus this self.warn can be completely replaced by a warnings.warn
i beleive we can remove it as internal api, if there turns out to be a user, we can bring it back
|
|
||
| * ``"config"``: during pytest configuration/initialization stage. | ||
| * ``"collect"``: during test collection. | ||
| * ``"runtest"``: during test execution. |
There was a problem hiding this comment.
for better tracking i propose having not just runtest, but also the item phases called setup, call, teardown
There was a problem hiding this comment.
Oops just discovered that trying to catch warnings in pytest_runtest_setup, pytest_runtest_call and pytest_runtest_teardown cause problems with recwarn fixture. I don't see a way to fix this properly.
Any suggestions here @RonnyPfannschmidt ?
There was a problem hiding this comment.
i believe we need to find a way to connect the fixture there - i propose postponing the detail work on that as i believe it will be tricky , require a tracking data-structure and a set of extra support utilities
There was a problem hiding this comment.
OK, I'm reverting this to "runtest" then.
If we implement support for setup, call and teardown in the future, we will have a breaking change in our hands because of runtest...
There was a problem hiding this comment.
actually, can we fake it by just implementing it like runtest, but switching the internal string based on phase?
There was a problem hiding this comment.
But warnings are cached by warnings.catch_warnings, so it seems out of our control.
Feel free to try out an idea and pushing to my branch if you have a clear picture on how to implement it. 👍
src/_pytest/warnings.py
Outdated
| if item is not None: | ||
| for mark in item.iter_markers(name="filterwarnings"): | ||
| for arg in mark.args: | ||
| warnings._setoption(arg) |
There was a problem hiding this comment.
btw - its a "bug" that we dont use our own setoption here, as it prevents regex usage
…ning_captured hook
|
Finally all builds have passed. I believe I have addressed all comments, so this is ready for a final review. 👍 |
RonnyPfannschmidt
left a comment
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
tremendous and fabulous work, this shaped up nicely over time
Codecov Report
@@ Coverage Diff @@
## features #3931 +/- ##
============================================
+ Coverage 96.24% 96.25% +0.01%
============================================
Files 108 107 -1
Lines 23419 23568 +149
============================================
+ Hits 22539 22686 +147
- Misses 880 882 +2
Continue to review full report at Codecov.
|
|
@nicoddemus @blueyed it seems quite painful that the baseline now takes more time that a full test originally |
? |
pytest.mark.filterwarnings?
#3785
6 participants
This PR replaces our usage of the internal warnings system to use standard warnings instead.
Some comments:
pytest_warning_capturehook.Config.warnandNode.warnare now deprecated.pytest_logwarninghook to keep displaying warnings issued byConfig.warnandNode.warn. When we removeConfig.warnandNode.warnwe are free to markpytest_logwarningfor deprecation.Fix #2452
Fix #2908
Fix #3251