Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add B017 - detection for an evil form of assertRaises #163

Merged
merged 2 commits into from Mar 31, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions README.rst
Expand Up @@ -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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand Down Expand Up @@ -249,6 +256,11 @@ MIT
Change Log
----------

21.4.1
~~~~~~

* Add B017: check for gotta-catch-em-all assertRaises(Exception)

21.3.2
~~~~~~

Expand Down
33 changes: 33 additions & 0 deletions bugbear.py
Expand Up @@ -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)
Expand Down Expand Up @@ -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
cricalix marked this conversation as resolved.
Show resolved Hide resolved
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)):
Expand Down Expand Up @@ -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.
Expand Down
20 changes: 20 additions & 0 deletions 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))
cricalix marked this conversation as resolved.
Show resolved Hide resolved

def regex_raises(self) -> None:
with self.assertRaisesRegex(Exception, "Regex is good"):
raise Exception("Regex is good")
8 changes: 8 additions & 0 deletions tests/test_bugbear.py
Expand Up @@ -27,6 +27,7 @@
B014,
B015,
B016,
B017,
B301,
B302,
B303,
Expand Down Expand Up @@ -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))
Expand Down