From f3fdae5d0aa0f115a331a219f628d0e67451dab7 Mon Sep 17 00:00:00 2001 From: zhurong Date: Mon, 21 Dec 2015 16:14:20 +0800 Subject: [PATCH] Add mutable default arguments hacking check Add mutable default arguments hacking check for murano The patch was inspired from: I4aa8aede57d6fd31b4b30c3f7535b819e591165d Change-Id: I9b5d7c1edb96fed6936a27b542f902fe1b579c23 Closes-Bug: #1530041 --- HACKING.rst | 5 ++ murano/hacking/__init__.py | 0 murano/hacking/checks.py | 41 +++++++++++++++ murano/tests/unit/test_hacking.py | 83 +++++++++++++++++++++++++++++++ tox.ini | 1 + 5 files changed, 130 insertions(+) create mode 100644 murano/hacking/__init__.py create mode 100644 murano/hacking/checks.py create mode 100644 murano/tests/unit/test_hacking.py diff --git a/HACKING.rst b/HACKING.rst index cb0747656..5e0a25086 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -2,3 +2,8 @@ Style Commandments ================== Read the OpenStack Style Commandments http://docs.openstack.org/developer/hacking/ + +Murano Specific Commandments +--------------------------- + +- [M322] Method's default argument shouldn't be mutable. diff --git a/murano/hacking/__init__.py b/murano/hacking/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/murano/hacking/checks.py b/murano/hacking/checks.py new file mode 100644 index 000000000..74e3b2770 --- /dev/null +++ b/murano/hacking/checks.py @@ -0,0 +1,41 @@ +# Copyright (c) 2015 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 Murano 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 /tests/unit/test_hacking.py +""" + +mutable_default_args = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])") + + +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 factory(register): + register(no_mutable_default_args) diff --git a/murano/tests/unit/test_hacking.py b/murano/tests/unit/test_hacking.py new file mode 100644 index 000000000..00a11dff3 --- /dev/null +++ b/murano/tests/unit/test_hacking.py @@ -0,0 +1,83 @@ +# 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 mock +import pep8 +import textwrap + +from murano.hacking import checks +from murano.tests.unit import base + + +class HackingTestCase(base.MuranoTestCase): + """This class tests the hacking checks in murano.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_no_mutable_default_args(self): + self.assertEqual(1, len(list(checks.no_mutable_default_args( + "def get_info_from_bdm(virt_type, bdm, mapping=[])")))) + + self.assertEqual(0, len(list(checks.no_mutable_default_args( + "defined = []")))) + + self.assertEqual(0, len(list(checks.no_mutable_default_args( + "defined, undefined = [], {}")))) diff --git a/tox.ini b/tox.ini index 07706b804..0dc39b25e 100644 --- a/tox.ini +++ b/tox.ini @@ -51,5 +51,6 @@ builtins = _ exclude=.venv,.git,.tox,dist,doc,*openstack/common*,*lib/python*,*egg,tools [hacking] +local-check-factory = murano.hacking.checks.factory import_exceptions = oslo.db.sqlalchemy.test_base, murano.common.i18n