From dea8754c6dadc382856d4115006e7b488620dafd Mon Sep 17 00:00:00 2001 From: Jeremy Liu Date: Fri, 11 Nov 2016 18:07:02 +0800 Subject: [PATCH] Introduce hacking check to Barbican Add hacking check to ensure we use proper rules and follow community guideline [1]. [1] http://docs.openstack.org/developer/hacking/ Change-Id: I61e366969385aa044aea9d9678fbc91d32325497 --- HACKING.rst | 92 ++++++++ barbican/hacking/__init__.py | 0 barbican/hacking/checks.py | 379 +++++++++++++++++++++++++++++++++ barbican/tests/test_hacking.py | 295 +++++++++++++++++++++++++ test-requirements.txt | 6 +- tox.ini | 3 + 6 files changed, 774 insertions(+), 1 deletion(-) create mode 100644 barbican/hacking/__init__.py create mode 100644 barbican/hacking/checks.py create mode 100644 barbican/tests/test_hacking.py diff --git a/HACKING.rst b/HACKING.rst index 4bcd5c29..2327bafc 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -9,3 +9,95 @@ Barbican Style Commandments Barbican Specific Commandments ------------------------------- +- [B310] Check for improper use of logging format arguments. +- [B311] Use assertIsNone(...) instead of assertEqual(None, ...). +- [B312] Use assertTrue(...) rather than assertEqual(True, ...). +- [B313] Validate that debug level logs are not translated. +- [B314] str() and unicode() cannot be used on an exception. Remove or use six.text_type(). +- [B315] Translated messages cannot be concatenated. String should be + included in translated message. +- [B316] Log messages, except debug ones, require translations! +- [B317] 'oslo_' should be used instead of 'oslo.' +- [B318] Must use a dict comprehension instead of a dict constructor + with a sequence of key-value pairs. +- [B319] Ensure to not use xrange(). +- [B320] Do not use LOG.warn as it's deprecated. +- [B321] Use assertIsNotNone(...) rather than assertNotEqual(None, ...) or + assertIsNot(None, ...). + +LOG Translations +---------------- + +LOG.debug messages will not get translated. Use ``_LI()`` for +``LOG.info``, ``_LW`` for ``LOG.warning``, ``_LE`` for ``LOG.error`` +and ``LOG.exception``, and ``_LC()`` for ``LOG.critical``. + +``_()`` is preferred for any user facing message, even if it is also +going to a log file. This ensures that the translated version of the +message will be available to the user. + +The log marker functions (``_LI()``, ``_LW()``, ``_LE()``, and ``_LC()``) +must only be used when the message is only sent directly to the log. +Anytime that the message will be passed outside of the current context +(for example as part of an exception) the ``_()`` marker function +must be used. + +A common pattern is to define a single message object and use it more +than once, for the log call and the exception. In that case, ``_()`` +must be used because the message is going to appear in an exception that +may be presented to the user. + +For more details about translations, see +http://docs.openstack.org/developer/oslo.i18n/guidelines.html + +Creating Unit Tests +------------------- +For every new feature, unit tests should be created that both test and +(implicitly) document the usage of said feature. If submitting a patch for a +bug that had no unit test, a new passing unit test should be added. If a +submitted bug fix does have a unit test, be sure to add a new one that fails +without the patch and passes with the patch. + +Running Tests +------------- +The testing system is based on a combination of tox and testr. If you just +want to run the whole suite, run `tox` and all will be fine. However, if +you'd like to dig in a bit more, you might want to learn some things about +testr itself. A basic walkthrough for OpenStack can be found at +http://wiki.openstack.org/testr + +OpenStack Trademark +------------------- + +OpenStack is a registered trademark of OpenStack, LLC, and uses the +following capitalization: + + OpenStack + +Commit Messages +--------------- +Using a common format for commit messages will help keep our git history +readable. Follow these guidelines: + + First, provide a brief summary (it is recommended to keep the commit title + under 50 chars). + + The first line of the commit message should provide an accurate + description of the change, not just a reference to a bug or + blueprint. It must be followed by a single blank line. + + Following your brief summary, provide a more detailed description of + the patch, manually wrapping the text at 72 characters. This + description should provide enough detail that one does not have to + refer to external resources to determine its high-level functionality. + + Once you use 'git review', two lines will be appended to the commit + message: a blank line followed by a 'Change-Id'. This is important + to correlate this commit with a specific review in Gerrit, and it + should not be modified. + +For further information on constructing high quality commit messages, +and how to split up commits into a series of changes, consult the +project wiki: + + http://wiki.openstack.org/GitCommitMessages diff --git a/barbican/hacking/__init__.py b/barbican/hacking/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/barbican/hacking/checks.py b/barbican/hacking/checks.py new file mode 100644 index 00000000..31a7f20b --- /dev/null +++ b/barbican/hacking/checks.py @@ -0,0 +1,379 @@ +# Copyright (c) 2016, GohighSec +# 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 ast +import re +import six + +import pep8 + + +""" +Guidelines for writing new hacking checks + + - Use only for Barbican specific tests. OpenStack general tests + should be submitted to the common 'hacking' module. + - Pick numbers in the range B3xx. Find the current test with + the highest allocated number and then pick the next value. + - Keep the test method code in the source file ordered based + on the B3xx value. + - List the new rule in the top level HACKING.rst file + - Add test cases for each new rule to barbican/tests/test_hacking.py + +""" + +UNDERSCORE_IMPORT_FILES = [] + +log_translation = re.compile( + r"(.)*LOG\.(audit|error|info|critical|exception)\(\s*('|\")") +log_translation_LC = re.compile( + r"(.)*LOG\.(critical)\(\s*(_\(|'|\")") +log_translation_LE = re.compile( + r"(.)*LOG\.(error|exception)\(\s*(_\(|'|\")") +log_translation_LI = re.compile( + r"(.)*LOG\.(info)\(\s*(_\(|'|\")") +log_translation_LW = re.compile( + r"(.)*LOG\.(warning|warn)\(\s*(_\(|'|\")") +translated_log = re.compile( + r"(.)*LOG\.(audit|error|info|warn|warning|critical|exception)" + "\(\s*_\(\s*('|\")") +string_translation = re.compile(r"[^_]*_\(\s*('|\")") +underscore_import_check = re.compile(r"(.)*import _$") +underscore_import_check_multi = re.compile(r"(.)*import (.)*_, (.)*") +# We need this for cases where they have created their own _ function. +custom_underscore_check = re.compile(r"(.)*_\s*=\s*(.)*") +oslo_namespace_imports = re.compile(r"from[\s]*oslo[.](.*)") +dict_constructor_with_list_copy_re = re.compile(r".*\bdict\((\[)?(\(|\[)") +assert_no_xrange_re = re.compile(r"\s*xrange\s*\(") +assert_True = re.compile(r".*assertEqual\(True, .*\)") +assert_None = re.compile(r".*assertEqual\(None, .*\)") +assert_Not_Equal = re.compile(r".*assertNotEqual\(None, .*\)") +assert_Is_Not = re.compile(r".*assertIsNot\(None, .*\)") +no_log_warn = re.compile(r".*LOG.warn\(.*\)") + + +class BaseASTChecker(ast.NodeVisitor): + """Provides a simple framework for writing AST-based checks. + + Subclasses should implement visit_* methods like any other AST visitor + implementation. When they detect an error for a particular node the + method should call ``self.add_error(offending_node)``. Details about + where in the code the error occurred will be pulled from the node + object. + + Subclasses should also provide a class variable named CHECK_DESC to + be used for the human readable error message. + + """ + + CHECK_DESC = 'No check message specified' + + def __init__(self, tree, filename): + """This object is created automatically by pep8. + + :param tree: an AST tree + :param filename: name of the file being analyzed + (ignored by our checks) + """ + self._tree = tree + self._errors = [] + + def run(self): + """Called automatically by pep8.""" + self.visit(self._tree) + return self._errors + + def add_error(self, node, message=None): + """Add an error caused by a node to the list of errors for pep8.""" + message = message or self.CHECK_DESC + error = (node.lineno, node.col_offset, message, self.__class__) + self._errors.append(error) + + def _check_call_names(self, call_node, names): + if isinstance(call_node, ast.Call): + if isinstance(call_node.func, ast.Name): + if call_node.func.id in names: + return True + return False + + +def no_translate_debug_logs(logical_line, filename): + """Check for 'LOG.debug(u._(' + + As per our translation policy, + https://wiki.openstack.org/wiki/LoggingStandards#Log_Translation + we shouldn't translate debug level logs. + + * This check assumes that 'LOG' is a logger. + * Use filename so we can start enforcing this in specific folders instead + of needing to do so all at once. + + B313 + """ + if logical_line.startswith("LOG.debug(u._("): + yield(0, "B313 Don't translate debug level logs") + + +class CheckLoggingFormatArgs(BaseASTChecker): + """Check for improper use of logging format arguments. + + LOG.debug("Volume %s caught fire and is at %d degrees C and climbing.", + ('volume1', 500)) + + The format arguments should not be a tuple as it is easy to miss. + + """ + + CHECK_DESC = 'B310 Log method arguments should not be a tuple.' + LOG_METHODS = [ + 'debug', 'info', + 'warn', 'warning', + 'error', 'exception', + 'critical', 'fatal', + 'trace', 'log' + ] + + def _find_name(self, node): + """Return the fully qualified name or a Name or Attribute.""" + if isinstance(node, ast.Name): + return node.id + elif (isinstance(node, ast.Attribute) + and isinstance(node.value, (ast.Name, ast.Attribute))): + method_name = node.attr + obj_name = self._find_name(node.value) + if obj_name is None: + return None + return obj_name + '.' + method_name + elif isinstance(node, six.string_types): + return node + else: # could be Subscript, Call or many more + return None + + def visit_Call(self, node): + """Look for the 'LOG.*' calls.""" + # extract the obj_name and method_name + if isinstance(node.func, ast.Attribute): + obj_name = self._find_name(node.func.value) + if isinstance(node.func.value, ast.Name): + method_name = node.func.attr + elif isinstance(node.func.value, ast.Attribute): + obj_name = self._find_name(node.func.value) + method_name = node.func.attr + else: # could be Subscript, Call or many more + return super(CheckLoggingFormatArgs, self).generic_visit(node) + + # obj must be a logger instance and method must be a log helper + if (obj_name != 'LOG' + or method_name not in self.LOG_METHODS): + return super(CheckLoggingFormatArgs, self).generic_visit(node) + + # the call must have arguments + if not len(node.args): + return super(CheckLoggingFormatArgs, self).generic_visit(node) + + # any argument should not be a tuple + for arg in node.args: + if isinstance(arg, ast.Tuple): + self.add_error(arg) + + return super(CheckLoggingFormatArgs, self).generic_visit(node) + + +def validate_log_translations(logical_line, physical_line, filename): + # Translations are not required in the test directories. + if ("barbican/tests" in filename or "functional" in filename): + return + if pep8.noqa(physical_line): + return + msg = "B316: LOG.critical messages require translations `_LC()`!" + if log_translation_LC.match(logical_line): + yield (0, msg) + msg = ("B316: LOG.error and LOG.exception messages require translations " + "`_LE()`!") + if log_translation_LE.match(logical_line): + yield (0, msg) + msg = "B316: LOG.info messages require translations `_LI()`!" + if log_translation_LI.match(logical_line): + yield (0, msg) + msg = "B316: LOG.warning messages require translations `_LW()`!" + if log_translation_LW.match(logical_line): + yield (0, msg) + msg = "B316: Log messages require translations!" + if log_translation.match(logical_line): + yield (0, msg) + + +class CheckForStrUnicodeExc(BaseASTChecker): + """Checks for the use of str() or unicode() on an exception. + + This currently only handles the case where str() or unicode() + is used in the scope of an exception handler. If the exception + is passed into a function, returned from an assertRaises, or + used on an exception created in the same scope, this does not + catch it. + """ + + CHECK_DESC = ('B314 str() and unicode() cannot be used on an ' + 'exception. Remove or use six.text_type()') + + def __init__(self, tree, filename): + super(CheckForStrUnicodeExc, self).__init__(tree, filename) + self.name = [] + self.already_checked = [] + + # Python 2 + def visit_TryExcept(self, node): + for handler in node.handlers: + if handler.name: + self.name.append(handler.name.id) + super(CheckForStrUnicodeExc, self).generic_visit(node) + self.name = self.name[:-1] + else: + super(CheckForStrUnicodeExc, self).generic_visit(node) + + # Python 3 + def visit_ExceptHandler(self, node): + if node.name: + self.name.append(node.name) + super(CheckForStrUnicodeExc, self).generic_visit(node) + self.name = self.name[:-1] + else: + super(CheckForStrUnicodeExc, self).generic_visit(node) + + def visit_Call(self, node): + if self._check_call_names(node, ['str', 'unicode']): + if node not in self.already_checked: + self.already_checked.append(node) + if isinstance(node.args[0], ast.Name): + if node.args[0].id in self.name: + self.add_error(node.args[0]) + super(CheckForStrUnicodeExc, self).generic_visit(node) + + +class CheckForTransAdd(BaseASTChecker): + """Checks for the use of concatenation on a translated string. + + Translations should not be concatenated with other strings, but + should instead include the string being added to the translated + string to give the translators the most information. + """ + + CHECK_DESC = ('B315 Translated messages cannot be concatenated. ' + 'String should be included in translated message.') + + TRANS_FUNC = ['_', '_LI', '_LW', '_LE', '_LC'] + + def visit_BinOp(self, node): + if isinstance(node.op, ast.Add): + if self._check_call_names(node.left, self.TRANS_FUNC): + self.add_error(node.left) + elif self._check_call_names(node.right, self.TRANS_FUNC): + self.add_error(node.right) + super(CheckForTransAdd, self).generic_visit(node) + + +def check_oslo_namespace_imports(logical_line, physical_line, filename): + """'oslo_' should be used instead of 'oslo.' + + B317 + """ + if pep8.noqa(physical_line): + return + if re.match(oslo_namespace_imports, logical_line): + msg = ("B317: '%s' must be used instead of '%s'.") % ( + logical_line.replace('oslo.', 'oslo_'), + logical_line) + yield(0, msg) + + +def dict_constructor_with_list_copy(logical_line): + """Use a dict comprehension instead of a dict constructor + + B318 + """ + msg = ("B318: 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_xrange(logical_line): + """Do not use 'xrange' + + B319 + """ + if assert_no_xrange_re.match(logical_line): + yield(0, "B319: Do not use xrange().") + + +def validate_assertTrue(logical_line): + """Use 'assertTrue' instead of 'assertEqual' + + B312 + """ + if re.match(assert_True, logical_line): + msg = ("B312: Unit tests should use assertTrue(value) instead" + " of using assertEqual(True, value).") + yield(0, msg) + + +def validate_assertIsNone(logical_line): + """Use 'assertIsNone' instead of 'assertEqual' + + B311 + """ + if re.match(assert_None, logical_line): + msg = ("B311: Unit tests should use assertIsNone(value) instead" + " of using assertEqual(None, value).") + yield(0, msg) + + +def no_log_warn_check(logical_line): + """Disallow 'LOG.warn' + + B320 + """ + msg = ("B320: LOG.warn is deprecated, please use LOG.warning!") + if re.match(no_log_warn, logical_line): + yield(0, msg) + + +def validate_assertIsNotNone(logical_line): + """Use 'assertIsNotNone' + + B321 + """ + if re.match(assert_Not_Equal, logical_line) or \ + re.match(assert_Is_Not, logical_line): + msg = ("B321: Unit tests should use assertIsNotNone(value) instead" + " of using assertNotEqual(None, value) or" + " assertIsNot(None, value).") + yield(0, msg) + + +def factory(register): + register(validate_log_translations) + register(no_translate_debug_logs) + register(CheckForStrUnicodeExc) + register(CheckLoggingFormatArgs) + register(CheckForTransAdd) + register(check_oslo_namespace_imports) + register(dict_constructor_with_list_copy) + register(no_xrange) + register(validate_assertTrue) + register(validate_assertIsNone) + register(no_log_warn_check) + register(validate_assertIsNotNone) diff --git a/barbican/tests/test_hacking.py b/barbican/tests/test_hacking.py new file mode 100644 index 00000000..9a85893b --- /dev/null +++ b/barbican/tests/test_hacking.py @@ -0,0 +1,295 @@ +# Copyright 2016 GohighSec +# +# 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 sys +import textwrap + +import ddt +import mock +import pep8 + +from barbican.hacking import checks +from barbican.tests import utils + + +@ddt.ddt +class HackingTestCase(utils.BaseTestCase): + """Hacking test cases + + This class tests the hacking checks in barbican.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_no_translate_debug_logs(self): + self.assertEqual(1, len(list(checks.no_translate_debug_logs( + "LOG.debug(u._('foo'))", "barbican/scheduler/foo.py")))) + + self.assertEqual(0, len(list(checks.no_translate_debug_logs( + "LOG.debug('foo')", "barbican/scheduler/foo.py")))) + + self.assertEqual(0, len(list(checks.no_translate_debug_logs( + "LOG.info(_('foo'))", "barbican/scheduler/foo.py")))) + + # 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_logging_format_no_tuple_arguments(self): + checker = checks.CheckLoggingFormatArgs + code = """ + import logging + LOG = logging.getLogger() + LOG.info("Message without a second argument.") + LOG.critical("Message with %s arguments.", 'two') + LOG.debug("Volume %s caught fire and is at %d degrees C and" + " climbing.", 'volume1', 500) + """ + self._assert_has_no_errors(code, checker) + + @ddt.data(*checks.CheckLoggingFormatArgs.LOG_METHODS) + def test_logging_with_tuple_argument(self, log_method): + checker = checks.CheckLoggingFormatArgs + code = """ + import logging + LOG = logging.getLogger() + LOG.{0}("Volume %s caught fire and is at %d degrees C and " + "climbing.", ('volume1', 500)) + """ + self._assert_has_errors(code.format(log_method), checker, + expected_errors=[(4, 21, 'B310')]) + + def test_str_on_exception(self): + + checker = checks.CheckForStrUnicodeExc + code = """ + def f(a, b): + try: + p = str(a) + str(b) + except ValueError as e: + p = str(e) + return p + """ + errors = [(5, 16, 'B314')] + self._assert_has_errors(code, checker, expected_errors=errors) + + def test_no_str_unicode_on_exception(self): + checker = checks.CheckForStrUnicodeExc + code = """ + def f(a, b): + try: + p = unicode(a) + str(b) + except ValueError as e: + p = e + return p + """ + self._assert_has_no_errors(code, checker) + + def test_unicode_on_exception(self): + checker = checks.CheckForStrUnicodeExc + code = """ + def f(a, b): + try: + p = str(a) + str(b) + except ValueError as e: + p = unicode(e) + return p + """ + errors = [(5, 20, 'B314')] + self._assert_has_errors(code, checker, expected_errors=errors) + + def test_str_on_multiple_exceptions(self): + checker = checks.CheckForStrUnicodeExc + code = """ + def f(a, b): + try: + p = str(a) + str(b) + except ValueError as e: + try: + p = unicode(a) + unicode(b) + except ValueError as ve: + p = str(e) + str(ve) + p = e + return p + """ + errors = [(8, 20, 'B314'), (8, 29, 'B314')] + self._assert_has_errors(code, checker, expected_errors=errors) + + def test_str_unicode_on_multiple_exceptions(self): + checker = checks.CheckForStrUnicodeExc + code = """ + def f(a, b): + try: + p = str(a) + str(b) + except ValueError as e: + try: + p = unicode(a) + unicode(b) + except ValueError as ve: + p = str(e) + unicode(ve) + p = str(e) + return p + """ + errors = [(8, 20, 'B314'), (8, 33, 'B314'), (9, 16, 'B314')] + self._assert_has_errors(code, checker, expected_errors=errors) + + def test_trans_add(self): + + checker = checks.CheckForTransAdd + code = """ + def fake_tran(msg): + return msg + + + _ = fake_tran + _LI = _ + _LW = _ + _LE = _ + _LC = _ + + + def f(a, b): + msg = _('test') + 'add me' + msg = _LI('test') + 'add me' + msg = _LW('test') + 'add me' + msg = _LE('test') + 'add me' + msg = _LC('test') + 'add me' + msg = 'add to me' + _('test') + return msg + """ + + # Python 3.4.0 introduced a change to the column calculation during AST + # parsing. This was reversed in Python 3.4.3, hence the version-based + # expected value calculation. See #1499743 for more background. + if sys.version_info < (3, 4, 0) or sys.version_info >= (3, 4, 3): + errors = [(13, 10, 'B315'), (14, 10, 'B315'), (15, 10, 'B315'), + (16, 10, 'B315'), (17, 10, 'B315'), (18, 24, 'B315')] + else: + errors = [(13, 11, 'B315'), (14, 13, 'B315'), (15, 13, 'B315'), + (16, 13, 'B315'), (17, 13, 'B315'), (18, 25, 'B315')] + self._assert_has_errors(code, checker, expected_errors=errors) + + code = """ + def f(a, b): + msg = 'test' + 'add me' + return msg + """ + errors = [] + self._assert_has_errors(code, checker, expected_errors=errors) + + 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_no_xrange(self): + self.assertEqual(1, len(list(checks.no_xrange("xrange(45)")))) + + self.assertEqual(0, len(list(checks.no_xrange("range(45)")))) + + def test_validate_assertTrue(self): + test_value = True + self.assertEqual(0, len(list(checks.validate_assertTrue( + "assertTrue(True)")))) + self.assertEqual(1, len(list(checks.validate_assertTrue( + "assertEqual(True, %s)" % test_value)))) + + def test_validate_assertIsNone(self): + test_value = None + self.assertEqual(0, len(list(checks.validate_assertIsNone( + "assertIsNone(None)")))) + self.assertEqual(1, len(list(checks.validate_assertIsNone( + "assertEqual(None, %s)" % test_value)))) + + def test_validate_assertIsNotNone(self): + test_value = None + self.assertEqual(0, len(list(checks.validate_assertIsNotNone( + "assertIsNotNone(NotNone)")))) + self.assertEqual(1, len(list(checks.validate_assertIsNotNone( + "assertNotEqual(None, %s)" % test_value)))) + self.assertEqual(1, len(list(checks.validate_assertIsNotNone( + "assertIsNot(None, %s)" % test_value)))) + + def test_no_log_warn_check(self): + self.assertEqual(0, len(list(checks.no_log_warn_check( + "LOG.warning('This should not trigger LOG.warn" + "hacking check.')")))) + self.assertEqual(1, len(list(checks.no_log_warn_check( + "LOG.warn('We should not use LOG.warn')")))) diff --git a/test-requirements.txt b/test-requirements.txt index 98e7c1f7..5311ce8a 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -1,8 +1,12 @@ # The order of packages is significant, because pip processes them in the order # of appearance. Changing the order has an impact on the overall integration # process, which may cause wedges in the gate later. + +# hacking should appear first in case something else depends on pep8 +hacking>=0.12.0,<0.13 # Apache-2.0 + coverage>=4.0 # Apache-2.0 -hacking<0.11,>=0.10.0 +ddt>=1.0.1 # MIT mock>=2.0 # BSD oslotest>=1.10.0 # Apache-2.0 pykmip>=0.5.0 # Apache 2.0 License diff --git a/tox.ini b/tox.ini index f3354194..5639047c 100644 --- a/tox.ini +++ b/tox.ini @@ -92,3 +92,6 @@ commands = sphinx-build -a -E -W -d install-guide/build/doctrees -b html install # separately, outside of the requirements files. deps = bindep commands = bindep test + +[hacking] +local-check-factory = barbican.hacking.checks.factory