Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No longer override existing warning filters during warnings capture #2445

Merged
merged 1 commit into from
May 30, 2017

Conversation

nicoddemus
Copy link
Member

In the end it was possible to actually fix the existing behavior. I checked and it no longer breaks SQLAlchemy test suite which was the original post on #2430.

As a downside/side-effect, this no longer automatically captures deprecation warnings because we want to play safe and don't try to mess the existing filters, but I believe we can review this in the future if we like.

Fix #2430

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0008%) to 92.236% when pulling c6774cc on nicoddemus:warnings-remove-filter into 6117930 on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0008%) to 92.236% when pulling db8d83a on nicoddemus:warnings-remove-filter into 6117930 on pytest-dev:master.

@The-Compiler
Copy link
Member

LGTM as-is - and with --pythonwarnings error I still get an exception with DeprecationWarnings like expected.

@nicoddemus
Copy link
Member Author

@The-Compiler would you do the honors of merging it then? 😁

@blueyed
Copy link
Contributor

blueyed commented May 30, 2017

What still seems to be problematic is:

logging.captureWarnings(True)
logging.getLogger('py.warnings').addFilter(filter_deprecation_warnings)

With or without this in setup.cfg:

[tool:pytest]
filterwarnings =
    once::DeprecationWarning
    once::PendingDeprecationWarning

I want to use this method to filter out DeprecationWarnings based on where they are coming from.

@nicoddemus
Copy link
Member Author

@blueyed thanks for the feedback.

Can you provide more info about what you intend with that? I'm not familiar with logging.captureWarnings and I'm short on time at the moment to look it up.

Also, do you feel this should block a new release? And do you have a suggestion on how to implement what you want in regards to logging vs warnings?

(Sorry for leaving this questions here, just that I have to leave for a meeting and will be out for a few hours)

@RonnyPfannschmidt
Copy link
Member

@nicoddemus logging.captureWarnings replaces the warnings.show function with something that redirects to logging

@nicoddemus
Copy link
Member Author

Rebased and updated to add a news fragment. 👍

@nicoddemus
Copy link
Member Author

Thanks @RonnyPfannschmidt.

Hmmm that feels like some additional things are still needed. @blueyed do you feel this should hold the release or is something we can tackle afterwards without changing current behavior? My feeling is the former, but would like to hear your opinion first.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0008%) to 92.236% when pulling f591c6d on nicoddemus:warnings-remove-filter into ee0844d on pytest-dev:master.

@blueyed
Copy link
Contributor

blueyed commented May 30, 2017

I think it should not hold back a release probably, since it fixes more crucial things already.

As for logging.captureWarnings: it monkeypatches warnings.showwarning to log them instead, which then allows for more fine-grained filtering.
pytest could maybe check if logging._warnings_showwarning is None? (which means it is not patched to go through logging)

@nicoddemus
Copy link
Member Author

I think it should not hold back a release probably, since it fixes more crucial things already.

Thanks! Would you mind opening a separate issue for the logging and warning support? We can move this discussion there. 👍

There's an ongoing discussion about this on `#2430
<https://github.com/pytest-dev/pytest/issues/2430>`_.


Starting from version ``3.1``, pytest now automatically catches all warnings during test execution
and displays them at the end of the session::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*except for DeprecationWarning and PendingDeprecationWarning

@@ -0,0 +1,4 @@
pytest warning capture no longer override existing warning filters. The previous
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/override/overrides/ ?

@@ -0,0 +1,4 @@
pytest warning capture no longer override existing warning filters. The previous
behaviour would override all filters and caused regressions in test suites which configure warning
filters to match their needs. Note as a side-effect of this is that ``DeprecationWarning``
Copy link
Contributor

@blueyed blueyed May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Note as/Note that/

@nicoddemus
Copy link
Member Author

Thanks @blueyed, applied all your suggestions. 👍

Unless someone else tells otherwise, I will merge this as soon as CI passes again. Thanks everyone for the reviews.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0008%) to 92.236% when pulling 32e2642 on nicoddemus:warnings-remove-filter into 454426c on pytest-dev:master.

@nicoddemus nicoddemus merged commit 1dee443 into pytest-dev:master May 30, 2017
@nicoddemus nicoddemus deleted the warnings-remove-filter branch May 30, 2017 21:14
@hynek
Copy link
Contributor

hynek commented Jun 1, 2017

I think this might be doing too much? pytest 3.1.1 is silencing ResourceWarnings by default now which is a pity. In any case it seems at odds with the docs?

platform darwin -- Python 3.6.1, pytest-3.1.1, py-1.4.33, pluggy-0.4.0
rootdir: /Users/hynek/Work/epp_proxy, inifile: setup.cfg
plugins: catchlog-1.2.2, asyncio-0.6.0, hypothesis-3.11.1

I hope I’ve reverted to the old behavior by using:

filterwarnings =
    once::Warning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning integration breaks warnings.filterwarnings
6 participants