From 0138345cc310f53da227f9e2184c8a586e22486a Mon Sep 17 00:00:00 2001 From: Nguyen Quang Huy Date: Thu, 16 Aug 2018 17:07:40 +0700 Subject: [PATCH] Improve pep8 checking along with hacking This change will help to check pep8. And the gates will check this missing in the next time. Change-Id: I62a29a8880e4d2cc1d410ef69991249b85ab7f6e --- cyborg/hacking/__init__.py | 0 cyborg/hacking/checks.py | 173 +++++++++++++++++++++ cyborg/tests/unit/test_hacking.py | 247 ++++++++++++++++++++++++++++++ tox.ini | 3 + 4 files changed, 423 insertions(+) create mode 100644 cyborg/hacking/__init__.py create mode 100644 cyborg/hacking/checks.py create mode 100644 cyborg/tests/unit/test_hacking.py diff --git a/cyborg/hacking/__init__.py b/cyborg/hacking/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/cyborg/hacking/checks.py b/cyborg/hacking/checks.py new file mode 100644 index 00000000..c114be32 --- /dev/null +++ b/cyborg/hacking/checks.py @@ -0,0 +1,173 @@ +# +# 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 Magnum 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 magnum/tests/unit/test_hacking.py + +""" +UNDERSCORE_IMPORT_FILES = [] + +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_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*\(") +log_translation = re.compile( + r"(.)*LOG\.(audit|error|critical)\(\s*('|\")") +log_translation_info = re.compile( + r"(.)*LOG\.(info)\(\s*(_\(|'|\")") +log_translation_exception = re.compile( + r"(.)*LOG\.(exception)\(\s*(_\(|'|\")") +log_translation_LW = re.compile( + r"(.)*LOG\.(warning|warn)\(\s*(_\(|'|\")") +custom_underscore_check = re.compile(r"(.)*_\s*=\s*(.)*") +underscore_import_check = re.compile(r"(.)*import _(.)*") +translated_log = re.compile( + r"(.)*LOG\.(audit|error|info|critical|exception)" + "\(\s*_\(\s*('|\")") +string_translation = re.compile(r"[^_]*_\(\s*('|\")") + + +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_not_none(logical_line): + """Check for assertEqual(A is not None) sentences M302""" + msg = "M302: assertEqual(A is not None) sentences not allowed." + res = assert_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 no_log_warn(logical_line): + """Disallow 'LOG.warn(' + + Deprecated LOG.warn(), instead use LOG.warning + https://bugs.launchpad.net/magnum/+bug/1508442 + + M352 + """ + + msg = ("M352: LOG.warn is deprecated, please use LOG.warning!") + if "LOG.warn(" in logical_line: + yield (0, msg) + + +def check_explicit_underscore_import(logical_line, filename): + """Check for explicit import of the _ function + + We need to ensure that any files that are using the _() function + to translate logs are explicitly importing the _ function. We + can't trust unit test to catch whether the import has been + added so we need to check for it here. + """ + + # Build a list of the files that have _ imported. No further + # checking needed once it is found. + if filename in UNDERSCORE_IMPORT_FILES: + pass + elif (underscore_import_check.match(logical_line) or + custom_underscore_check.match(logical_line)): + UNDERSCORE_IMPORT_FILES.append(filename) + elif (translated_log.match(logical_line) or + string_translation.match(logical_line)): + yield(0, "M340: Found use of _() without explicit import of _ !") + + +def factory(register): + register(no_mutable_default_args) + 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) + register(no_log_warn) + register(check_explicit_underscore_import) diff --git a/cyborg/tests/unit/test_hacking.py b/cyborg/tests/unit/test_hacking.py new file mode 100644 index 00000000..132b6a40 --- /dev/null +++ b/cyborg/tests/unit/test_hacking.py @@ -0,0 +1,247 @@ +# 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 cyborg.hacking import checks +from cyborg.tests import base + + +class HackingTestCase(base.TestCase): + """Hacking test class. + + This class tests the hacking checks in magnum.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_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.assertIsNotNone()" + 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_no_log_warn(self): + errors = [(1, 0, "M352")] + check = checks.no_log_warn + code = """ + LOG.warn("LOG.warn is deprecated") + """ + self._assert_has_errors(code, check, errors) + + code = """ + LOG.warning("LOG.warn is deprecated") + """ + 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__)")))) + + def test_check_explicit_underscore_import(self): + self.assertEqual(len(list(checks.check_explicit_underscore_import( + "LOG.info(_('My info message'))", + "magnum/tests/other_files.py"))), 1) + self.assertEqual(len(list(checks.check_explicit_underscore_import( + "msg = _('My message')", + "magnum/tests/other_files.py"))), 1) + self.assertEqual(len(list(checks.check_explicit_underscore_import( + "from magnum.i18n import _", + "magnum/tests/other_files.py"))), 0) + self.assertEqual(len(list(checks.check_explicit_underscore_import( + "LOG.info(_('My info message'))", + "magnum/tests/other_files.py"))), 0) + self.assertEqual(len(list(checks.check_explicit_underscore_import( + "msg = _('My message')", + "magnum/tests/other_files.py"))), 0) + self.assertEqual(len(list(checks.check_explicit_underscore_import( + "from magnum.i18n import _, _LW", + "magnum/tests/other_files2.py"))), 0) + self.assertEqual(len(list(checks.check_explicit_underscore_import( + "msg = _('My message')", + "magnum/tests/other_files2.py"))), 0) + self.assertEqual(len(list(checks.check_explicit_underscore_import( + "_ = translations.ugettext", + "magnum/tests/other_files3.py"))), 0) + self.assertEqual(len(list(checks.check_explicit_underscore_import( + "msg = _('My message')", + "magnum/tests/other_files3.py"))), 0) diff --git a/tox.ini b/tox.ini index a4983885..2003dab3 100644 --- a/tox.ini +++ b/tox.ini @@ -83,3 +83,6 @@ show-source = True ignore = E123,E125 builtins = _ exclude=.venv,.git,.tox,dist,doc,*lib/python*,*egg,build + +[hacking] +local-check-factory = cyborg.hacking.checks.factory