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
This commit is contained in:
parent
3b8aafb735
commit
dea8754c6d
92
HACKING.rst
92
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
|
||||
|
0
barbican/hacking/__init__.py
Normal file
0
barbican/hacking/__init__.py
Normal file
379
barbican/hacking/checks.py
Normal file
379
barbican/hacking/checks.py
Normal file
@ -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)
|
295
barbican/tests/test_hacking.py
Normal file
295
barbican/tests/test_hacking.py
Normal file
@ -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')"))))
|
@ -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
|
||||
|
Loading…
Reference in New Issue
Block a user