diff --git a/HACKING.rst b/HACKING.rst index 1161a350cb..f0c4ccfbd8 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 144ad5d2a9..b4a477adca 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 192b465592..bd123d8945 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 54b41c6c60..e94a279a29 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 4f62f7d82c..6bf8c2377e 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 0f3df6b12f..107dcf562a 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 33ca2f6521..a2d0b4a8cd 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 2d8a800f2f..32d1dd7295 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 f1b603dddf..aa138404fc 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 4918ce0d05..8579df9592 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 302008a22d..faae06e22a 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 e19dd4048d..31b5e302c2 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