Add Hacking Rule to Higgins.

Change-Id: I22509f5054a574656cc5fcc41aa0d366b9a4134b

HACKING.rst

@@ -2,3 +2,22 @@ Higgins Style Commandments
 ===============================================
 
 Read the OpenStack Style Commandments http://docs.openstack.org/developer/hacking/
+
+Higgins Specific Commandments
+----------------------------
+
+- [M302] Change assertEqual(A is not None) by optimal assert like
+  assertIsNotNone(A).
+- [M310] timeutils.utcnow() wrapper must be used instead of direct calls to
+  datetime.datetime.utcnow() to make it easy to override its return value.
+- [M316] Change assertTrue(isinstance(A, B)) by optimal assert like
+  assertIsInstance(A, B).
+- [M318] Change assertEqual(A, None) or assertEqual(None, A) by optimal assert
+  like assertIsNone(A)
+- [M322] Method's default argument shouldn't be mutable.
+- [M323] Change assertEqual(True, A) or assertEqual(False, A) by optimal assert
+  like assertTrue(A) or assertFalse(A)
+- [M336] Must use a dict comprehension instead of a dict constructor
+  with a sequence of key-value pairs.
+- [M338] Use assertIn/NotIn(A, B) rather than assertEqual(A in B, True/False).
+- [M339] Don't use xrange()

higgins/common/service.py

@@ -44,7 +44,9 @@ LOG = log.getLogger(__name__)
 CONF.register_opts(service_opts)
 
 
-def prepare_service(argv=[]):
+def prepare_service(argv=None):
+    if argv is None:
+        argv = []
     log.register_options(CONF)
     config.parse_args(argv)
     log.setup(CONF, 'higgins')

higgins/hacking/checks.py

@@ -0,0 +1,156 @@
+# Copyright (c) 2016 Intel, Inc.
+# All Rights Reserved.
+#
+# 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.
+
+import re
+
+"""
+Guidelines for writing new hacking checks
+
+ - Use only for Higgins specific tests. OpenStack general tests
+   should be submitted to the common 'hacking' module.
+ - Pick numbers in the range M3xx. Find the current test with
+   the highest allocated number and then pick the next value.
+   If nova has an N3xx code for that test, use the same number.
+ - Keep the test method code in the source file ordered based
+   on the M3xx value.
+ - List the new rule in the top level HACKING.rst file
+ - Add test cases for each new rule to higgins/tests/unit/test_hacking.py
+
+"""
+
+mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])")
+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_end_with_none_re = re.compile(
+    r"(.)*assertEqual\((\w|\.|\'|\"|\[|\])+, None\)")
+assert_equal_start_with_none_re = re.compile(
+    r"(.)*assertEqual\(None, (\w|\.|\'|\"|\[|\])+\)")
+assert_equal_with_true_re = re.compile(
+    r"assertEqual\(True,")
+assert_equal_with_false_re = re.compile(
+    r"assertEqual\(False,")
+asse_equal_with_is_not_none_re = re.compile(
+    r"assertEqual\(.*?\s+is+\s+not+\s+None\)$")
+assert_true_isinstance_re = re.compile(
+    r"(.)*assertTrue\(isinstance\((\w|\.|\'|\"|\[|\])+, "
+    "(\w|\.|\'|\"|\[|\])+\)\)")
+dict_constructor_with_list_copy_re = re.compile(r".*\bdict\((\[)?(\(|\[)")
+assert_xrange_re = re.compile(
+    r"\s*xrange\s*\(")
+
+
+def assert_equal_none(logical_line):
+    """Check for assertEqual(A, None) or assertEqual(None, A) sentences
+
+    M318
+    """
+    msg = ("M318: assertEqual(A, None) or assertEqual(None, A) "
+           "sentences not allowed")
+    res = (assert_equal_start_with_none_re.match(logical_line) or
+           assert_equal_end_with_none_re.match(logical_line))
+    if res:
+        yield (0, msg)
+
+
+def no_mutable_default_args(logical_line):
+    msg = "M322: Method's default argument shouldn't be mutable!"
+    if mutable_default_args.match(logical_line):
+        yield (0, msg)
+
+
+def assert_equal_true_or_false(logical_line):
+    """Check for assertEqual(True, A) or assertEqual(False, A) sentences
+
+    M323
+    """
+    res = (assert_equal_with_true_re.search(logical_line) or
+           assert_equal_with_false_re.search(logical_line))
+    if res:
+        yield (0, "M323: assertEqual(True, A) or assertEqual(False, A) "
+               "sentences not allowed")
+
+
+def assert_equal_not_none(logical_line):
+    """Check for assertEqual(A is not None) sentences M302"""
+    msg = "M302: assertEqual(A is not None) sentences not allowed."
+    res = asse_equal_with_is_not_none_re.search(logical_line)
+    if res:
+        yield (0, msg)
+
+
+def assert_true_isinstance(logical_line):
+    """Check for assertTrue(isinstance(a, b)) sentences
+
+    M316
+    """
+    if assert_true_isinstance_re.match(logical_line):
+        yield (0, "M316: assertTrue(isinstance(a, b)) sentences not allowed")
+
+
+def assert_equal_in(logical_line):
+    """Check for assertEqual(True|False, A in B), assertEqual(A in B, True|False)
+
+    M338
+    """
+    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, "M338: Use assertIn/NotIn(A, B) rather than "
+                  "assertEqual(A in B, True/False) when checking collection "
+                  "contents.")
+
+
+def no_xrange(logical_line):
+    """Disallow 'xrange()'
+
+    M339
+    """
+    if assert_xrange_re.match(logical_line):
+        yield(0, "M339: Do not use xrange().")
+
+
+def use_timeutils_utcnow(logical_line, filename):
+    # tools are OK to use the standard datetime module
+    if "/tools/" in filename:
+        return
+
+    msg = "M310: timeutils.utcnow() must be used instead of datetime.%s()"
+    datetime_funcs = ['now', 'utcnow']
+    for f in datetime_funcs:
+        pos = logical_line.find('datetime.%s' % f)
+        if pos != -1:
+            yield (pos, msg % f)
+
+
+def dict_constructor_with_list_copy(logical_line):
+    msg = ("M336: Must use a dict comprehension instead of a dict constructor"
+           " with a sequence of key-value pairs."
+           )
+    if dict_constructor_with_list_copy_re.match(logical_line):
+        yield (0, msg)
+
+
+def factory(register):
+    register(no_mutable_default_args)
+    register(assert_equal_none)
+    register(assert_equal_true_or_false)
+    register(assert_equal_not_none)
+    register(assert_true_isinstance)
+    register(assert_equal_in)
+    register(use_timeutils_utcnow)
+    register(dict_constructor_with_list_copy)
+    register(no_xrange)

higgins/tests/unit/test_hacking.py

@@ -0,0 +1,234 @@
+#    Copyright 2015 Intel, Inc.
+#
+#    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.
+
+import textwrap
+
+import mock
+import pep8
+
+from higgins.hacking import checks
+from higgins.tests import base
+
+
+class HackingTestCase(base.TestCase):
+    """Hacking test class.
+
+    This class tests the hacking checks higgins .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.
+    """
+    # We are patching pep8 so that only the check under test is actually
+    # installed.
+
+    @mock.patch('pep8._checks',
+                {'physical_line': {}, 'logical_line': {}, 'tree': {}})
+    def _run_check(self, code, checker, filename=None):
+        pep8.register_check(checker)
+
+        lines = textwrap.dedent(code).strip().splitlines(True)
+
+        checker = pep8.Checker(filename=filename, lines=lines)
+        checker.check_all()
+        checker.report._deferred_print.sort()
+        return checker.report._deferred_print
+
+    def _assert_has_errors(self, code, checker, expected_errors=None,
+                           filename=None):
+        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_in(self):
+        errors = [(1, 0, "M338")]
+        check = checks.assert_equal_in
+
+        code = "self.assertEqual(a in b, True)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertEqual('str' in 'string', True)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertEqual(any(a==1 for a in b), True)"
+        self._assert_has_no_errors(code, check)
+
+        code = "self.assertEqual(True, a in b)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertEqual(True, 'str' in 'string')"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertEqual(True, any(a==1 for a in b))"
+        self._assert_has_no_errors(code, check)
+
+        code = "self.assertEqual(a in b, False)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertEqual('str' in 'string', False)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertEqual(any(a==1 for a in b), False)"
+        self._assert_has_no_errors(code, check)
+
+        code = "self.assertEqual(False, a in b)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertEqual(False, 'str' in 'string')"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertEqual(False, any(a==1 for a in b))"
+        self._assert_has_no_errors(code, check)
+
+    def test_assert_equal_none(self):
+        errors = [(1, 0, "M318")]
+        check = checks.assert_equal_none
+
+        code = "self.assertEqual(A, None)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertEqual(None, A)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertIsNone()"
+        self._assert_has_no_errors(code, check)
+
+    def test_assert_equal_true_or_false(self):
+        errors = [(1, 0, "M323")]
+        check = checks.assert_equal_true_or_false
+
+        code = "self.assertEqual(True, A)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertEqual(False, A)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertTrue()"
+        self._assert_has_no_errors(code, check)
+
+        code = "self.assertFalse()"
+        self._assert_has_no_errors(code, check)
+
+    def test_no_mutable_default_args(self):
+        errors = [(1, 0, "M322")]
+        check = checks.no_mutable_default_args
+
+        code = "def get_info_from_bdm(virt_type, bdm, mapping=[])"
+        self._assert_has_errors(code, check, errors)
+
+        code = "defined = []"
+        self._assert_has_no_errors(code, check)
+
+        code = "defined, undefined = [], {}"
+        self._assert_has_no_errors(code, check)
+
+    def test_assert_is_not_none(self):
+        errors = [(1, 0, "M302")]
+        check = checks.assert_equal_not_none
+
+        code = "self.assertEqual(A is not None)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertIsNone()"
+        self._assert_has_no_errors(code, check)
+
+    def test_assert_true_isinstance(self):
+        errors = [(1, 0, "M316")]
+        check = checks.assert_true_isinstance
+
+        code = "self.assertTrue(isinstance(e, exception.BuilAbortException))"
+        self._assert_has_errors(code, check, errors)
+
+        code = "self.assertTrue()"
+        self._assert_has_no_errors(code, check)
+
+    def test_no_xrange(self):
+        errors = [(1, 0, "M339")]
+        check = checks.no_xrange
+
+        code = "xrange(45)"
+        self._assert_has_errors(code, check, errors)
+
+        code = "range(45)"
+        self._assert_has_no_errors(code, check)
+
+    def test_use_timeunitls_utcow(self):
+        errors = [(1, 0, "M310")]
+        check = checks.use_timeutils_utcnow
+
+        code = "datetime.now"
+        self._assert_has_errors(code, check, errors)
+
+        code = "datetime.utcnow"
+        self._assert_has_errors(code, check, errors)
+
+        code = "datetime.aa"
+        self._assert_has_no_errors(code, check)
+
+        code = "aaa"
+        self._assert_has_no_errors(code, check)
+
+    def test_dict_constructor_with_list_copy(self):
+        self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy(
+            "    dict([(i, connect_info[i])"))))
+
+        self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy(
+            "    attrs = dict([(k, _from_json(v))"))))
+
+        self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy(
+            "        type_names = dict((value, key) for key, value in"))))
+
+        self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy(
+            "   dict((value, key) for key, value in"))))
+
+        self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy(
+            "foo(param=dict((k, v) for k, v in bar.items()))"))))
+
+        self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy(
+            " dict([[i,i] for i in range(3)])"))))
+
+        self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy(
+            "  dd = dict([i,i] for i in range(3))"))))
+
+        self.assertEqual(0, len(list(checks.dict_constructor_with_list_copy(
+            "        create_kwargs = dict(snapshot=snapshot,"))))
+
+        self.assertEqual(0, len(list(checks.dict_constructor_with_list_copy(
+            "      self._render_dict(xml, data_el, data.__dict__)"))))

test-requirements.txt

@@ -5,6 +5,7 @@
 hacking<0.11,>=0.10.2 # Apache-2.0
 
 coverage>=3.6 # Apache-2.0
+mock>=1.2 # BSD
 python-subunit>=0.0.18 # Apache-2.0/BSD
 sphinx!=1.2.0,!=1.3b1,<1.3,>=1.1.2 # BSD
 oslosphinx!=3.4.0,>=2.5.0 # Apache-2.0

tox.ini

@@ -56,6 +56,9 @@ show-source = True
 builtins = _
 exclude=.venv,.git,.tox,dist,doc,*openstack/common*,*lib/python*,*egg,build
 
+[hacking]
+local-check-factory = higgins.hacking.checks.factory
+
 [testenv:fast8]
 # NOTE(sheel.rana): `tox -e fast8` cab be used to run pep8 command only for
 # updated code instead for running whole code base.