-
-
Notifications
You must be signed in to change notification settings - Fork 750
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
Handle logging issues at teardown #3703
Conversation
f0f14b6
to
f94702b
Compare
Confirm effective on #3701 . Logging much more sensible. Issue was due to adding logging handlers in the CLI module which aren't otherwise properly cleaned up. |
test/cli/commands_test.py
Outdated
@@ -29,6 +30,31 @@ | |||
re_ansi_escape = re.compile(r"\x1b[^m]*m") | |||
|
|||
|
|||
@pytest.fixture() |
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.
@pytest.fixture() | |
@pytest.fixture(autouse=True) |
I think if you make this change, it'll automatically apply this fixture to every test in this .py
file without you having to explicitly add it to each test. (This can be handy because it means future tests added to this file will automatically get the same behavior.)
https://docs.pytest.org/en/6.2.x/fixture.html#autouse-fixtures-fixtures-you-don-t-have-to-request
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 tried that and it didn't work 🤔 . I'll have another go - but if not then I'll just merge as is.
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.
No worries. We can always do it later. I've successfully used autouse
fixtures a few times, so there may be something subtle going wrong. They are definitely a bit magical, which is both good and bad. 😬
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! One possible suggestion for how to simplify/futureproof this a bit.
Codecov Report
@@ Coverage Diff @@
## main #3703 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 176 176
Lines 13413 13413
=========================================
Hits 13413 13413 Continue to review full report at Codecov.
|
Resolves #3702.
I'm testing the effects of this on the draft name deprecation PR.