diff --git a/HACKING.rst b/HACKING.rst index b5f20ffd19e3..e893d6dc561b 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -75,6 +75,7 @@ Nova Specific Commandments calling is using priviledge escalation. - [N363] Disallow ``(not_a_tuple)`` because you meant ``(a_tuple_of_one,)``. - [N364] Check non-existent mock assertion methods and attributes. +- [N365] Check misuse of assertTrue/assertIsNone. Creating Unit Tests ------------------- diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 3d5c5cf47dc7..51c6f8944e7b 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -125,6 +125,9 @@ mock_assert_method_re = re.compile( r"(asser|asset|asssert|assset)_(called(_(once|with|once_with))?" r"|any_call|has_calls|not_called))\(") mock_attribute_re = re.compile(r"[\.\(](retrun_value)[,=\s]") +# Regex for useless assertions +useless_assertion_re = re.compile( + r"\.((assertIsNone)\(None|(assertTrue)\((True|\d+|'.+'|\".+\")),") class BaseASTChecker(ast.NodeVisitor): @@ -942,6 +945,28 @@ def nonexistent_assertion_methods_and_attributes(logical_line, filename): yield (0, msg % match.group(1)) +def useless_assertion(logical_line, filename): + """Check useless assertions in tests. + + The following assertions are useless. + + - assertIsNone(None, ...) + - assertTrue(True, ...) + - assertTrue(2, ...) # Constant number + - assertTrue('Constant string', ...) + - assertTrue("Constant string", ...) + + They are usually misuses of assertIsNone or assertTrue. + + N365 + """ + msg = "N365: Misuse of %s." + if 'nova/tests/' in filename: + match = useless_assertion_re.search(logical_line) + if match: + yield (0, msg % (match.group(2) or match.group(3))) + + def factory(register): register(import_no_db_in_virt) register(no_db_session_in_public_api) @@ -989,3 +1014,4 @@ def factory(register): register(privsep_imports_not_aliased) register(did_you_mean_tuple) register(nonexistent_assertion_methods_and_attributes) + register(useless_assertion) diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index ab570f4d310c..e942b3a07390 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -991,3 +991,31 @@ class HackingTestCase(test.NoDBTestCase): self._assert_has_no_errors( code, checks.nonexistent_assertion_methods_and_attributes, filename="nova/tests/unit/test_context.py") + + def test_useless_assertion(self): + code = """ + self.assertIsNone(None, status) + self.assertTrue(True, flag) + self.assertTrue(10, count) + self.assertTrue('active', status) + self.assertTrue("building", status) + """ + errors = [(x + 1, 0, 'N365') for x in range(5)] + # Check errors in 'nova/tests' directory. + self._assert_has_errors( + code, checks.useless_assertion, + expected_errors=errors, filename="nova/tests/unit/test_context.py") + # Check no errors in other than 'nova/tests' directory. + self._assert_has_no_errors( + code, checks.nonexistent_assertion_methods_and_attributes, + filename="nova/compute/api.py") + code = """ + self.assertIsNone(None_test_var, "Fails") + self.assertTrue(True_test_var, 'Fails') + self.assertTrue(var2, "Fails") + self.assertTrue(test_class.is_active('active'), 'Fails') + self.assertTrue(check_status("building"), 'Fails') + """ + self._assert_has_no_errors( + code, checks.useless_assertion, + filename="nova/tests/unit/test_context.py")