Skip to content

Commit 9d18e5f

Browse files
cricalixcricalix
authored
Add B017 - detection for an evil form of assertRaises (#163)
* Add B017 - detection for a bad form of assertRaises ```with assertRaises(Exception):``` is basically a "catch 'em all" assert that casts far too broad of a net when it comes to detecting failures in code being tested. Assertions should be testing specific failure cases, not "did Python throw /any/ type of error?", and so the context manager form, or the assertRaisesRegex form are far better to use. * Amend documentation, revert version change
1 parent e82bb8d commit 9d18e5f

4 files changed

Lines changed: 73 additions & 0 deletions

File tree

‎README.rst‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,13 @@ waste CPU instructions. Either prepend ``assert`` or remove it.
123123
**B016**: Cannot raise a literal. Did you intend to return it or raise
124124
an Exception?
125125

126+
**B017**: ``self.assertRaises(Exception):`` should be considered evil. It can lead
127+
to your test passing even if the code being tested is never executed due to a typo.
128+
Either assert for a more specific exception (builtin or custom), use
129+
``assertRaisesRegex``, or use the context manager form of assertRaises
130+
(``with self.assertRaises(Exception) as ex:``) with an assertion against the
131+
data available in ``ex``.
132+
126133

127134
Python 3 compatibility warnings
128135
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -249,6 +256,11 @@ MIT
249256
Change Log
250257
----------
251258

259+
21.4.1
260+
~~~~~~
261+
262+
* Add B017: check for gotta-catch-em-all assertRaises(Exception)
263+
252264
21.3.2
253265
~~~~~~
254266

‎bugbear.py‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,10 @@ def visit_Raise(self, node):
317317
self.check_for_b016(node)
318318
self.generic_visit(node)
319319

320+
def visit_With(self, node):
321+
self.check_for_b017(node)
322+
self.generic_visit(node)
323+
320324
def compose_call_path(self, node):
321325
if isinstance(node, ast.Attribute):
322326
yield from self.compose_call_path(node.value)
@@ -423,6 +427,26 @@ def check_for_b016(self, node):
423427
if isinstance(node.exc, (ast.NameConstant, ast.Num, ast.Str)):
424428
self.errors.append(B016(node.lineno, node.col_offset))
425429

430+
def check_for_b017(self, node):
431+
"""Checks for use of the evil syntax 'with assertRaises(Exception):'
432+
433+
This form of assertRaises will catch everything that subclasses
434+
Exception, which happens to be the vast majority of Python internal
435+
errors, including the ones raised when a non-existing method/function
436+
is called, or a function is called with an invalid dictionary key
437+
lookup.
438+
"""
439+
item = node.items[0]
440+
item_context = item.context_expr
441+
if (
442+
hasattr(item_context.func, "attr")
443+
and item_context.func.attr == "assertRaises" # noqa W503
444+
and len(item_context.args) == 1 # noqa W503
445+
and item_context.args[0].id == "Exception" # noqa W503
446+
and not item.optional_vars # noqa W503
447+
):
448+
self.errors.append(B017(node.lineno, node.col_offset))
449+
426450
def walk_function_body(self, node):
427451
def _loop(parent, node):
428452
if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)):
@@ -727,6 +751,15 @@ def visit(self, node):
727751
"an Exception?"
728752
)
729753
)
754+
B017 = Error(
755+
message=(
756+
"B017 assertRaises(Exception): should be considered evil. "
757+
"It can lead to your test passing even if the code being tested is "
758+
"never executed due to a typo. Either assert for a more specific "
759+
"exception (builtin or custom), use assertRaisesRegex, or use the "
760+
"context manager form of assertRaises."
761+
)
762+
)
730763

731764
# Those could be false positives but it's more dangerous to let them slip
732765
# through if they're not.

‎tests/b017.py‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
"""
2+
Should emit:
3+
B017 - on lines 10
4+
"""
5+
import unittest
6+
7+
8+
class Foobar(unittest.TestCase):
9+
def evil_raises(self) -> None:
10+
with self.assertRaises(Exception):
11+
raise Exception("Evil I say!")
12+
13+
def context_manager_raises(self) -> None:
14+
with self.assertRaises(Exception) as ex:
15+
raise Exception("Context manager is good")
16+
self.assertEqual("Context manager is good", str(ex.exception))
17+
18+
def regex_raises(self) -> None:
19+
with self.assertRaisesRegex(Exception, "Regex is good"):
20+
raise Exception("Regex is good")

‎tests/test_bugbear.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
B014,
2828
B015,
2929
B016,
30+
B017,
3031
B301,
3132
B302,
3233
B303,
@@ -205,6 +206,13 @@ def test_b016(self):
205206
expected = self.errors(B016(6, 0), B016(7, 0), B016(8, 0))
206207
self.assertEqual(errors, expected)
207208

209+
def test_b017(self):
210+
filename = Path(__file__).absolute().parent / "b017.py"
211+
bbc = BugBearChecker(filename=str(filename))
212+
errors = list(bbc.run())
213+
expected = self.errors(B017(10, 8))
214+
self.assertEqual(errors, expected)
215+
208216
def test_b301_b302_b305(self):
209217
filename = Path(__file__).absolute().parent / "b301_b302_b305.py"
210218
bbc = BugBearChecker(filename=str(filename))

0 commit comments

Comments
 (0)