Add optional H203 to check that assertIs(Not)None is used

...instead of assertEqual(None, ...) or assertIs(None, ...) and their
negations.

Apparently that's a thing that people want? At any rate, 30+ projects
have this bug marked "Fix Released". However, without a check to keep
them from regressing, there is a long tail of follow-up patches that
distracts developers and reviewers from addressing user-facing bugs. See
the following Nova changes, spread out over the course of a year, for an
example:

    If3824356ddf4e6e2d91f6bc2fbfa41946d8463cc
    I09f38e219931e0d7ad27f04861d1ebbc3b5e2c5f
    Iee1379b941f93388900e89388676000b845fc8fc
    I0d38a82e78fbe94657ab9a71c08422007843d179
    I9316c0b125aa87b6ebfa996a559c3551093ea711
    I406ea23b0e78f45f16306813e4111a95716cd6b0
    Ic8cb1192e001409d827c8da55fe536681895944b

Add a check for it in the interest of not wasting reviewer time or
having more code churn than necessary.

Change-Id: Iad65cb6399f4f933cbd9f503c88ce144387d39b5
Related-Bug: #1280522

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

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"

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)

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