-
Notifications
You must be signed in to change notification settings - Fork 82
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
Raise all warnings in _catch_warnings
#2765
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2765 +/- ##
=======================================
+ Coverage 99.8% 99.8% +0.1%
=======================================
Files 301 301
Lines 27889 27904 +15
=======================================
+ Hits 27826 27841 +15
Misses 63 63
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.
looks good to me!
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.
Great catch! I left a few nits and a few questions, plus a suggestion to improve and fix tests. Looks good otherwise!
@@ -5102,6 +5102,25 @@ def test_pipeline_parameter_warnings_only_parameternotused( | |||
assert str(warning) == "Big bad warning" | |||
|
|||
|
|||
def test_pipeline_parameter_warnings_with_other_types(): |
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.
Is there test coverage on if we get multiple warnings with the same message? ie coverage for
elif str(msg.message) not in raised_messages:
warnings.warn(msg.message)
raised_messages.append(str(msg.message))
?
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, this test covers that! The TargetLeakage
warning actually throws the same warning for each generated pipeline, just like the ParameterNotUsed
warnings come up. This test is actually how I discovered we needed to filter duplicate warnings.
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.
LGTM once all tests pass! Thanks for clarifying!
Closes #2761