Replace assert False where an exception should always be thrown (B011)
#6815
Conversation
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ±0 15 suites ±0 6h 38m 32s ⏱️ + 11m 14s For more details on these failures and errors, see this check. Results for commit 724ba00. ± Comparison against base commit 9267a21. |
jrbourbeau
left a comment
jrbourbeau
left a comment
There was a problem hiding this comment.
Thanks @hendrikmakait! Just one small question
| bad_thread = bad_threads[0] | ||
| call_stacks = profile.call_stack(sys._current_frames()[bad_thread.ident]) | ||
| assert False, (bad_thread, call_stacks) | ||
| assert False, (bad_thread, call_stacks) # noqa: B011 |
There was a problem hiding this comment.
Why ignore here instead of raising an error?
There was a problem hiding this comment.
Honestly, I don't mind raising an error either. My line of thinking was as follows: From my understanding, the rule is targeted toward production code that should always throw, not tests. Therefore, in #6809, I propose to ignore B011 for all tests since assert statements are perfectly fine in there. (I'm happy to iterate on that.) As check_thread_leak is a helper function for tests, it felt like the same rules applied to it as to all other tests for which we ignore B011.
There was a problem hiding this comment.
Thanks for the additional context. I don't have strong feelings either way. Let's go with what you have here -- it's easy enough to follow-up and remove the ignore if other have different thoughts on the topic
2 participants
Partially addresses pre-commit failures in #6809
pre-commit run --all-files