-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-89900: Add option to preserve existing handlers for assertLogs #143970
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
base: main
Are you sure you want to change the base?
gh-89900: Add option to preserve existing handlers for assertLogs #143970
Conversation
…tLogs added documentation for new keep_handlers kwarg for assertLogs function in unittest
|
@vsajip Could you review this PR when you have time? Thanks so much! |
picnixz
left a comment
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 did not say that I was ok with the request so for now, please do not open PRs without explicit approval.
| logger.addHandler(handler) | ||
| else: | ||
| logger.handlers = [handler] | ||
| logger.propagate = False |
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.
Thie sets propagate before the level but it was not case previously.
| @@ -0,0 +1,2 @@ | |||
| Add *keep_handlers* parameter to :meth:`unittest.TestCase.assertLogs` to | |||
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.
This would require more than just a NEWS entry. We would also need a What's New.
| The default is a formatter with format string | ||
| ``"%(levelname)s:%(name)s:%(message)s"`` | ||
|
|
||
| If given, *keep_handlers* should be a boolean value. If ``True``, |
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.
If we were to add an additional parameter I think a keyword-only parameter is better.
| The default is a formatter with format string | ||
| ``"%(levelname)s:%(name)s:%(message)s"`` | ||
|
|
||
| If given, *keep_handlers* should be a boolean value. If ``True``, |
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.
This is overly verbose.Usually if the default is a boolean we assume that the reader is smart enough to infer that the type is a boolean (though at runtime we just use truthiness)
| Added the *keep_handlers* parameter to optionally preserve | ||
| existing handlers. | ||
|
|
||
| .. method:: assertNoLogs(logger=None, level=None) |
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.
For instance we should also allow a keep_handlers keyword here.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Closes #89900
Description
The Problem
The Fix
Added optional keep_handlers param in assertLogs inside
Lib/unittest/case.pyto optionally preserve existing handlers for assertLogs, preventing the situation described above where the same exact test must be run twiceSince keep_handlers is
Falseby default, this change is backwards-compatible.Tests
Added new test cases in
Lib/test/test_unittestto ensure keep_handlers=True preserves existing handlers.Validation
Ran
./python.exe -m test test_unittestand passed all 1093 tests📚 Documentation preview 📚: https://cpython-previews--143970.org.readthedocs.build/