diff --git a/HACKING.rst b/HACKING.rst index bef67061ff..eb7e7c2997 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -8,10 +8,10 @@ As such, we tend to follow Neutron conventions regarding coding style. Octavia Specific Commandments ----------------------------- -- [N316] Change assertTrue(isinstance(A, B)) by optimal assert like +- [O316] Change assertTrue(isinstance(A, B)) by optimal assert like assertIsInstance(A, B). -- [N320] Validate that LOG messages, except debug ones, have translations -- [N334] Change assertTrue/False(A in/not in B, message) to the more specific +- [O320] Validate that LOG messages, except debug ones, have translations +- [O334] Change assertTrue/False(A in/not in B, message) to the more specific assertIn/NotIn(A, B, message) Creating Unit Tests diff --git a/octavia/hacking/checks.py b/octavia/hacking/checks.py index 320a6dc480..cb0568c33e 100644 --- a/octavia/hacking/checks.py +++ b/octavia/hacking/checks.py @@ -55,6 +55,18 @@ for level, hint in six.iteritems(_all_log_levels): } log_translation_hints.append(re.compile(r)) +assert_trueinst_re = re.compile( + r"(.)*assertTrue\(isinstance\((\w|\.|\'|\"|\[|\])+, " + "(\w|\.|\'|\"|\[|\])+\)\)") +assert_equal_in_end_with_true_or_false_re = re.compile( + r"assertEqual\((\w|[][.'\"])+ in (\w|[][.'\", ])+, (True|False)\)") +assert_equal_in_start_with_true_or_false_re = re.compile( + r"assertEqual\((True|False), (\w|[][.'\"])+ in (\w|[][.'\", ])+\)") +assert_equal_with_true_re = re.compile( + r"assertEqual\(True,") +assert_equal_with_false_re = re.compile( + r"assertEqual\(False,") + def _directory_to_check_translation(filename): return True @@ -119,7 +131,43 @@ def no_translate_debug_logs(logical_line, filename): """ if _directory_to_check_translation(filename) and logical_line.startswith( "LOG.debug(_("): - yield(0, "O319 Don't translate debug level logs") + yield(0, "O319 Don't translate debug level logs") + + +def assert_true_instance(logical_line): + """Check for assertTrue(isinstance(a, b)) sentences + + O316 + """ + if assert_trueinst_re.match(logical_line): + yield (0, "O316: assertTrue(isinstance(a, b)) sentences not allowed") + + +def assert_equal_true_or_false(logical_line): + """Check for assertEqual(True, A) or assertEqual(False, A) sentences + + O323 + """ + res = (assert_equal_with_true_re.search(logical_line) or + assert_equal_with_false_re.search(logical_line)) + if res: + yield (0, "O323: assertEqual(True, A) or assertEqual(False, A) " + "sentences not allowed") + + +def assert_equal_in(logical_line): + """Check for assertEqual(A in B, True), assertEqual(True, A in B), + + assertEqual(A in B, False) or assertEqual(False, A in B) sentences + + O338 + """ + res = (assert_equal_in_start_with_true_or_false_re.search(logical_line) or + assert_equal_in_end_with_true_or_false_re.search(logical_line)) + if res: + yield (0, "O338: Use assertIn/NotIn(A, B) rather than " + "assertEqual(A in B, True/False) when checking collection " + "contents.") def factory(register): @@ -127,3 +175,6 @@ def factory(register): register(use_jsonutils) register(no_author_tags) register(no_translate_debug_logs) + register(assert_true_instance) + register(assert_equal_true_or_false) + register(assert_equal_in) diff --git a/octavia/tests/unit/test_hacking.py b/octavia/tests/unit/test_hacking.py new file mode 100644 index 0000000000..be846026fa --- /dev/null +++ b/octavia/tests/unit/test_hacking.py @@ -0,0 +1,109 @@ +# Copyright 2015 +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from oslotest import base + +from octavia.hacking import checks + + +class HackingTestCase(base.BaseTestCase): + """Hacking test class. + + This class tests the hacking checks in octavia.hacking.checks by passing + strings to the check methods like the pep8/flake8 parser would. The parser + loops over each line in the file and then passes the parameters to the + check method. The parameter names in the check method dictate what type of + object is passed to the check method. The parameter types are:: + + logical_line: A processed line with the following modifications: + - Multi-line statements converted to a single line. + - Stripped left and right. + - Contents of strings replaced with "xxx" of same length. + - Comments removed. + physical_line: Raw line of text from the input file. + lines: a list of the raw lines from the input file + tokens: the tokens that contribute to this logical line + line_number: line number in the input file + total_lines: number of lines in the input file + blank_lines: blank lines before this one + indent_char: indentation character in this file (" " or "\t") + indent_level: indentation (with tabs expanded to multiples of 8) + previous_indent_level: indentation on previous line + previous_logical: previous logical line + filename: Path of the file being run through pep8 + + When running a test on a check method the return will be False/None if + there is no violation in the sample input. If there is an error a tuple is + returned with a position in the line, and a message. So to check the result + just assertTrue if the check is expected to fail and assertFalse if it + should pass. + """ + + def test_assert_true_instance(self): + self.assertEqual(1, len(list(checks.assert_true_instance( + "self.assertTrue(isinstance(e, " + "exception.BuildAbortException))")))) + + self.assertEqual(0, len(list(checks.assert_true_instance( + "self.assertTrue()")))) + + def test_assert_equal_in(self): + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(a in b, True)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual('str' in 'string', True)")))) + + self.assertEqual(0, len(list(checks.assert_equal_in( + "self.assertEqual(any(a==1 for a in b), True)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(True, a in b)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(True, 'str' in 'string')")))) + + self.assertEqual(0, len(list(checks.assert_equal_in( + "self.assertEqual(True, any(a==1 for a in b))")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(a in b, False)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual('str' in 'string', False)")))) + + self.assertEqual(0, len(list(checks.assert_equal_in( + "self.assertEqual(any(a==1 for a in b), False)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(False, a in b)")))) + + self.assertEqual(1, len(list(checks.assert_equal_in( + "self.assertEqual(False, 'str' in 'string')")))) + + self.assertEqual(0, len(list(checks.assert_equal_in( + "self.assertEqual(False, any(a==1 for a in b))")))) + + def test_assert_equal_true_or_false(self): + self.assertEqual(1, len(list(checks.assert_equal_true_or_false( + "self.assertEqual(True, A)")))) + + self.assertEqual(1, len(list(checks.assert_equal_true_or_false( + "self.assertEqual(False, A)")))) + + self.assertEqual(0, len(list(checks.assert_equal_true_or_false( + "self.assertTrue()")))) + + self.assertEqual(0, len(list(checks.assert_equal_true_or_false( + "self.assertFalse()"))))