diff --git a/HACKING.rst b/HACKING.rst index 8ad1e13..f6cecca 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -304,6 +304,11 @@ exception possible should be used. with self.assertRaises(exception.InstanceNotFound): db.instance_get_by_uuid(elevated, instance_uuid) +- [H203] Unit test assertions tend to give better messages for more specific + assertions. As a result, ``assertIsNone(...)`` is preferred over + ``assertEqual(None, ...)`` and ``assertIs(None, ...)``, and + ``assertIsNotNone(...)`` is preferred over ``assertNotEqual(None, ...)`` + and ``assertIsNot(None, ...)``. Off by default. OpenStack Trademark ------------------- diff --git a/hacking/checks/except_checks.py b/hacking/checks/except_checks.py index 6207184..595644a 100644 --- a/hacking/checks/except_checks.py +++ b/hacking/checks/except_checks.py @@ -12,8 +12,11 @@ # module cannot be called except since that is a reserved word +import ast import re +from six import PY2 + from hacking import core RE_ASSERT_RAISES_EXCEPTION = re.compile(r"self\.assertRaises\(Exception[,\)]") @@ -55,3 +58,74 @@ def hacking_except_format_assert(logical_line, noqa): return if RE_ASSERT_RAISES_EXCEPTION.search(logical_line): yield 1, "H202: assertRaises Exception too broad" + + +def is_none(node): + '''Check whether an AST node corresponds to None. + + In Python 2 None uses the same ast.Name class that variables etc. use, + but in Python 3 there is a new ast.NameConstant class. + ''' + if PY2: + return isinstance(node, ast.Name) and node.id == 'None' + return isinstance(node, ast.NameConstant) and node.value is None + + +class NoneArgChecker(ast.NodeVisitor): + '''NodeVisitor to check function calls for None arguments. + + :param func_name: only check calls to functions with this name + :param num_args: number of arguments to check for None + + self.none_found will be True if any None arguments were found. + ''' + def __init__(self, func_name, num_args=2): + self.func_name = func_name + self.num_args = num_args + self.none_found = False + + def visit_Call(self, node): + if isinstance(node.func, ast.Attribute): + local_func_name = node.func.attr + elif isinstance(node.func, ast.Name): + local_func_name = node.func.id + else: # ast.Subscript, etc. -- ignore + local_func_name = None + + if local_func_name == self.func_name: + args_to_check = node.args[:self.num_args] + self.none_found |= any(is_none(x) for x in args_to_check) + self.generic_visit(node) + + +@core.flake8ext +@core.off_by_default +def hacking_assert_is_none(logical_line, noqa): + """Use assertIs(Not)None to check for None in assertions. + + Okay: self.assertEqual('foo', 'bar') + Okay: self.assertNotEqual('foo', {}.get('bar', None)) + Okay: self.assertIs('foo', 'bar') + Okay: self.assertIsNot('foo', 'bar', None) + Okay: foo(self.assertIsNot('foo', 'bar')) + H203: self.assertEqual(None, 'foo') + H203: self.assertNotEqual('foo', None) + H203: self.assertIs(None, 'foo', 'bar') + H203: self.assertIsNot('foo', None, 'bar') + H203: foo(self.assertIsNot('foo', None, 'bar')) + Okay: self.assertEqual(None, 'foo') # noqa + Okay: self.assertIs(None, 'foo') # noqa + Okay: self.assertIsNone('foo') + """ + if noqa: + return + for func_name in ('assertEqual', 'assertIs', 'assertNotEqual', + 'assertIsNot'): + try: + start = logical_line.index('.%s(' % func_name) + 1 + except ValueError: + continue + checker = NoneArgChecker(func_name) + checker.visit(ast.parse(logical_line)) + if checker.none_found: + yield start, "H203: Use assertIs(Not)None to check for None" diff --git a/hacking/tests/test_doctest.py b/hacking/tests/test_doctest.py index a5cd530..76e5e09 100644 --- a/hacking/tests/test_doctest.py +++ b/hacking/tests/test_doctest.py @@ -39,7 +39,7 @@ class HackingTestCase(hacking.tests.TestCase): def test_pep8(self): # NOTE(jecarey): Add tests marked as off_by_default to enable testing - turn_on = set(['H106', 'H904']) + turn_on = set(['H106', 'H203', 'H904']) if self.options.select: turn_on.update(self.options.select) self.options.select = tuple(turn_on) diff --git a/setup.cfg b/setup.cfg index 9e73e9c..26c4789 100644 --- a/setup.cfg +++ b/setup.cfg @@ -37,6 +37,7 @@ flake8.extension = H106 = hacking.checks.vim_check:no_vim_headers H201 = hacking.checks.except_checks:hacking_except_format H202 = hacking.checks.except_checks:hacking_except_format_assert + H203 = hacking.checks.except_checks:hacking_assert_is_none H231 = hacking.checks.python23:hacking_python3x_except_compatible H232 = hacking.checks.python23:hacking_python3x_octal_literals H233 = hacking.checks.python23:hacking_python3x_print_function