From 6ef76f47b10527481a47d0f4482f02b75475bfb1 Mon Sep 17 00:00:00 2001 From: Madhuri Kumari Date: Fri, 20 May 2016 11:30:28 +0530 Subject: [PATCH] Add Hacking Rule to Higgins. Change-Id: I22509f5054a574656cc5fcc41aa0d366b9a4134b --- HACKING.rst | 19 +++ higgins/common/service.py | 4 +- higgins/hacking/__init__.py | 0 higgins/hacking/checks.py | 156 +++++++++++++++++++ higgins/tests/unit/test_hacking.py | 234 +++++++++++++++++++++++++++++ test-requirements.txt | 1 + tox.ini | 3 + 7 files changed, 416 insertions(+), 1 deletion(-) create mode 100644 higgins/hacking/__init__.py create mode 100644 higgins/hacking/checks.py create mode 100644 higgins/tests/unit/test_hacking.py diff --git a/HACKING.rst b/HACKING.rst index 0ccf0f455..774acd996 100644 --- a/HACKING.rst +++ b/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() diff --git a/higgins/common/service.py b/higgins/common/service.py index d22ff701e..eeab470a2 100644 --- a/higgins/common/service.py +++ b/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') diff --git a/higgins/hacking/__init__.py b/higgins/hacking/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/higgins/hacking/checks.py b/higgins/hacking/checks.py new file mode 100644 index 000000000..2ca38e5f3 --- /dev/null +++ b/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) diff --git a/higgins/tests/unit/test_hacking.py b/higgins/tests/unit/test_hacking.py new file mode 100644 index 000000000..91e9eea83 --- /dev/null +++ b/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__)")))) diff --git a/test-requirements.txt b/test-requirements.txt index 5c116a23c..cfb840496 100644 --- a/test-requirements.txt +++ b/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 diff --git a/tox.ini b/tox.ini index 5cf897b23..fd12ff44a 100644 --- a/tox.ini +++ b/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.