diff --git a/README.rst b/README.rst index 058a3d6..75ac35f 100644 --- a/README.rst +++ b/README.rst @@ -123,6 +123,13 @@ waste CPU instructions. Either prepend ``assert`` or remove it. **B016**: Cannot raise a literal. Did you intend to return it or raise an Exception? +**B017**: ``self.assertRaises(Exception):`` should be considered evil. It can lead +to your test passing even if the code being tested is never executed due to a typo. +Either assert for a more specific exception (builtin or custom), use +``assertRaisesRegex``, or use the context manager form of assertRaises +(``with self.assertRaises(Exception) as ex:``) with an assertion against the +data available in ``ex``. + Python 3 compatibility warnings ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -249,6 +256,11 @@ MIT Change Log ---------- +21.4.1 +~~~~~~ + +* Add B017: check for gotta-catch-em-all assertRaises(Exception) + 21.3.2 ~~~~~~ diff --git a/bugbear.py b/bugbear.py index d39ff01..1d9ee9e 100644 --- a/bugbear.py +++ b/bugbear.py @@ -317,6 +317,10 @@ def visit_Raise(self, node): self.check_for_b016(node) self.generic_visit(node) + def visit_With(self, node): + self.check_for_b017(node) + self.generic_visit(node) + def compose_call_path(self, node): if isinstance(node, ast.Attribute): yield from self.compose_call_path(node.value) @@ -423,6 +427,26 @@ def check_for_b016(self, node): if isinstance(node.exc, (ast.NameConstant, ast.Num, ast.Str)): self.errors.append(B016(node.lineno, node.col_offset)) + def check_for_b017(self, node): + """Checks for use of the evil syntax 'with assertRaises(Exception):' + + This form of assertRaises will catch everything that subclasses + Exception, which happens to be the vast majority of Python internal + errors, including the ones raised when a non-existing method/function + is called, or a function is called with an invalid dictionary key + lookup. + """ + item = node.items[0] + item_context = item.context_expr + if ( + hasattr(item_context.func, "attr") + and item_context.func.attr == "assertRaises" # noqa W503 + and len(item_context.args) == 1 # noqa W503 + and item_context.args[0].id == "Exception" # noqa W503 + and not item.optional_vars # noqa W503 + ): + self.errors.append(B017(node.lineno, node.col_offset)) + def walk_function_body(self, node): def _loop(parent, node): if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)): @@ -727,6 +751,15 @@ def visit(self, node): "an Exception?" ) ) +B017 = Error( + message=( + "B017 assertRaises(Exception): should be considered evil. " + "It can lead to your test passing even if the code being tested is " + "never executed due to a typo. Either assert for a more specific " + "exception (builtin or custom), use assertRaisesRegex, or use the " + "context manager form of assertRaises." + ) +) # Those could be false positives but it's more dangerous to let them slip # through if they're not. diff --git a/tests/b017.py b/tests/b017.py new file mode 100644 index 0000000..13a4c06 --- /dev/null +++ b/tests/b017.py @@ -0,0 +1,20 @@ +""" +Should emit: +B017 - on lines 10 +""" +import unittest + + +class Foobar(unittest.TestCase): + def evil_raises(self) -> None: + with self.assertRaises(Exception): + raise Exception("Evil I say!") + + def context_manager_raises(self) -> None: + with self.assertRaises(Exception) as ex: + raise Exception("Context manager is good") + self.assertEqual("Context manager is good", str(ex.exception)) + + def regex_raises(self) -> None: + with self.assertRaisesRegex(Exception, "Regex is good"): + raise Exception("Regex is good") diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index c755fb9..a797a33 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -27,6 +27,7 @@ B014, B015, B016, + B017, B301, B302, B303, @@ -205,6 +206,13 @@ def test_b016(self): expected = self.errors(B016(6, 0), B016(7, 0), B016(8, 0)) self.assertEqual(errors, expected) + def test_b017(self): + filename = Path(__file__).absolute().parent / "b017.py" + bbc = BugBearChecker(filename=str(filename)) + errors = list(bbc.run()) + expected = self.errors(B017(10, 8)) + self.assertEqual(errors, expected) + def test_b301_b302_b305(self): filename = Path(__file__).absolute().parent / "b301_b302_b305.py" bbc = BugBearChecker(filename=str(filename))