openstack/neutron
Code Issues Proposed changes

Switch to neutron-lib hacking factory

Browse Source 
This allows us to kill a bunch of in-tree checks. There are still some
checks that are either not yet in the library, or don't belong there in
the first place, so we still stick to our own factory, just reusing
whatever is there in neutron-lib.

This change skips some new checks. We gotta figure out what to do with
those, that will belong in a separate follow-up.

Change-Id: Ifeb40ec0e0c4ca623b33a6b9f500dec15cec4de0
This commit is contained in:
Ihar Hrachyshka 2017-04-12 16:43:44 -07:00
parent 58cbcc13f7
commit 9c1e48e79d
4 changed files with 22 additions and 205 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
HACKING.rst
View File

@ -8,30 +8,34 @@ Neutron Style Commandments
Neutron Specific Commandments
-----------------------------
- [N319] Validate that debug level logs are not translated
Some rules are enforced by `neutron-lib hacking factory
<https://docs.openstack.org/developer/neutron-lib/usage.html#hacking-checks>`_
while other rules are specific to Neutron repository.
Below you can find a list of checks specific to this repository.
- [N320] Validate that LOG messages, except debug ones, have translations
- [N321] Validate that jsonutils module is used instead of json
- [N322] Detect common errors with assert_called_once_with
- [N324] Prevent use of deprecated contextlib.nested.
- [N325] Python 3: Do not use xrange.
- [N326] Python 3: do not use basestring.
- [N327] Python 3: do not use dict.iteritems.
- [N328] Detect wrong usage with assertEqual
- [N329] Method's default argument shouldn't be mutable
- [N330] Use assertEqual(*empty*, observed) instead of
assertEqual(observed, *empty*)
- [N331] Detect wrong usage with assertTrue(isinstance()).
- [N332] Use assertEqual(expected_http_code, observed_http_code) instead of
assertEqual(observed_http_code, expected_http_code).
- [N333] Validate that LOG.warning is used instead of LOG.warn. The latter
is deprecated.
- [N334] Use unittest2 uniformly across Neutron.
- [N340] Check usage of <module>.i18n (and neutron.i18n)
- [N341] Check usage of _ from python builtins
- [N343] Production code must not import from neutron.tests.*
- [N344] Python 3: Do not use filter(lambda obj: test(obj), data). Replace it
with [obj for obj in data if test(obj)].
- [N345] Detect wrong usage with assertEqual(None, A) and assertEqual(A, None).
.. note::
When adding a new hacking check to this repository or ``neutron-lib``, make
sure its number (Nxxx) doesn't clash with any other check.
.. note::
As you may have noticed, the numbering for Neutron checks has gaps. This is
because some checks were removed or moved to ``neutron-lib``.
Creating Unit Tests
-------------------

117
neutron/hacking/checks.py
View File

@ -61,8 +61,6 @@ def _regex_for_level(level, hint):
}
log_warn = re.compile(
r"(.)*LOG\.(warn)\(\s*('|\"|_)")
unittest_imports_dot = re.compile(r"\bimport[\s]+unittest\b")
unittest_imports_from = re.compile(r"\bfrom[\s]+unittest\b")
filter_match = re.compile(r".*filter\(lambda ")
@ -72,45 +70,6 @@ tests_imports_from1 = re.compile(r"\bfrom[\s]+neutron.tests\b")
tests_imports_from2 = re.compile(r"\bfrom[\s]+neutron[\s]+import[\s]+tests\b")
@flake8ext
def use_jsonutils(logical_line, filename):
"""N321 - Use jsonutils instead of json."""
msg = "N321: jsonutils.%(fun)s must be used instead of json.%(fun)s"
# Some files in the tree are not meant to be run from inside Neutron
# itself, so we should not complain about them not using jsonutils
json_check_skipped_patterns = [
"neutron/plugins/ml2/drivers/openvswitch/agent/xenapi/etc/xapi.d/"
"plugins/netwrap",
]
for pattern in json_check_skipped_patterns:
if pattern in filename:
return
if "json." in logical_line:
json_funcs = ['dumps(', 'dump(', 'loads(', 'load(']
for f in json_funcs:
pos = logical_line.find('json.%s' % f)
if pos != -1:
yield (pos, msg % {'fun': f[:-1]})
@flake8ext
def no_translate_debug_logs(logical_line, filename):
"""N319 - Check for 'LOG.debug(_(' and 'LOG.debug(_Lx('
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.
"""
for hint in _all_hints:
if logical_line.startswith("LOG.debug(%s(" % hint):
yield(0, "N319 Don't translate debug level logs")
@flake8ext
def check_assert_called_once_with(logical_line, filename):
"""N322 - Try to detect unintended calls of nonexistent mock methods like:
@ -136,43 +95,6 @@ def check_assert_called_once_with(logical_line, filename):
yield (0, msg)
@flake8ext
def check_no_contextlib_nested(logical_line, filename):
"""N324 - Don't use contextlib.nested."""
msg = ("N324: contextlib.nested is deprecated. With Python 2.7 and later "
"the with-statement supports multiple nested objects. See https://"
"docs.python.org/2/library/contextlib.html#contextlib.nested for "
"more information.")
if checks.contextlib_nested.match(logical_line):
yield(0, msg)
@flake8ext
def check_python3_xrange(logical_line):
"""N325 - Do not use xrange."""
if re.search(r"\bxrange\s*\(", logical_line):
yield(0, "N325: Do not use xrange. Use range, or six.moves.range for "
"large loops.")
@flake8ext
def check_no_basestring(logical_line):
"""N326 - Don't use basestring."""
if re.search(r"\bbasestring\b", logical_line):
msg = ("N326: basestring is not Python3-compatible, use "
"six.string_types instead.")
yield(0, msg)
@flake8ext
def check_python3_no_iteritems(logical_line):
"""N327 - Use six.iteritems()"""
if re.search(r".*\.iteritems\(\)", logical_line):
msg = ("N327: Use six.iteritems() instead of dict.iteritems().")
yield(0, msg)
@flake8ext
def check_asserttruefalse(logical_line, filename):
"""N328 - Don't use assertEqual(True/False, observed)."""
@ -195,14 +117,6 @@ def check_asserttruefalse(logical_line, filename):
yield (0, msg)
@flake8ext
def no_mutable_default_args(logical_line):
"""N329 - Don't use mutable default arguments."""
msg = "N329: Method's default argument shouldn't be mutable!"
if checks.mutable_default_args.match(logical_line):
yield (0, msg)
@flake8ext
def check_assertempty(logical_line, filename):
"""N330 - Enforce using assertEqual parameter ordering in case of empty
@ -240,14 +154,6 @@ def check_assertequal_for_httpcode(logical_line, filename):
yield (0, msg)
@flake8ext
def check_log_warn_deprecated(logical_line, filename):
"""N333 - Use LOG.warning."""
msg = "N333: Use LOG.warning due to compatibility with py3"
if log_warn.match(logical_line):
yield (0, msg)
@flake8ext
def check_oslo_i18n_wrapper(logical_line, filename, noqa):
"""N340 - Check for neutron.i18n usage.
@ -356,19 +262,6 @@ def check_python3_no_filter(logical_line):
yield(0, msg)
@flake8ext
def check_assertIsNone(logical_line, filename):
"""N345 - Enforce using assertIsNone."""
if 'neutron/tests/' in filename:
asse_eq_end_with_none_re = re.compile(r"assertEqual\(.*?,\s+None\)$")
asse_eq_start_with_none_re = re.compile(r"assertEqual\(None,")
res = (asse_eq_start_with_none_re.search(logical_line) or
asse_eq_end_with_none_re.search(logical_line))
if res:
yield (0, "N345: assertEqual(A, None) or assertEqual(None, A) "
"sentences not allowed")
@flake8ext
def check_no_sqlalchemy_event_import(logical_line, filename, noqa):
"""N346 - Use neutron.db.api.sqla_listen instead of sqlalchemy event."""
@ -387,23 +280,15 @@ def check_no_sqlalchemy_event_import(logical_line, filename, noqa):
def factory(register):
register(use_jsonutils)
checks.factory(register)
register(check_assert_called_once_with)
register(no_translate_debug_logs)
register(check_no_contextlib_nested)
register(check_python3_xrange)
register(check_no_basestring)
register(check_python3_no_iteritems)
register(check_asserttruefalse)
register(no_mutable_default_args)
register(check_assertempty)
register(check_assertisinstance)
register(check_assertequal_for_httpcode)
register(check_log_warn_deprecated)
register(check_oslo_i18n_wrapper)
register(check_builtins_gettext)
register(check_unittest_imports)
register(check_no_imports_from_tests)
register(check_python3_no_filter)
register(check_assertIsNone)
register(check_no_sqlalchemy_event_import)

78
neutron/tests/unit/hacking/test_checks.py
View File

@ -36,38 +36,6 @@ class HackingTestCase(base.BaseTestCase):
def assertLineFails(self, func, line):
self.assertIsInstance(next(func(line)), tuple)
def test_no_translate_debug_logs(self):
for hint in checks._all_hints:
bad = "LOG.debug(%s('bad'))" % hint
self.assertEqual(
1, len(list(checks.no_translate_debug_logs(bad, 'f'))))
def test_use_jsonutils(self):
def __get_msg(fun):
msg = ("N321: jsonutils.%(fun)s must be used instead of "
"json.%(fun)s" % {'fun': fun})
return [(0, msg)]
for method in ('dump', 'dumps', 'load', 'loads'):
self.assertEqual(
__get_msg(method),
list(checks.use_jsonutils("json.%s(" % method,
"./neutron/common/rpc.py")))
self.assertEqual(0,
len(list(checks.use_jsonutils("jsonx.%s(" % method,
"./neutron/common/rpc.py"))))
self.assertEqual(0,
len(list(checks.use_jsonutils("json.%sx(" % method,
"./neutron/common/rpc.py"))))
self.assertEqual(0,
len(list(checks.use_jsonutils(
"json.%s" % method,
"./neutron/plugins/ml2/drivers/openvswitch/agent/xenapi/"
"etc/xapi.d/plugins/netwrap"))))
def test_assert_called_once_with(self):
fail_code1 = """
mock = Mock()
@ -118,36 +86,6 @@ class HackingTestCase(base.BaseTestCase):
0, len(list(checks.check_assert_called_once_with(pass_code2,
"neutron/tests/test_assert.py"))))
def test_check_python3_xrange(self):
f = checks.check_python3_xrange
self.assertLineFails(f, 'a = xrange(1000)')
self.assertLineFails(f, 'b =xrange ( 42 )')
self.assertLineFails(f, 'c = xrange(1, 10, 2)')
self.assertLinePasses(f, 'd = range(1000)')
self.assertLinePasses(f, 'e = six.moves.range(1337)')
def test_no_basestring(self):
self.assertEqual(1,
len(list(checks.check_no_basestring("isinstance(x, basestring)"))))
def test_check_python3_iteritems(self):
f = checks.check_python3_no_iteritems
self.assertLineFails(f, "d.iteritems()")
self.assertLinePasses(f, "six.iteritems(d)")
def test_no_mutable_default_args(self):
self.assertEqual(1, len(list(checks.no_mutable_default_args(
" def fake_suds_context(calls={}):"))))
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 = [], {}"))))
def test_asserttruefalse(self):
true_fail_code1 = """
test_bool = True
@ -268,11 +206,6 @@ class HackingTestCase(base.BaseTestCase):
self.assertLineFails(f, 'from unittest.TestSuite')
self.assertLineFails(f, 'import unittest')
def test_check_log_warn_deprecated(self):
bad = "LOG.warn(_LW('i am zlatan!'))"
self.assertEqual(
1, len(list(checks.check_log_warn_deprecated(bad, 'f'))))
def test_check_no_imports_from_tests(self):
fail_codes = ('from neutron import tests',
'from neutron.tests import base',
@ -294,16 +227,6 @@ class HackingTestCase(base.BaseTestCase):
self.assertLinePasses(f, "filter(function, range(0,10))")
self.assertLinePasses(f, "lambda x, y: x+y")
def test_check_assertIsNone(self):
self.assertEqual(1, len(list(checks.check_assertIsNone(
"self.assertEqual(A, None)", "neutron/tests/test_assert.py"))))
self.assertEqual(1, len(list(checks.check_assertIsNone(
"self.assertEqual(None, A)", "neutron/tests/test_assert.py"))))
self.assertEqual(0, len(list(checks.check_assertIsNone(
"self.assertIsNone()", "neutron/tests/test_assert.py"))))
# The following is borrowed from hacking/tests/test_doctest.py.
# Tests defined in docstring is easier to understand
@ -332,6 +255,7 @@ class HackingDocTestCase(hacking_doctest.HackingTestCase):
if self.options.select:
turn_on.update(self.options.select)
self.options.select = tuple(turn_on)
self.options.ignore = ('N530',)
report = pep8.BaseReport(self.options)
checker = pep8.Checker(filename=self.filename, lines=self.lines,

8
tox.ini
View File

@ -119,11 +119,15 @@ commands = sphinx-build -W -b html doc/source doc/build/html
# E265 block comment should start with '# '
# H404 multi line docstring should start with a summary
# H405 multi line docstring summary not separated with an empty line
ignore = E125,E126,E128,E129,E265,H404,H405
# N530 direct neutron imports not allowed
# N534 Untranslated exception message
# N536 Use assertIsNone rather than assertEqual to check for None values
# TODO(ihrachys) figure out what to do with N534 and N536
ignore = E125,E126,E128,E129,E265,H404,H405,N530,N534,N536
# H904: Delay string interpolations at logging calls
enable-extensions=H904
show-source = true
exclude = ./.*,build,dist
exclude = ./.*,build,dist,doc
[hacking]
import_exceptions = neutron._i18n