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

Author: cricalix

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
 ~~~~~~
 

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.

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")

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))