From b19774031e68eaeca3cbac68bfbb7fa22107ed4b Mon Sep 17 00:00:00 2001 From: Duncan Hill Date: Tue, 30 Mar 2021 10:41:47 +0100 Subject: [PATCH 1/7] 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. --- bugbear.py | 35 ++++++++++++++++++++++++++++++++++- tests/b017.py | 20 ++++++++++++++++++++ tests/test_bugbear.py | 8 ++++++++ 3 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 tests/b017.py diff --git a/bugbear.py b/bugbear.py index d39ff01..474ba09 100644 --- a/bugbear.py +++ b/bugbear.py @@ -12,7 +12,7 @@ import attr import pycodestyle -__version__ = "21.3.2" +__version__ = "21.3.3" LOG = logging.getLogger("flake8.bugbear") @@ -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)) From e7a02e4978474aec69c33fa36eb2a3c7ef57685d Mon Sep 17 00:00:00 2001 From: Duncan Hill Date: Wed, 31 Mar 2021 09:34:36 +0100 Subject: [PATCH 2/7] Amend documentation, revert version change --- README.rst | 12 ++++++++++++ bugbear.py | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) 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 474ba09..1d9ee9e 100644 --- a/bugbear.py +++ b/bugbear.py @@ -12,7 +12,7 @@ import attr import pycodestyle -__version__ = "21.3.3" +__version__ = "21.3.2" LOG = logging.getLogger("flake8.bugbear") From 7b2ca96398e2a0fe2ce90f0c48c2b326e9766ce4 Mon Sep 17 00:00:00 2001 From: Duncan Hill Date: Thu, 1 Apr 2021 21:47:52 +0100 Subject: [PATCH 3/7] Bugfix: Add more conditionals to B017 checks --- bugbear.py | 6 +++--- tests/b017.py | 3 +-- tests/test_bugbear.py | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/bugbear.py b/bugbear.py index 429ebd6..0b15d78 100644 --- a/bugbear.py +++ b/bugbear.py @@ -10,6 +10,7 @@ from keyword import iskeyword import attr + import pycodestyle __version__ = "21.4.2" @@ -443,6 +444,7 @@ def check_for_b017(self, node): and hasattr(item_context.func, "attr") # noqa W503 and item_context.func.attr == "assertRaises" # noqa W503 and len(item_context.args) == 1 # noqa W503 + and isinstance(item_context.args[0], ast.Name) # noqa W503 and item_context.args[0].id == "Exception" # noqa W503 and not item.optional_vars # noqa W503 ): @@ -468,9 +470,7 @@ def check_for_b901(self, node): for parent, x in self.walk_function_body(node): # Only consider yield when it is part of an Expr statement. - if isinstance(parent, ast.Expr) and isinstance( - x, (ast.Yield, ast.YieldFrom) - ): + if isinstance(parent, ast.Expr) and isinstance(x, (ast.Yield, ast.YieldFrom)): has_yield = True if isinstance(x, ast.Return) and x.value is not None: diff --git a/tests/b017.py b/tests/b017.py index e9b7430..526d430 100644 --- a/tests/b017.py +++ b/tests/b017.py @@ -1,10 +1,9 @@ """ Should emit: -B017 - on lines 10 +B017 - on lines 17 """ import unittest - CONSTANT = True diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index 8bd279f..4062c4d 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -210,7 +210,7 @@ def test_b017(self): filename = Path(__file__).absolute().parent / "b017.py" bbc = BugBearChecker(filename=str(filename)) errors = list(bbc.run()) - expected = self.errors(B017(18, 8)) + expected = self.errors(B017(17, 8)) self.assertEqual(errors, expected) def test_b301_b302_b305(self): From 1c816ad78044d2017afe24503faf537a694d5bdd Mon Sep 17 00:00:00 2001 From: Duncan Hill Date: Thu, 1 Apr 2021 21:47:52 +0100 Subject: [PATCH 4/7] Bugfix: Add more conditionals to B017 checks Attempts to ensure we're only looking at Name type objects in the item_context.args, since apparently this code is still matching other objects that don't have an id attr. --- bugbear.py | 6 +++--- tests/b017.py | 3 +-- tests/test_bugbear.py | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/bugbear.py b/bugbear.py index 429ebd6..0b15d78 100644 --- a/bugbear.py +++ b/bugbear.py @@ -10,6 +10,7 @@ from keyword import iskeyword import attr + import pycodestyle __version__ = "21.4.2" @@ -443,6 +444,7 @@ def check_for_b017(self, node): and hasattr(item_context.func, "attr") # noqa W503 and item_context.func.attr == "assertRaises" # noqa W503 and len(item_context.args) == 1 # noqa W503 + and isinstance(item_context.args[0], ast.Name) # noqa W503 and item_context.args[0].id == "Exception" # noqa W503 and not item.optional_vars # noqa W503 ): @@ -468,9 +470,7 @@ def check_for_b901(self, node): for parent, x in self.walk_function_body(node): # Only consider yield when it is part of an Expr statement. - if isinstance(parent, ast.Expr) and isinstance( - x, (ast.Yield, ast.YieldFrom) - ): + if isinstance(parent, ast.Expr) and isinstance(x, (ast.Yield, ast.YieldFrom)): has_yield = True if isinstance(x, ast.Return) and x.value is not None: diff --git a/tests/b017.py b/tests/b017.py index e9b7430..526d430 100644 --- a/tests/b017.py +++ b/tests/b017.py @@ -1,10 +1,9 @@ """ Should emit: -B017 - on lines 10 +B017 - on lines 17 """ import unittest - CONSTANT = True diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index 8bd279f..4062c4d 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -210,7 +210,7 @@ def test_b017(self): filename = Path(__file__).absolute().parent / "b017.py" bbc = BugBearChecker(filename=str(filename)) errors = list(bbc.run()) - expected = self.errors(B017(18, 8)) + expected = self.errors(B017(17, 8)) self.assertEqual(errors, expected) def test_b301_b302_b305(self): From d5ee400c17a23bcd86971024ae2dd04de8a1c2e0 Mon Sep 17 00:00:00 2001 From: Duncan Hill Date: Thu, 1 Apr 2021 21:57:06 +0100 Subject: [PATCH 5/7] More updates to fix the B017 breakage --- bugbear.py | 2 +- tests/b017.py | 9 ++++++++- tests/test_bugbear.py | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/bugbear.py b/bugbear.py index 0b15d78..f35a646 100644 --- a/bugbear.py +++ b/bugbear.py @@ -13,7 +13,7 @@ import pycodestyle -__version__ = "21.4.2" +__version__ = "21.4.3" LOG = logging.getLogger("flake8.bugbear") diff --git a/tests/b017.py b/tests/b017.py index 526d430..561971e 100644 --- a/tests/b017.py +++ b/tests/b017.py @@ -1,8 +1,9 @@ """ Should emit: -B017 - on lines 17 +B017 - on lines 20 """ import unittest +import asyncio CONSTANT = True @@ -11,6 +12,8 @@ def something_else() -> None: for i in (1, 2, 3): print(i) +class Foo: + pass class Foobar(unittest.TestCase): def evil_raises(self) -> None: @@ -25,3 +28,7 @@ def context_manager_raises(self) -> None: def regex_raises(self) -> None: with self.assertRaisesRegex(Exception, "Regex is good"): raise Exception("Regex is good") + + def raises_with_absolute_reference(self): + with self.assertRaises(asyncio.CancelledError): + Foo() \ No newline at end of file diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index 4062c4d..64bd941 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -210,7 +210,7 @@ def test_b017(self): filename = Path(__file__).absolute().parent / "b017.py" bbc = BugBearChecker(filename=str(filename)) errors = list(bbc.run()) - expected = self.errors(B017(17, 8)) + expected = self.errors(B017(20, 8)) self.assertEqual(errors, expected) def test_b301_b302_b305(self): From 52d0518ddf5d684038fd3c7319d37cea019666d0 Mon Sep 17 00:00:00 2001 From: Duncan Hill Date: Thu, 1 Apr 2021 22:00:43 +0100 Subject: [PATCH 6/7] Black things --- bugbear.py | 4 +++- tests/b017.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/bugbear.py b/bugbear.py index f35a646..b881b8b 100644 --- a/bugbear.py +++ b/bugbear.py @@ -470,7 +470,9 @@ def check_for_b901(self, node): for parent, x in self.walk_function_body(node): # Only consider yield when it is part of an Expr statement. - if isinstance(parent, ast.Expr) and isinstance(x, (ast.Yield, ast.YieldFrom)): + if isinstance(parent, ast.Expr) and isinstance( + x, (ast.Yield, ast.YieldFrom) + ): has_yield = True if isinstance(x, ast.Return) and x.value is not None: diff --git a/tests/b017.py b/tests/b017.py index 561971e..21836a6 100644 --- a/tests/b017.py +++ b/tests/b017.py @@ -12,9 +12,11 @@ def something_else() -> None: for i in (1, 2, 3): print(i) + class Foo: pass + class Foobar(unittest.TestCase): def evil_raises(self) -> None: with self.assertRaises(Exception): @@ -31,4 +33,4 @@ def regex_raises(self) -> None: def raises_with_absolute_reference(self): with self.assertRaises(asyncio.CancelledError): - Foo() \ No newline at end of file + Foo() From 0f298d90e04ba2b1f81a0ddd82194ab9db7899de Mon Sep 17 00:00:00 2001 From: Duncan Hill Date: Thu, 1 Apr 2021 22:01:46 +0100 Subject: [PATCH 7/7] And change the line number again due to black --- tests/test_bugbear.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py index 64bd941..c6e31bb 100644 --- a/tests/test_bugbear.py +++ b/tests/test_bugbear.py @@ -210,7 +210,7 @@ def test_b017(self): filename = Path(__file__).absolute().parent / "b017.py" bbc = BugBearChecker(filename=str(filename)) errors = list(bbc.run()) - expected = self.errors(B017(20, 8)) + expected = self.errors(B017(22, 8)) self.assertEqual(errors, expected) def test_b301_b302_b305(self):