diff --git a/HACKING.rst b/HACKING.rst index a3c3a814b..5e8ec136c 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -6,8 +6,11 @@ Read the OpenStack Style Commandments https://docs.openstack.org/developer/hacki Mistral Specific Commandments ----------------------------- +- [M001] Use LOG.warning(). LOG.warn() is deprecated. - [M318] Change assertEqual(A, None) or assertEqual(None, A) by optimal assert like assertIsNone(A) +- [M319] Enforce use of assertTrue/assertFalse +- [M320] Enforce use of assertIs/assertIsNot - [M327] Do not use xrange(). xrange() is not compatible with Python 3. Use range() or six.moves.range() instead. - [M328] Python 3: do not use dict.iteritems. diff --git a/mistral/hacking/checks.py b/mistral/hacking/checks.py index 2f48bb95b..e1ea14aca 100644 --- a/mistral/hacking/checks.py +++ b/mistral/hacking/checks.py @@ -49,6 +49,34 @@ def assert_equal_none(logical_line): yield (0, msg) +def no_assert_equal_true_false(logical_line): + """Check for assertTrue/assertFalse sentences + + M319 + """ + _start_re = re.compile(r'assert(Not)?Equal\((True|False),') + _end_re = re.compile(r'assert(Not)?Equal\(.*,\s+(True|False)\)$') + + if _start_re.search(logical_line) or _end_re.search(logical_line): + yield (0, "M319: assertEqual(A, True|False), " + "assertEqual(True|False, A), assertNotEqual(A, True|False), " + "or assertEqual(True|False, A) sentences must not be used. " + "Use assertTrue(A) or assertFalse(A) instead") + + +def no_assert_true_false_is_not(logical_line): + """Check for assertIs/assertIsNot sentences + + M320 + """ + _re = re.compile(r'assert(True|False)\(.+\s+is\s+(not\s+)?.+\)$') + + if _re.search(logical_line): + yield (0, "M320: assertTrue(A is|is not B) or " + "assertFalse(A is|is not B) sentences must not be used. " + "Use assertIs(A, B) or assertIsNot(A, B) instead") + + def check_oslo_namespace_imports(logical_line): if re.match(oslo_namespace_imports_from_dot, logical_line): msg = ("O323: '%s' must be used instead of '%s'.") % ( @@ -255,6 +283,8 @@ class CheckForLoggingIssues(BaseASTChecker): def factory(register): register(assert_equal_none) + register(no_assert_equal_true_false) + register(no_assert_true_false_is_not) register(check_oslo_namespace_imports) register(CheckForLoggingIssues) register(check_python3_no_iteritems) diff --git a/mistral/tests/unit/hacking/test_checks.py b/mistral/tests/unit/hacking/test_checks.py index 4ecffa013..ea7ddbc0c 100644 --- a/mistral/tests/unit/hacking/test_checks.py +++ b/mistral/tests/unit/hacking/test_checks.py @@ -38,27 +38,27 @@ class BaseLoggingCheckTest(base.BaseTest): # installed. @mock.patch('pep8._checks', {'physical_line': {}, 'logical_line': {}, 'tree': {}}) - def run_check(self, code): - pep8.register_check(self.get_checker()) + def run_check(self, code, checker, filename=None): + pep8.register_check(checker) lines = textwrap.dedent(code).strip().splitlines(True) - checker = pep8.Checker(lines=lines) - checker.check_all() + checker = pep8.Checker(filename=filename, lines=lines) + with mock.patch('pep8.StandardReport.get_file_results'): + checker.check_all() checker.report._deferred_print.sort() return checker.report._deferred_print - def assert_has_errors(self, code, expected_errors=None): + def _assert_has_errors(self, code, checker, expected_errors=None, + filename=None): # Pull out the parts of the error that we'll match against. - actual_errors = (e[:3] for e in self.run_check(code)) - - # Adjust line numbers to make the fixture data more readable. - import_lines = len(self.code_ex.shared_imports.split('\n')) - 1 - actual_errors = [(e[0] - import_lines, e[1], e[2]) - for e in actual_errors] - + actual_errors = [e[:3] for e in + self.run_check(code, checker, filename)] self.assertEqual(expected_errors or [], actual_errors) + def _assert_has_no_errors(self, code, checker, filename=None): + self._assert_has_errors(code, checker, filename=filename) + def test_assert_equal_none(self): self.assertEqual(len(list(checks.assert_equal_none( "self.assertEqual(A, None)"))), 1) @@ -69,6 +69,40 @@ class BaseLoggingCheckTest(base.BaseTest): self.assertEqual( len(list(checks.assert_equal_none("self.assertIsNone()"))), 0) + def test_no_assert_equal_true_false(self): + code = """ + self.assertEqual(context_is_admin, True) + self.assertEqual(context_is_admin, False) + self.assertEqual(True, context_is_admin) + self.assertEqual(False, context_is_admin) + self.assertNotEqual(context_is_admin, True) + self.assertNotEqual(context_is_admin, False) + self.assertNotEqual(True, context_is_admin) + self.assertNotEqual(False, context_is_admin) + """ + errors = [(1, 0, 'M319'), (2, 0, 'M319'), (3, 0, 'M319'), + (4, 0, 'M319'), (5, 0, 'M319'), (6, 0, 'M319'), + (7, 0, 'M319'), (8, 0, 'M319')] + self._assert_has_errors(code, checks.no_assert_equal_true_false, + expected_errors=errors) + code = """ + self.assertEqual(context_is_admin, stuff) + self.assertNotEqual(context_is_admin, stuff) + """ + self._assert_has_no_errors(code, checks.no_assert_equal_true_false) + + def test_no_assert_true_false_is_not(self): + code = """ + self.assertTrue(test is None) + self.assertTrue(False is my_variable) + self.assertFalse(None is test) + self.assertFalse(my_variable is False) + """ + errors = [(1, 0, 'M320'), (2, 0, 'M320'), (3, 0, 'M320'), + (4, 0, 'M320')] + self._assert_has_errors(code, checks.no_assert_true_false_is_not, + expected_errors=errors) + def test_check_python3_xrange(self): func = checks.check_python3_xrange self.assertEqual(1, len(list(func('for i in xrange(10)')))) @@ -105,4 +139,5 @@ class TestLoggingWithWarn(BaseLoggingCheckTest): code = self.code_ex.shared_imports + data['code'] errors = data['expected_errors'] - self.assert_has_errors(code, expected_errors=errors) + self._assert_has_errors(code, checks.CheckForLoggingIssues, + expected_errors=errors) diff --git a/mistral/tests/unit/mstrlfixtures/hacking.py b/mistral/tests/unit/mstrlfixtures/hacking.py index e7a3e59df..77790fc90 100644 --- a/mistral/tests/unit/mstrlfixtures/hacking.py +++ b/mistral/tests/unit/mstrlfixtures/hacking.py @@ -35,6 +35,6 @@ class HackingLogging(fixtures.Fixture): LOG.warn('text') """, 'expected_errors': [ - (4, 9, 'M001'), + (8, 9, 'M001'), ], }