Enable global hacking checks and removed local checks
In this patchset, I set up 'enabled-extensions' in flake8 section of tox.ini. In newer hacking versions, we can enable some of the non-default hacking rules (one by one), which are disabled by default and implemented in the newer versions of hacking. The enabled extensions are the following: * [H106] Don’t put vim configuration in source files (off by default). * [H203] Use assertIs(Not)None to check for None (off by default). * [H904] Delay string interpolations at logging calls (off by default). Together with enabling these rules, I also removed the locally implemented versions of them. Due to limitations of local checking engine, there were some places in the tests, where pep8 failed. In this patchset, these codes are also fixed. Change-Id: I3a9d2dc007a269cdb2cad441e22f5eb9b58ce0a0
This commit is contained in:
Gábor Antal
parent
d02d4b3f1b
commit
51c6b13847
@ -21,13 +21,10 @@ Nova Specific Commandments
|
||||
should be moved into a common module
|
||||
- [N313] capitalize help string
|
||||
Config parameter help strings should have a capitalized first letter
|
||||
- [N314] vim configuration should not be kept in source files.
|
||||
- [N316] Change assertTrue(isinstance(A, B)) by optimal assert like
|
||||
assertIsInstance(A, B).
|
||||
- [N317] Change assertEqual(type(A), B) by optimal assert like
|
||||
assertIsInstance(A, B)
|
||||
- [N318] Change assertEqual(A, None) or assertIs(A, None) to optimal assert
|
||||
like assertIsNone(A)
|
||||
- [N319] Validate that debug level logs are not translated.
|
||||
- [N320] Setting CONF.* attributes directly in tests is forbidden. Use
|
||||
self.flags(option=value) instead.
|
||||
@ -64,7 +61,6 @@ Nova Specific Commandments
|
||||
- [N351] Do not use the oslo_policy.policy.Enforcer.enforce() method.
|
||||
- [N352] LOG.warn is deprecated. Enforce use of LOG.warning.
|
||||
- [N353] Validate that context objects is not passed in logging calls.
|
||||
- [N354] String interpolation should be delayed at logging calls.
|
||||
- [N355] Enforce use of assertTrue/assertFalse
|
||||
- [N356] Enforce use of assertIs/assertIsNot
|
||||
- [N357] Use oslo_utils.uuidutils or uuidsentinel(in case of test cases) to
|
||||
|
||||
@ -42,7 +42,6 @@ cfg_re = re.compile(r".*\scfg\.")
|
||||
cfg_opt_re = re.compile(r".*[\s\[]cfg\.[a-zA-Z]*Opt\(")
|
||||
rule_default_re = re.compile(r".*RuleDefault\(")
|
||||
policy_enforce_re = re.compile(r".*_ENFORCER\.enforce\(")
|
||||
vi_header_re = re.compile(r"^#\s+vim?:.+")
|
||||
virt_file_re = re.compile(r"\./nova/(?:tests/)?virt/(\w+)/")
|
||||
virt_import_re = re.compile(
|
||||
r"^\s*(?:import|from) nova\.(?:tests\.)?virt\.(\w+)")
|
||||
@ -105,9 +104,6 @@ doubled_words_re = re.compile(
|
||||
r"\b(then?|[iao]n|i[fst]|but|f?or|at|and|[dt]o)\s+\1\b")
|
||||
log_remove_context = re.compile(
|
||||
r"(.)*LOG\.(.*)\(.*(context=[_a-zA-Z0-9].*)+.*\)")
|
||||
log_string_interpolation = re.compile(r".*LOG\.(error|warning|info"
|
||||
r"|critical|exception|debug)"
|
||||
r"\([^,]*%[^,]*[,)]")
|
||||
return_not_followed_by_space = re.compile(r"^\s*return(?:\(|{|\"|'|#).*$")
|
||||
|
||||
|
||||
@ -248,20 +244,6 @@ def capital_cfg_help(logical_line, tokens):
|
||||
yield(0, msg)
|
||||
|
||||
|
||||
def no_vi_headers(physical_line, line_number, lines):
|
||||
"""Check for vi editor configuration in source files.
|
||||
|
||||
By default vi modelines can only appear in the first or
|
||||
last 5 lines of a source file.
|
||||
|
||||
N314
|
||||
"""
|
||||
# NOTE(gilliard): line_number is 1-indexed
|
||||
if line_number <= 5 or line_number > len(lines) - 5:
|
||||
if vi_header_re.match(physical_line):
|
||||
return 0, "N314: Don't put vi configuration in source files"
|
||||
|
||||
|
||||
def assert_true_instance(logical_line):
|
||||
"""Check for assertTrue(isinstance(a, b)) sentences
|
||||
|
||||
@ -280,27 +262,6 @@ def assert_equal_type(logical_line):
|
||||
yield (0, "N317: assertEqual(type(A), B) sentences not allowed")
|
||||
|
||||
|
||||
def assert_equal_none(logical_line):
|
||||
"""Check for assertEqual(A, None) or assertEqual(None, A) sentences
|
||||
|
||||
N318
|
||||
"""
|
||||
_start_re = re.compile(r"assertEqual\(.*?,\s+None\)$")
|
||||
_end_re = re.compile(r"assertEqual\(None,")
|
||||
|
||||
if _start_re.search(logical_line) or _end_re.search(logical_line):
|
||||
yield (0, "N318: assertEqual(A, None) or assertEqual(None, A) "
|
||||
"sentences not allowed. Use assertIsNone(A) instead.")
|
||||
|
||||
_start_re = re.compile(r"assertIs(Not)?\(None,")
|
||||
_end_re = re.compile(r"assertIs(Not)?\(.*,\s+None\)$")
|
||||
|
||||
if _start_re.search(logical_line) or _end_re.search(logical_line):
|
||||
yield (0, "N318: assertIsNot(A, None) or assertIsNot(None, A) must "
|
||||
"not be used. Use assertIsNone(A) or assertIsNotNone(A) "
|
||||
"instead.")
|
||||
|
||||
|
||||
def check_python3_xrange(logical_line):
|
||||
if re.search(r"\bxrange\s*\(", logical_line):
|
||||
yield(0, "N327: Do not use xrange(). 'xrange()' is not compatible "
|
||||
@ -803,28 +764,6 @@ def check_context_log(logical_line, physical_line, filename):
|
||||
"kwarg.")
|
||||
|
||||
|
||||
def check_delayed_string_interpolation(logical_line, physical_line, filename):
|
||||
"""Check whether string interpolation is delayed at logging calls
|
||||
|
||||
Not correct: LOG.debug('Example: %s' % 'bad')
|
||||
Correct: LOG.debug('Example: %s', 'good')
|
||||
|
||||
N354
|
||||
"""
|
||||
if "nova/tests" in filename:
|
||||
return
|
||||
|
||||
if pep8.noqa(physical_line):
|
||||
return
|
||||
|
||||
if log_string_interpolation.match(logical_line):
|
||||
yield(logical_line.index('%'),
|
||||
"N354: String interpolation should be delayed to be "
|
||||
"handled by the logging code, rather than being done "
|
||||
"at the point of the logging call. "
|
||||
"Use ',' instead of '%'.")
|
||||
|
||||
|
||||
def no_assert_equal_true_false(logical_line):
|
||||
"""Enforce use of assertTrue/assertFalse.
|
||||
|
||||
@ -901,11 +840,9 @@ def factory(register):
|
||||
register(import_no_virt_driver_import_deps)
|
||||
register(import_no_virt_driver_config_deps)
|
||||
register(capital_cfg_help)
|
||||
register(no_vi_headers)
|
||||
register(no_import_translation_in_tests)
|
||||
register(assert_true_instance)
|
||||
register(assert_equal_type)
|
||||
register(assert_equal_none)
|
||||
register(assert_raises_regexp)
|
||||
register(no_translate_debug_logs)
|
||||
register(no_setting_conf_directly_in_tests)
|
||||
@ -934,7 +871,6 @@ def factory(register):
|
||||
register(no_log_warn)
|
||||
register(CheckForUncalledTestClosure)
|
||||
register(check_context_log)
|
||||
register(check_delayed_string_interpolation)
|
||||
register(no_assert_equal_true_false)
|
||||
register(no_assert_true_false_is_not)
|
||||
register(check_uuid4)
|
||||
|
||||
@ -57,7 +57,7 @@ def generate_new_element(items, prefix, numeric=False):
|
||||
candidate = prefix + generate_random_alphanumeric(8)
|
||||
if candidate not in items:
|
||||
return candidate
|
||||
LOG.debug("Random collision on %s" % candidate)
|
||||
LOG.debug("Random collision on %s", candidate)
|
||||
|
||||
|
||||
class _IntegratedTestBase(test.TestCase):
|
||||
@ -143,7 +143,7 @@ class _IntegratedTestBase(test.TestCase):
|
||||
|
||||
# Set a valid flavorId
|
||||
flavor = self.api.get_flavors()[0]
|
||||
LOG.debug("Using flavor: %s" % flavor)
|
||||
LOG.debug("Using flavor: %s", flavor)
|
||||
server[self._flavor_ref_parameter] = ('http://fake.server/%s'
|
||||
% flavor['id'])
|
||||
|
||||
@ -184,14 +184,14 @@ class _IntegratedTestBase(test.TestCase):
|
||||
def _build_server(self, flavor_id):
|
||||
server = {}
|
||||
image = self.api.get_images()[0]
|
||||
LOG.debug("Image: %s" % image)
|
||||
LOG.debug("Image: %s", image)
|
||||
|
||||
# We now have a valid imageId
|
||||
server[self._image_ref_parameter] = image['id']
|
||||
|
||||
# Set a valid flavorId
|
||||
flavor = self.api.get_flavor(flavor_id)
|
||||
LOG.debug("Using flavor: %s" % flavor)
|
||||
LOG.debug("Using flavor: %s", flavor)
|
||||
server[self._flavor_ref_parameter] = ('http://fake.server/%s'
|
||||
% flavor['id'])
|
||||
|
||||
|
||||
@ -78,7 +78,7 @@ class NUMAServersTest(ServersTestBase):
|
||||
post = {'server': good_server}
|
||||
|
||||
created_server = self.api.post_server(post)
|
||||
LOG.debug("created_server: %s" % created_server)
|
||||
LOG.debug("created_server: %s", created_server)
|
||||
self.assertTrue(created_server['id'])
|
||||
created_server_id = created_server['id']
|
||||
|
||||
|
||||
@ -105,7 +105,7 @@ class SRIOVServersTest(ServersTestBase):
|
||||
post = {'server': good_server}
|
||||
|
||||
created_server = self.api.post_server(post)
|
||||
LOG.debug("created_server: %s" % created_server)
|
||||
LOG.debug("created_server: %s", created_server)
|
||||
self.assertTrue(created_server['id'])
|
||||
created_server_id = created_server['id']
|
||||
|
||||
|
||||
@ -78,7 +78,7 @@ class ServersTestBase(integrated_helpers._IntegratedTestBase):
|
||||
LOG.debug("Got 404, proceeding")
|
||||
break
|
||||
|
||||
LOG.debug("Found_server=%s" % found_server)
|
||||
LOG.debug("Found_server=%s", found_server)
|
||||
|
||||
# TODO(justinsb): Mock doesn't yet do accurate state changes
|
||||
# if found_server['status'] != 'deleting':
|
||||
@ -109,7 +109,7 @@ class ServersTest(ServersTestBase):
|
||||
# Simple check that listing servers works.
|
||||
servers = self.api.get_servers()
|
||||
for server in servers:
|
||||
LOG.debug("server: %s" % server)
|
||||
LOG.debug("server: %s", server)
|
||||
|
||||
def test_create_server_with_error(self):
|
||||
# Create a server which will enter error state.
|
||||
@ -174,7 +174,7 @@ class ServersTest(ServersTestBase):
|
||||
server['name'] = good_server['name']
|
||||
|
||||
created_server = self.api.post_server(post)
|
||||
LOG.debug("created_server: %s" % created_server)
|
||||
LOG.debug("created_server: %s", created_server)
|
||||
self.assertTrue(created_server['id'])
|
||||
created_server_id = created_server['id']
|
||||
|
||||
@ -215,7 +215,7 @@ class ServersTest(ServersTestBase):
|
||||
server = self._build_minimal_create_server_request()
|
||||
|
||||
created_server = self.api.post_server({'server': server})
|
||||
LOG.debug("created_server: %s" % created_server)
|
||||
LOG.debug("created_server: %s", created_server)
|
||||
self.assertTrue(created_server['id'])
|
||||
created_server_id = created_server['id']
|
||||
|
||||
@ -250,7 +250,7 @@ class ServersTest(ServersTestBase):
|
||||
server = self._build_minimal_create_server_request()
|
||||
|
||||
created_server = self.api.post_server({'server': server})
|
||||
LOG.debug("created_server: %s" % created_server)
|
||||
LOG.debug("created_server: %s", created_server)
|
||||
self.assertTrue(created_server['id'])
|
||||
created_server_id = created_server['id']
|
||||
|
||||
@ -282,7 +282,7 @@ class ServersTest(ServersTestBase):
|
||||
server = self._build_minimal_create_server_request()
|
||||
|
||||
created_server = self.api.post_server({'server': server})
|
||||
LOG.debug("created_server: %s" % created_server)
|
||||
LOG.debug("created_server: %s", created_server)
|
||||
self.assertTrue(created_server['id'])
|
||||
created_server_id = created_server['id']
|
||||
|
||||
@ -320,7 +320,7 @@ class ServersTest(ServersTestBase):
|
||||
|
||||
post = {'server': server}
|
||||
created_server = self.api.post_server(post)
|
||||
LOG.debug("created_server: %s" % created_server)
|
||||
LOG.debug("created_server: %s", created_server)
|
||||
self.assertTrue(created_server['id'])
|
||||
created_server_id = created_server['id']
|
||||
|
||||
@ -402,7 +402,7 @@ class ServersTest(ServersTestBase):
|
||||
server_post['server']['metadata'] = metadata
|
||||
|
||||
created_server = self.api.post_server(server_post)
|
||||
LOG.debug("created_server: %s" % created_server)
|
||||
LOG.debug("created_server: %s", created_server)
|
||||
self.assertTrue(created_server['id'])
|
||||
created_server_id = created_server['id']
|
||||
|
||||
@ -420,7 +420,7 @@ class ServersTest(ServersTestBase):
|
||||
post['rebuild'].update(self._get_access_ips_params())
|
||||
|
||||
self.api.post_server_action(created_server_id, post)
|
||||
LOG.debug("rebuilt server: %s" % created_server)
|
||||
LOG.debug("rebuilt server: %s", created_server)
|
||||
self.assertTrue(created_server['id'])
|
||||
|
||||
found_server = self.api.get_server(created_server_id)
|
||||
@ -439,7 +439,7 @@ class ServersTest(ServersTestBase):
|
||||
}
|
||||
|
||||
self.api.post_server_action(created_server_id, post)
|
||||
LOG.debug("rebuilt server: %s" % created_server)
|
||||
LOG.debug("rebuilt server: %s", created_server)
|
||||
self.assertTrue(created_server['id'])
|
||||
|
||||
found_server = self.api.get_server(created_server_id)
|
||||
@ -459,7 +459,7 @@ class ServersTest(ServersTestBase):
|
||||
# Create a server
|
||||
server = self._build_minimal_create_server_request()
|
||||
created_server = self.api.post_server({'server': server})
|
||||
LOG.debug("created_server: %s" % created_server)
|
||||
LOG.debug("created_server: %s", created_server)
|
||||
server_id = created_server['id']
|
||||
self.assertTrue(server_id)
|
||||
|
||||
@ -534,7 +534,7 @@ class ServersTest(ServersTestBase):
|
||||
post = {'server': server}
|
||||
|
||||
created_server = self.api.post_server(post)
|
||||
LOG.debug("created_server: %s" % created_server)
|
||||
LOG.debug("created_server: %s", created_server)
|
||||
self.assertTrue(created_server['id'])
|
||||
created_server_id = created_server['id']
|
||||
|
||||
|
||||
@ -62,7 +62,7 @@ class SecgroupsFullstack(testscenarios.WithScenarios, test.TestCase):
|
||||
server = {}
|
||||
|
||||
image = self.api.get_images()[0]
|
||||
LOG.info("Image: %s" % image)
|
||||
LOG.info("Image: %s", image)
|
||||
|
||||
if self._image_ref_parameter in image:
|
||||
image_href = image[self._image_ref_parameter]
|
||||
|
||||
@ -3510,7 +3510,7 @@ class LdapDNSTestCase(test.NoDBTestCase):
|
||||
|
||||
self.driver.delete_entry(name1, domain1)
|
||||
entries = self.driver.get_entries_by_address(address1, domain1)
|
||||
LOG.debug("entries: %s" % entries)
|
||||
LOG.debug("entries: %s", entries)
|
||||
self.assertEqual(1, len(entries))
|
||||
self.assertEqual(name2, entries[0])
|
||||
|
||||
|
||||
@ -82,30 +82,6 @@ class HackingTestCase(test.NoDBTestCase):
|
||||
"'nova.virt.libvirt.driver', group='libvirt')",
|
||||
"./nova/virt/libvirt/volume.py"))
|
||||
|
||||
def test_no_vi_headers(self):
|
||||
|
||||
lines = ['Line 1\n', 'Line 2\n', 'Line 3\n', 'Line 4\n', 'Line 5\n',
|
||||
'Line 6\n', 'Line 7\n', 'Line 8\n', 'Line 9\n', 'Line 10\n',
|
||||
'Line 11\n', 'Line 12\n', 'Line 13\n', 'Line14\n', 'Line15\n']
|
||||
|
||||
self.assertIsNone(checks.no_vi_headers(
|
||||
"Test string foo", 1, lines))
|
||||
self.assertEqual(len(list(checks.no_vi_headers(
|
||||
"# vim: et tabstop=4 shiftwidth=4 softtabstop=4",
|
||||
2, lines))), 2)
|
||||
self.assertIsNone(checks.no_vi_headers(
|
||||
"# vim: et tabstop=4 shiftwidth=4 softtabstop=4",
|
||||
6, lines))
|
||||
self.assertIsNone(checks.no_vi_headers(
|
||||
"# vim: et tabstop=4 shiftwidth=4 softtabstop=4",
|
||||
9, lines))
|
||||
self.assertEqual(len(list(checks.no_vi_headers(
|
||||
"# vim: et tabstop=4 shiftwidth=4 softtabstop=4",
|
||||
14, lines))), 2)
|
||||
self.assertIsNone(checks.no_vi_headers(
|
||||
"Test end string for vi",
|
||||
15, lines))
|
||||
|
||||
def test_assert_true_instance(self):
|
||||
self.assertEqual(len(list(checks.assert_true_instance(
|
||||
"self.assertTrue(isinstance(e, "
|
||||
@ -158,28 +134,6 @@ class HackingTestCase(test.NoDBTestCase):
|
||||
self.assertEqual(len(list(checks.assert_equal_in(
|
||||
"self.assertEqual(False, any(a==1 for a in b))"))), 0)
|
||||
|
||||
def test_assert_equal_none(self):
|
||||
self.assertEqual(len(list(checks.assert_equal_none(
|
||||
"self.assertEqual(A, None)"))), 1)
|
||||
|
||||
self.assertEqual(len(list(checks.assert_equal_none(
|
||||
"self.assertEqual(None, A)"))), 1)
|
||||
|
||||
self.assertEqual(
|
||||
len(list(checks.assert_equal_none("self.assertIsNone()"))), 0)
|
||||
|
||||
self.assertEqual(len(list(checks.assert_equal_none(
|
||||
"self.assertIs(A, None)"))), 1)
|
||||
|
||||
self.assertEqual(len(list(checks.assert_equal_none(
|
||||
"self.assertIsNot(A, None)"))), 1)
|
||||
|
||||
self.assertEqual(len(list(checks.assert_equal_none(
|
||||
"self.assertIs(None, A)"))), 1)
|
||||
|
||||
self.assertEqual(len(list(checks.assert_equal_none(
|
||||
"self.assertIsNot(None, A)"))), 1)
|
||||
|
||||
def test_assert_true_or_false_with_in_or_not_in(self):
|
||||
self.assertEqual(len(list(checks.assert_true_or_false_with_in(
|
||||
"self.assertTrue(A in B)"))), 1)
|
||||
@ -793,57 +747,6 @@ class HackingTestCase(test.NoDBTestCase):
|
||||
"""
|
||||
self._assert_has_no_errors(code, checks.check_context_log)
|
||||
|
||||
def test_check_delayed_string_interpolation(self):
|
||||
checker = checks.check_delayed_string_interpolation
|
||||
code = """
|
||||
msg_w = _LW('Test string (%s)')
|
||||
msg_i = _LI('Test string (%s)')
|
||||
value = 'test'
|
||||
|
||||
LOG.error(_LE("Test string (%s)") % value)
|
||||
LOG.warning(msg_w % 'test%string')
|
||||
LOG.info(msg_i %
|
||||
"test%string%info")
|
||||
LOG.critical(
|
||||
_LC('Test string (%s)') % value,
|
||||
instance=instance)
|
||||
LOG.exception(_LE(" 'Test quotation %s' \"Test\"") % 'test')
|
||||
LOG.debug(' "Test quotation %s" \'Test\'' % "test")
|
||||
LOG.debug('Tesing %(test)s' %
|
||||
{'test': ','.join(
|
||||
['%s=%s' % (name, value)
|
||||
for name, value in test.items()])})
|
||||
"""
|
||||
|
||||
expected_errors = [(5, 34, 'N354'), (6, 18, 'N354'), (7, 15, 'N354'),
|
||||
(10, 28, 'N354'), (12, 49, 'N354'),
|
||||
(13, 40, 'N354'), (14, 28, 'N354')]
|
||||
self._assert_has_errors(code, checker, expected_errors=expected_errors)
|
||||
self._assert_has_no_errors(code, checker,
|
||||
filename='nova/tests/unit/test_hacking.py')
|
||||
|
||||
code = """
|
||||
msg_w = _LW('Test string (%s)')
|
||||
msg_i = _LI('Test string (%s)')
|
||||
value = 'test'
|
||||
|
||||
LOG.error(_LE("Test string (%s)"), value)
|
||||
LOG.error(_LE("Test string (%s)") % value) # noqa
|
||||
LOG.warning(msg_w, 'test%string')
|
||||
LOG.info(msg_i,
|
||||
"test%string%info")
|
||||
LOG.critical(
|
||||
_LC('Test string (%s)'), value,
|
||||
instance=instance)
|
||||
LOG.exception(_LE(" 'Test quotation %s' \"Test\""), 'test')
|
||||
LOG.debug(' "Test quotation %s" \'Test\'', "test")
|
||||
LOG.debug('Tesing %(test)s',
|
||||
{'test': ','.join(
|
||||
['%s=%s' % (name, value)
|
||||
for name, value in test.items()])})
|
||||
"""
|
||||
self._assert_has_no_errors(code, checker)
|
||||
|
||||
def test_no_assert_equal_true_false(self):
|
||||
code = """
|
||||
self.assertEqual(context_is_admin, True)
|
||||
|
||||
@ -82,7 +82,7 @@ class FakeLibvirtTests(test.NoDBTestCase):
|
||||
def test_openAuth_accepts_None_uri_by_default(self):
|
||||
conn_method = self.get_openAuth_curry_func()
|
||||
conn = conn_method(None)
|
||||
self.assertNotEqual(conn, None, "Connecting to fake libvirt failed")
|
||||
self.assertIsNotNone(conn, "Connecting to fake libvirt failed")
|
||||
|
||||
def test_openAuth_can_refuse_None_uri(self):
|
||||
conn_method = self.get_openAuth_curry_func()
|
||||
|
||||
@ -60,7 +60,7 @@ def catch_notimplementederror(f):
|
||||
except NotImplementedError:
|
||||
frame = traceback.extract_tb(sys.exc_info()[2])[-1]
|
||||
LOG.error("%(driver)s does not implement %(method)s "
|
||||
"required for test %(test)s" %
|
||||
"required for test %(test)s",
|
||||
{'driver': type(self.connection),
|
||||
'method': frame[2], 'test': f.__name__})
|
||||
|
||||
|
||||
1
tox.ini
1
tox.ini
@ -161,6 +161,7 @@ commands = bash -c tools/releasenotes_tox.sh
|
||||
#
|
||||
# E251 Skipped due to https://github.com/jcrocholl/pep8/issues/301
|
||||
|
||||
enable-extensions = H106,H203,H904
|
||||
ignore = E121,E122,E123,E124,E125,E126,E127,E128,E129,E131,E251,H405
|
||||
exclude = .venv,.git,.tox,dist,doc,*lib/python*,*egg,build,tools/xenserver*,releasenotes
|
||||
# To get a list of functions that are more complex than 25, set max-complexity
|
||||
|
||||
Loading…
Reference in New Issue
Block a user