From 51c6b1384760e7d07da5997b2afdbc292e1f637c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Antal?= Date: Wed, 8 Feb 2017 16:41:31 +0100 Subject: [PATCH] Enable global hacking checks and removed local checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- HACKING.rst | 4 - nova/hacking/checks.py | 64 ------------ nova/tests/functional/integrated_helpers.py | 8 +- .../functional/libvirt/test_numa_servers.py | 2 +- .../libvirt/test_pci_sriov_servers.py | 2 +- nova/tests/functional/test_servers.py | 24 ++--- nova/tests/functional/wsgi/test_secgroup.py | 2 +- nova/tests/unit/network/test_manager.py | 2 +- nova/tests/unit/test_hacking.py | 97 ------------------- .../unit/virt/libvirt/test_fakelibvirt.py | 2 +- nova/tests/unit/virt/test_virt_drivers.py | 2 +- tox.ini | 1 + 12 files changed, 23 insertions(+), 187 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 1161a350cb3f..f0c4ccfbd852 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -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 diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 144ad5d2a92f..b4a477adcadc 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -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) diff --git a/nova/tests/functional/integrated_helpers.py b/nova/tests/functional/integrated_helpers.py index 192b465592c0..bd123d89453e 100644 --- a/nova/tests/functional/integrated_helpers.py +++ b/nova/tests/functional/integrated_helpers.py @@ -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']) diff --git a/nova/tests/functional/libvirt/test_numa_servers.py b/nova/tests/functional/libvirt/test_numa_servers.py index 54b41c6c60e5..e94a279a29b0 100644 --- a/nova/tests/functional/libvirt/test_numa_servers.py +++ b/nova/tests/functional/libvirt/test_numa_servers.py @@ -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'] diff --git a/nova/tests/functional/libvirt/test_pci_sriov_servers.py b/nova/tests/functional/libvirt/test_pci_sriov_servers.py index 4f62f7d82cbe..6bf8c2377e49 100644 --- a/nova/tests/functional/libvirt/test_pci_sriov_servers.py +++ b/nova/tests/functional/libvirt/test_pci_sriov_servers.py @@ -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'] diff --git a/nova/tests/functional/test_servers.py b/nova/tests/functional/test_servers.py index 0f3df6b12fff..107dcf562a27 100644 --- a/nova/tests/functional/test_servers.py +++ b/nova/tests/functional/test_servers.py @@ -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'] diff --git a/nova/tests/functional/wsgi/test_secgroup.py b/nova/tests/functional/wsgi/test_secgroup.py index 33ca2f65210f..a2d0b4a8cdae 100644 --- a/nova/tests/functional/wsgi/test_secgroup.py +++ b/nova/tests/functional/wsgi/test_secgroup.py @@ -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] diff --git a/nova/tests/unit/network/test_manager.py b/nova/tests/unit/network/test_manager.py index 2d8a800f2fa8..32d1dd7295ae 100644 --- a/nova/tests/unit/network/test_manager.py +++ b/nova/tests/unit/network/test_manager.py @@ -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]) diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index f1b603dddfcf..aa138404fc0d 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -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) diff --git a/nova/tests/unit/virt/libvirt/test_fakelibvirt.py b/nova/tests/unit/virt/libvirt/test_fakelibvirt.py index 4918ce0d05de..8579df959205 100644 --- a/nova/tests/unit/virt/libvirt/test_fakelibvirt.py +++ b/nova/tests/unit/virt/libvirt/test_fakelibvirt.py @@ -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() diff --git a/nova/tests/unit/virt/test_virt_drivers.py b/nova/tests/unit/virt/test_virt_drivers.py index 302008a22d31..faae06e22a28 100644 --- a/nova/tests/unit/virt/test_virt_drivers.py +++ b/nova/tests/unit/virt/test_virt_drivers.py @@ -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__}) diff --git a/tox.ini b/tox.ini index e19dd4048da0..31b5e302c2d9 100644 --- a/tox.ini +++ b/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