TST: Add unmatched warning messages to assert_produces_warning() error #42181
Conversation
There was a problem hiding this comment.
This is off to a good start!
I'm not a fan of logic in tests - I think test_fail_to_match is rather complicated and that the parametrisation doesn't add much, could you split it up into three distinct tests so that you don't have to substitute category.__name__ in the unmatched error message?
| def test_fail_to_match(category, match): | ||
| msg1 = "This is not a match." | ||
| msg2 = "Another unmatched warning." | ||
| unmatched = rf"{category.__name__}\('{msg1}'\), {category.__name__}\('{msg2}'\)" |
There was a problem hiding this comment.
can you make three distinct tests here, and match the full unmatched error message each time?
There was a problem hiding this comment.
Thank you for the feedback! Yes, I can do that. I think I see the reasoning for the original three test cases that I could preserve.
simonjayhawkins
added
Error Reporting
|
Hello @calvinsomething! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-06-24 13:13:28 UTC |
|
Nice! Looks like there's going to be a pre-commit failure though, did you run it locally? See https://pandas.pydata.org/docs/development/contributing.html#pre-commit Can the test names be slightly more descriptive, e.g. test_fail_to_match_future_warning for the first and likewise for the others? Other than that, looks good to me - thanks for doing this, it'll be useful to anyone testing warnings! |
|
Thanks! No I didn't run pre-commit this time. I will make the changes and make the checks before adding the next commit. |
if you do |
|
Hopefully that looks better! |
|
Sorry. Ran pre-commit manually before, but now I have it installed and have corrected the flake-8 error. Everything should be perfect... hopefully. |
| category = RuntimeWarning | ||
| match = "Did not see this warning" | ||
| unmatched = ( | ||
| r"Did not see warning 'RuntimeWarning' matching 'Did not see this warning'. " |
There was a problem hiding this comment.
Would it be more readable if you split the message in lines this way:
r"Did not see warning 'RuntimeWarning' matching 'Did not see this warning'. "
r"The emitted warning messages are "
r"\[RuntimeWarning\('This is not a match.'\), "
r"RuntimeWarning\('Another unmatched warning.'\)\]"
There was a problem hiding this comment.
Yeah, I think that is better. I'll do that.
There was a problem hiding this comment.
Great, not it looks much cleaner here and in other places as well!
|
Made the last suggested change and now all the warning messages are more conformed. |
|
|
I didn't add anything to the whatsnew file, as it seems it's not always necessary depending on the change?
Error message for a failed assert_produces_warning(match=". . .") now looks like e.g.:
Thanks for any feedback if I've done something wrong here.