Add B908: Rule to check that assertRaises-like contexts has no more than 1 top-level statements (#382)

* Add B908: Rule to check that assertRaises-like contexts has no more than 1 top-level statement

* Update README with B908

* Catch "with raises" and "with warns"

* Update README

* Fix typing

* Add "assertWarnsRegexp" to B908_unittest_methods

* Update README.rst

Co-authored-by: Cooper Lees <[email protected]>

* Remove assertWarnsRegexp

* Remove old comment

---------

Co-authored-by: Alexey Nikitin <[email protected]>
Co-authored-by: Cooper Lees <[email protected]>

Author: 3 people

README.rst

@@ -228,6 +228,8 @@ This is meant to be enabled by developers writing visitors using the ``ast`` mod
 
 **B907**: Consider replacing ``f"'{foo}'"`` with ``f"{foo!r}"`` which is both easier to read and will escape quotes inside ``foo`` if that would appear. The check tries to filter out any format specs that are invalid together with ``!r``. If you're using other conversion flags then e.g. ``f"'{foo!a}'"`` can be replaced with ``f"{ascii(foo)!r}"``. Not currently implemented for python<3.8 or ``str.format()`` calls.
 
+**B908**: Contexts with exceptions assertions like ``with self.assertRaises`` or ``with pytest.raises`` should not have multiple top-level statements. Each statement should be in its own context. That way, the test ensures that the exception is raised only in the exact statement where you expect it.
+
 **B950**: Line too long. This is a pragmatic equivalent of
 ``pycodestyle``'s ``E501``: it considers "max-line-length" but only triggers
 when the value has been exceeded by **more than 10%**. ``noqa`` and ``type: ignore`` comments are ignored. You will no

bugbear.py

@@ -28,6 +28,14 @@
     ast.GeneratorExp,
 )
 FUNCTION_NODES = (ast.AsyncFunctionDef, ast.FunctionDef, ast.Lambda)
+B908_pytest_functions = {"raises", "warns"}
+B908_unittest_methods = {
+    "assertRaises",
+    "assertRaisesRegex",
+    "assertRaisesRegexp",
+    "assertWarns",
+    "assertWarnsRegex",
+}
 
 Context = namedtuple("Context", ["node", "stack"])
 
@@ -504,6 +512,7 @@ def visit_Raise(self, node):
     def visit_With(self, node):
         self.check_for_b017(node)
         self.check_for_b022(node)
+        self.check_for_b908(node)
         self.generic_visit(node)
 
     def visit_JoinedStr(self, node):
@@ -1104,6 +1113,39 @@ def check_for_b022(self, node):
         ):
             self.errors.append(B022(node.lineno, node.col_offset))
 
+    @staticmethod
+    def _is_assertRaises_like(node: ast.withitem) -> bool:
+        if not (
+            isinstance(node, ast.withitem)
+            and isinstance(node.context_expr, ast.Call)
+            and isinstance(node.context_expr.func, (ast.Attribute, ast.Name))
+        ):
+            return False
+        if isinstance(node.context_expr.func, ast.Name):
+            # "with raises"
+            return node.context_expr.func.id in B908_pytest_functions
+        elif isinstance(node.context_expr.func, ast.Attribute) and isinstance(
+            node.context_expr.func.value, ast.Name
+        ):
+            return (
+                # "with pytest.raises"
+                node.context_expr.func.value.id == "pytest"
+                and node.context_expr.func.attr in B908_pytest_functions
+            ) or (
+                # "with self.assertRaises"
+                node.context_expr.func.value.id == "self"
+                and node.context_expr.func.attr in B908_unittest_methods
+            )
+        else:
+            return False
+
+    def check_for_b908(self, node: ast.With):
+        if len(node.body) < 2:
+            return
+        for node_item in node.items:
+            if self._is_assertRaises_like(node_item):
+                self.errors.append(B908(node.lineno, node.col_offset))
+
     def check_for_b025(self, node):
         seen = []
         for handler in node.handlers:
@@ -1759,7 +1801,12 @@ def visit_Lambda(self, node):
         " flag."
     )
 )
-
+B908 = Error(
+    message=(
+        "B908 assertRaises-type context should not contains more than one top-level"
+        " statement."
+    )
+)
 B950 = Error(message="B950 line too long ({} > {} characters)")
 
-disabled_by_default = ["B901", "B902", "B903", "B904", "B905", "B906", "B950"]
+disabled_by_default = ["B901", "B902", "B903", "B904", "B905", "B906", "B908", "B950"]

tests/b908.py

@@ -0,0 +1,57 @@
+import unittest
+import warnings
+
+import pytest
+from pytest import raises, warns
+
+with pytest.raises(TypeError):
+    a = 1 + "x"
+    b = "x" + 1
+print(a, b)
+
+
+class SomeTestCase(unittest.TestCase):
+    def test_func_raises(self):
+        with self.assertRaises(TypeError):
+            a = 1 + "x"
+            b = "x" + 1
+        print(a, b)
+
+    def test_func_raises_regex(self):
+        with self.assertRaisesRegex(TypeError):
+            a = 1 + "x"
+            b = "x" + 1
+        print(a, b)
+
+    def test_func_raises_regexp(self):
+        with self.assertRaisesRegexp(TypeError):
+            a = 1 + "x"
+            b = "x" + 1
+        print(a, b)
+
+    def test_raises_correct(self):
+        with self.assertRaises(TypeError):
+            print("1" + 1)
+
+
+with raises(Exception):
+    "1" + 1
+    "2" + 2
+
+with pytest.warns(Warning):
+    print("print before warning")
+    warnings.warn("some warning", stacklevel=1)
+
+with warns(Warning):
+    print("print before warning")
+    warnings.warn("some warning", stacklevel=1)
+
+# should not raise an error
+with pytest.raises(TypeError):
+    print("1" + 1)
+
+with pytest.warns(Warning):
+    warnings.warn("some warning", stacklevel=1)
+
+with raises(Exception):
+    raise Exception("some exception")

tests/test_bugbear.py

@@ -51,6 +51,7 @@
     B905,
     B906,
     B907,
+    B908,
     B950,
     BugBearChecker,
     BugBearVisitor,
@@ -489,6 +490,21 @@ def test_b032(self):
         )
         self.assertEqual(errors, expected)
 
+    def test_b908(self):
+        filename = Path(__file__).absolute().parent / "b908.py"
+        bbc = BugBearChecker(filename=str(filename))
+        errors = list(bbc.run())
+        expected = self.errors(
+            B908(7, 0),
+            B908(15, 8),
+            B908(21, 8),
+            B908(27, 8),
+            B908(37, 0),
+            B908(41, 0),
+            B908(45, 0),
+        )
+        self.assertEqual(errors, expected)
+
     @unittest.skipIf(sys.version_info < (3, 8), "not implemented for <3.8")
     def test_b907(self):
         filename = Path(__file__).absolute().parent / "b907.py"