Repository navigation
gh-143231: Do not swallow not matched warnings in assertWarns*() - #149229
serhiy-storchaka merged 7 commits into
Conversation
Sorry, something went wrong.
unittest.TestCase methods assertWarns() and assertWarnsRegex() no longer swallow warnings that do not match the specified category or regex. Nested context managers are now supported.
Documentation build overview41 files changed · ± 41 modified ± Modified |
Sorry, something went wrong.
bitdancer
left a comment
There was a problem hiding this comment.
Has the behavior of record=True changed at all?
Sorry, something went wrong.
| self.assertWarnsRegex(UserWarning, 'foo', self.assertMessagesCM, | ||
| 'assertWarnsRegex', (UserWarning, 'regex'), |
There was a problem hiding this comment.
When I read this I thought at first that you were calling assertWarnsRegex directly instead of calling assertMessagesCM. Even after understanding what you are actually doing it is hard to understand exactly what this is testing. If I understand the difference correctly (now it emits the warning where before it didn't, but that's not what the test is testing), then I think it would be easier to understand if you wrote it like this:
| self.assertWarnsRegex(UserWarning, 'foo', self.assertMessagesCM, | |
| 'assertWarnsRegex', (UserWarning, 'regex'), | |
| with self.assertWarnsRegex(UserWarning, 'foo'): | |
| self.assertMessagesCM('assertWarnsRegex', (UserWarning, 'regex'), |
Sorry, something went wrong.
There was a problem hiding this comment.
This test is for the callable interface of assertWarnsRegex(). The next test is for the context manager interface. I think it is better to not mix them in these tests.
Sorry, something went wrong.
There was a problem hiding this comment.
Right, I understand that. But that part of the test is in the inner call to assertMessagesCM, which is the thing calling assertWarnsRexex multiple times using the callable interface. The fact that a warning that is not caught is also emitted is a consequence of the nature of the test, but that's a byproduct, and by using the assertWarns context manager we indicate that we are expecting that byproduct.
If you want to leave it the way you have it I won't block on it ;)
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, I missed this with other tests in test_case where I use nested callables. Here, I used the callable form only because it was the minimal change. But a context manager looks more readable.
Sorry, something went wrong.
| exc_name = str(self.expected) | ||
| first_matching = None | ||
| matched = False | ||
| not_matching_warnings = [] |
There was a problem hiding this comment.
Standard English would make this variable name non_matching_warnings.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again. |
Sorry, something went wrong.
bitdancer
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
…ing by module name. Add the module attribute to warnings.WarningMessage.
|
I fixed most of the remaining issues with minimal changes in the warnings module. User-visible changes are that warning filtering with actions "default" and "module" now works as expected, and filtering by module name now works. It required more complex changes in unittest. I am planning to move them to the warnings module later, but this requires serious rewriting of the C code. |
Sorry, something went wrong.
|
Changes to the warnings module were merged in a separate PR #149298, with additional tests. |
Sorry, something went wrong.
unittest.TestCase methods assertWarns() and assertWarnsRegex() no longer swallow warnings that do not match the specified category or regex. Nested context managers are now supported.