-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Bugfix/warningschecker twice #4620
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
Bugfix/warningschecker twice #4620
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4620 +/- ##
==========================================
- Coverage 95.75% 95.75% -0.01%
==========================================
Files 111 111
Lines 24678 24683 +5
Branches 2446 2446
==========================================
+ Hits 23630 23634 +4
+ Misses 740 739 -1
- Partials 308 310 +2
Continue to review full report at Codecov.
|
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.
approving based on the discussion we had in the issue
Thank you! |
@@ -683,3 +683,27 @@ def test_false_function_no_warn(self, testdir): | |||
self.create_file(testdir, False) | |||
result = testdir.runpytest() | |||
result.stdout.fnmatch_lines(["*1 failed in*"]) | |||
|
|||
|
|||
def test_warningschecker_twice(testdir): |
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.
I would go for a simple unittest here:
def test_warningschecker_twice():
expectation = pytest.warns(UserWarning)
with expectation:
warnings.warn("Message A", UserWarning)
with expectation:
warnings.warn("Message B", UserWarning)
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.
@nicoddemus
Makes sense, sorry for merging it too early.
Will you create a followup?
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.
Yep, way simpler! But now it should be another PR?
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.
Yep, we can't change this one anymore. Can you do it @Sup3rGeo? If you don't have the time, I will get to it later. 👍
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.
Done: #4622
Fixes #4617
changelog
folder, with a name like<ISSUE NUMBER>.<TYPE>.rst
. See changelog/README.rst for details.master
branch for bug fixes, documentation updates and trivial changes.