Add a hacking check for test method closures

A recurring pattern when using multiple mocks is to create a closure
decorated with mocks like:

    def test_thing(self):
            @mock.patch.object(self.compute, 'foo')
            @mock.patch.object(self.compute, 'bar')
            def _do_test(mock_bar, mock_foo):
                # Test things
        _do_test()

However it is easy to leave off the _do_test() and have the test pass
because nothing runs. This check looks for that pattern.

Co-Authored-By: Andrew Laski <andrew@lascii.com>
Change-Id: I4c2395a01470acc7c9e5bcf1d3578d00270a2c07

HACKING.rst

@@ -59,6 +59,7 @@ Nova Specific Commandments
 - [N346] Python 3: do not use dict.itervalues.
 - [N347] Provide enough help text for config options
 - [N348] Deprecated library function os.popen()
+- [N349] Check for closures in tests which are not used
 
 Creating Unit Tests
 -------------------

nova/hacking/checks.py

@@ -14,6 +14,7 @@
 # under the License.
 
 import ast
+import os
 import re
 
 import pep8
@@ -469,6 +470,77 @@ class CheckForTransAdd(BaseASTChecker):
         super(CheckForTransAdd, self).generic_visit(node)
 
 
+class _FindVariableReferences(ast.NodeVisitor):
+    def __init__(self):
+        super(_FindVariableReferences, self).__init__()
+        self._references = []
+
+    def visit_Name(self, node):
+        if isinstance(node.ctx, ast.Load):
+            # This means the value of a variable was loaded. For example a
+            # variable 'foo' was used like:
+            # mocked_thing.bar = foo
+            # foo()
+            # self.assertRaises(excepion, foo)
+            self._references.append(node.id)
+        super(_FindVariableReferences, self).generic_visit(node)
+
+
+class CheckForUncalledTestClosure(BaseASTChecker):
+    """Look for closures that are never called in tests.
+
+    A recurring pattern when using multiple mocks is to create a closure
+    decorated with mocks like:
+
+    def test_thing(self):
+            @mock.patch.object(self.compute, 'foo')
+            @mock.patch.object(self.compute, 'bar')
+            def _do_test(mock_bar, mock_foo):
+                # Test things
+        _do_test()
+
+    However it is easy to leave off the _do_test() and have the test pass
+    because nothing runs. This check looks for methods defined within a test
+    method and ensures that there is a reference to them. Only methods defined
+    one level deep are checked. Something like:
+
+    def test_thing(self):
+        class FakeThing:
+            def foo(self):
+
+    would not ensure that foo is referenced.
+
+    N349
+    """
+
+    def __init__(self, tree, filename):
+        super(CheckForUncalledTestClosure, self).__init__(tree, filename)
+        self._filename = filename
+
+    def visit_FunctionDef(self, node):
+        # self._filename is 'stdin' in the unit test for this check.
+        if (not os.path.basename(self._filename).startswith('test_') and
+            not 'stdin'):
+            return
+
+        closures = []
+        references = []
+        # Walk just the direct nodes of the test method
+        for child_node in ast.iter_child_nodes(node):
+            if isinstance(child_node, ast.FunctionDef):
+                closures.append(child_node.name)
+
+        # Walk all nodes to find references
+        find_references = _FindVariableReferences()
+        find_references.generic_visit(node)
+        references = find_references._references
+
+        missed = set(closures) - set(references)
+        if missed:
+            self.add_error(node, 'N349: Test closures not called: %s'
+                    % ','.join(missed))
+
+
 def assert_true_or_false_with_in(logical_line):
     """Check for assertTrue/False(A in B), assertTrue/False(A not in B),
     assertTrue/False(A in B, message) or assertTrue/False(A not in B, message)
@@ -732,3 +804,4 @@ def factory(register):
     register(check_python3_no_itervalues)
     register(cfg_help_with_enough_text)
     register(no_os_popen)
+    register(CheckForUncalledTestClosure)

nova/tests/unit/test_hacking.py

@@ -750,3 +750,30 @@ class HackingTestCase(test.NoDBTestCase):
         errors = [(4, 0, 'N348'), (8, 8, 'N348')]
         self._assert_has_errors(code, checks.no_os_popen,
                                 expected_errors=errors)
+
+    def test_uncalled_closures(self):
+
+        checker = checks.CheckForUncalledTestClosure
+        code = """
+               def test_fake_thing():
+                   def _test():
+                       pass
+               """
+        self._assert_has_errors(code, checker,
+                expected_errors=[(1, 0, 'N349')])
+
+        code = """
+               def test_fake_thing():
+                   def _test():
+                       pass
+                   _test()
+               """
+        self._assert_has_no_errors(code, checker)
+
+        code = """
+               def test_fake_thing():
+                   def _test():
+                       pass
+                   self.assertRaises(FakeExcepion, _test)
+               """
+        self._assert_has_no_errors(code, checker)