-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Change RemovedInPytest4Warnings to errors by default #4349
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
Change RemovedInPytest4Warnings to errors by default #4349
Conversation
8dd5ca7
to
748ac78
Compare
Codecov Report
@@ Coverage Diff @@
## features #4349 +/- ##
============================================
+ Coverage 95.84% 95.84% +<.01%
============================================
Files 111 111
Lines 24928 24954 +26
Branches 2438 2440 +2
============================================
+ Hits 23892 23918 +26
Misses 737 737
Partials 299 299
Continue to review full report at Codecov.
|
a498f80
to
d9d9ec9
Compare
To keep existing tests which emit RemovedInPytest4Warnings running, decided to go with a command line option because: * Is harder to integrate an ini option with tests which already use an ini file * It also marks tests which need to be removed/updated in 4.1, when RemovedInPytest4Warning and related functionality are removed. Fix pytest-dev#3737
d9d9ec9
to
dc20ded
Compare
Otherwise the tests will use tox's env cache which makes them flaky
Ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the approach with the extra argument creates a little headache for me but on the flip side its a nice tool for various indications as you noted
practicality beats purity 👍
@@ -41,6 +41,14 @@ def test_success(): | |||
""" | |||
) | |||
|
|||
# customize cache directory so we don't use the tox's cache directory, which makes tests in this module flaky |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we ought to make this more general as i believe there are other bits that can and will affect that
its ok as a followup tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will follow it up on #4378. 👍
To keep existing tests which emit RemovedInPytest4Warnings running, decided
to go with a command line option because:
RemovedInPytest4Warning and related functionality are removed.
Fix #3737