From 397f49c2eed9b96a2c3c662154f5b89b53ea20af Mon Sep 17 00:00:00 2001 From: Takashi Kajinami Date: Thu, 25 Jan 2024 20:32:34 +0900 Subject: [PATCH] Bump hacking hacking was indirectly capped by pycodestyle. This bumps hacking to apply the rules recently added. Also remove the note about pip's behavior, which is no longer valid for recent versions. notes: - T117 test is now disabled. There are a lot of lines violating this rule and we have to decide if we really want to enforce it. - Once this is merged, we have to update bump hacking in some plugins which import hacking extensions from tempest. Change-Id: I5ee5e152418079f9f2720eb97c3a5361edba2695 --- requirements.txt | 3 - tempest/api/identity/admin/v3/test_groups.py | 2 +- tempest/common/utils/linux/remote_client.py | 18 +++--- tempest/common/utils/net_downtime.py | 21 ++++--- tempest/common/waiters.py | 5 +- tempest/hacking/checks.py | 65 ++++++++++---------- tempest/scenario/manager.py | 6 +- tempest/tests/test_hacking.py | 49 +++++++++------ tempest/tests/test_test.py | 4 +- test-requirements.txt | 8 +-- tox.ini | 3 +- 11 files changed, 94 insertions(+), 90 deletions(-) diff --git a/requirements.txt b/requirements.txt index d2f13a5917..a1eff53d99 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,3 @@ -# 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. pbr!=2.1.0,>=2.0.0 # Apache-2.0 cliff!=2.9.0,>=2.8.0 # Apache-2.0 jsonschema>=3.2.0 # MIT diff --git a/tempest/api/identity/admin/v3/test_groups.py b/tempest/api/identity/admin/v3/test_groups.py index b5b3c5d036..96218bbf4a 100644 --- a/tempest/api/identity/admin/v3/test_groups.py +++ b/tempest/api/identity/admin/v3/test_groups.py @@ -128,7 +128,7 @@ class GroupsV3TestJSON(base.BaseIdentityV3AdminTest): for g in user_groups: if 'membership_expires_at' in g: self.assertIsNone(g['membership_expires_at']) - del(g['membership_expires_at']) + del g['membership_expires_at'] self.assertEqual(sorted(groups, key=lambda k: k['name']), sorted(user_groups, key=lambda k: k['name'])) self.assertEqual(2, len(user_groups)) diff --git a/tempest/common/utils/linux/remote_client.py b/tempest/common/utils/linux/remote_client.py index dd181901e4..79cc09cde2 100644 --- a/tempest/common/utils/linux/remote_client.py +++ b/tempest/common/utils/linux/remote_client.py @@ -59,14 +59,14 @@ class RemoteClient(remote_client.RemoteClient): output = self.exec_command(command) selected = [] pos = None - for l in output.splitlines(): - if pos is None and l.find("TYPE") > 0: - pos = l.find("TYPE") + for line in output.splitlines(): + if pos is None and line.find("TYPE") > 0: + pos = line.find("TYPE") # Show header line too - selected.append(l) + selected.append(line) # lsblk lists disk type in a column right-aligned with TYPE - elif pos is not None and pos > 0 and l[pos:pos + 4] == "disk": - selected.append(l) + elif pos is not None and pos > 0 and line[pos:pos + 4] == "disk": + selected.append(line) if selected: return "\n".join(selected) @@ -121,9 +121,9 @@ class RemoteClient(remote_client.RemoteClient): def _get_dns_servers(self): cmd = 'cat /etc/resolv.conf' resolve_file = self.exec_command(cmd).strip().split('\n') - entries = (l.split() for l in resolve_file) - dns_servers = [l[1] for l in entries - if len(l) and l[0] == 'nameserver'] + entries = (line.split() for line in resolve_file) + dns_servers = [line[1] for line in entries + if len(line) and line[0] == 'nameserver'] return dns_servers def get_dns_servers(self, timeout=5): diff --git a/tempest/common/utils/net_downtime.py b/tempest/common/utils/net_downtime.py index 6f2a9474b3..ec1a4c8a22 100644 --- a/tempest/common/utils/net_downtime.py +++ b/tempest/common/utils/net_downtime.py @@ -50,10 +50,10 @@ done class NetDowntimeMeter(fixtures.Fixture): - def __init__(self, dest_ip, interval='0.2'): + def __init__(self, dest_ip, interval=0.2): self.dest_ip = dest_ip # Note: for intervals lower than 0.2 ping requires root privileges - self.interval = interval + self.interval = float(interval) self.ping_process = None def _setUp(self): @@ -61,18 +61,18 @@ class NetDowntimeMeter(fixtures.Fixture): def start_background_pinger(self): cmd = ['ping', '-q', '-s1'] - cmd.append('-i{}'.format(self.interval)) + cmd.append('-i%g' % self.interval) cmd.append(self.dest_ip) - LOG.debug("Starting background pinger to '{}' with interval {}".format( - self.dest_ip, self.interval)) + LOG.debug("Starting background pinger to '%s' with interval %g", + self.dest_ip, self.interval) self.ping_process = subprocess.Popen( cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) self.addCleanup(self.cleanup) def cleanup(self): if self.ping_process and self.ping_process.poll() is None: - LOG.debug('Terminating background pinger with pid {}'.format( - self.ping_process.pid)) + LOG.debug('Terminating background pinger with pid %d', + self.ping_process.pid) self.ping_process.terminate() self.ping_process = None @@ -83,7 +83,7 @@ class NetDowntimeMeter(fixtures.Fixture): output = self.ping_process.stderr.readline().strip().decode('utf-8') if output and len(output.split()[0].split('/')) == 2: succ, total = output.split()[0].split('/') - return (int(total) - int(succ)) * float(self.interval) + return (int(total) - int(succ)) * self.interval else: LOG.warning('Unexpected output obtained from the pinger: %s', output) @@ -115,8 +115,9 @@ class MetadataDowntimeMeter(fixtures.Fixture): chmod_cmd = 'chmod +x {}'.format(self.script_path) self.ssh_client.exec_command(';'.join((echo_cmd, chmod_cmd))) LOG.debug('script created: %s', self.script_path) - LOG.debug(self.ssh_client.exec_command( - 'cat {}'.format(self.script_path))) + output = self.ssh_client.exec_command( + 'cat {}'.format(self.script_path)) + LOG.debug('script content: %s', output) def run_metadata_script(self): self.ssh_client.exec_command('{} > {} &'.format(self.script_path, diff --git a/tempest/common/waiters.py b/tempest/common/waiters.py index 9e97f4797e..2c42bfd3ed 100644 --- a/tempest/common/waiters.py +++ b/tempest/common/waiters.py @@ -327,8 +327,7 @@ def wait_for_image_deleted_from_store(client, image, available_stores, # Check if image have last store location if len(available_stores) == 1: exc_cls = lib_exc.OtherRestClientException - message = ('Delete from last store location not allowed' - % (image, image_store_deleted)) + message = 'Delete from last store location not allowed' raise exc_cls(message) start = int(time.time()) while int(time.time()) - start < client.build_timeout: @@ -548,7 +547,7 @@ def wait_for_interface_status(client, server_id, port_id, status): interface_status = body['port_state'] start = int(time.time()) - while(interface_status != status): + while interface_status != status: time.sleep(client.build_interval) body = (client.show_interface(server_id, port_id) ['interfaceAttachment']) diff --git a/tempest/hacking/checks.py b/tempest/hacking/checks.py index 1c9c55b126..c81ec03c9b 100644 --- a/tempest/hacking/checks.py +++ b/tempest/hacking/checks.py @@ -16,7 +16,6 @@ import os import re from hacking import core -import pycodestyle PYTHON_CLIENTS = ['cinder', 'glance', 'keystone', 'nova', 'swift', 'neutron', @@ -40,22 +39,22 @@ _HAVE_NEGATIVE_DECORATOR = False @core.flake8ext -def import_no_clients_in_api_and_scenario_tests(physical_line, filename): +def import_no_clients_in_api_and_scenario_tests(logical_line, filename): """Check for client imports from tempest/api & tempest/scenario tests T102: Cannot import OpenStack python clients """ if "tempest/api" in filename or "tempest/scenario" in filename: - res = PYTHON_CLIENT_RE.match(physical_line) + res = PYTHON_CLIENT_RE.match(logical_line) if res: - return (physical_line.find(res.group(1)), + return (logical_line.find(res.group(1)), ("T102: python clients import not allowed" " in tempest/api/* or tempest/scenario/* tests")) @core.flake8ext -def scenario_tests_need_service_tags(physical_line, filename, +def scenario_tests_need_service_tags(logical_line, filename, previous_logical): """Check that scenario tests have service tags @@ -63,28 +62,28 @@ def scenario_tests_need_service_tags(physical_line, filename, """ if 'tempest/scenario/' in filename and '/test_' in filename: - if TEST_DEFINITION.match(physical_line): + if TEST_DEFINITION.match(logical_line): if not SCENARIO_DECORATOR.match(previous_logical): - return (physical_line.find('def'), + return (logical_line.find('def'), "T104: Scenario tests require a service decorator") @core.flake8ext -def no_setup_teardown_class_for_tests(physical_line, filename): +def no_setup_teardown_class_for_tests(logical_line, filename, noqa): - if pycodestyle.noqa(physical_line): + if noqa: return if 'tempest/test.py' in filename or 'tempest/lib/' in filename: return - if SETUP_TEARDOWN_CLASS_DEFINITION.match(physical_line): - return (physical_line.find('def'), + if SETUP_TEARDOWN_CLASS_DEFINITION.match(logical_line): + return (logical_line.find('def'), "T105: (setUp|tearDown)Class can not be used in tests") @core.flake8ext -def service_tags_not_in_module_path(physical_line, filename): +def service_tags_not_in_module_path(logical_line, filename): """Check that a service tag isn't in the module path A service tag should only be added if the service name isn't already in @@ -96,14 +95,14 @@ def service_tags_not_in_module_path(physical_line, filename): # created for services like heat which would cause false negatives for # those tests, so just exclude the scenario tests. if 'tempest/scenario' not in filename: - matches = SCENARIO_DECORATOR.match(physical_line) + matches = SCENARIO_DECORATOR.match(logical_line) if matches: services = matches.group(1).split(',') for service in services: service_name = service.strip().strip("'") modulepath = os.path.split(filename)[0] if service_name in modulepath: - return (physical_line.find(service_name), + return (logical_line.find(service_name), "T107: service tag should not be in path") @@ -140,28 +139,27 @@ def no_testtools_skip_decorator(logical_line): "decorators.skip_because from tempest.lib") -def _common_service_clients_check(logical_line, physical_line, filename): +def _common_service_clients_check(logical_line, filename, noqa): + if noqa: + return False + if not re.match('tempest/(lib/)?services/.*', filename): return False - if not METHOD.match(physical_line): - return False - - if pycodestyle.noqa(physical_line): + if not METHOD.match(logical_line): return False return True @core.flake8ext -def get_resources_on_service_clients(physical_line, logical_line, filename, - line_number, lines): +def get_resources_on_service_clients(logical_line, filename, + line_number, lines, noqa): """Check that service client names of GET should be consistent T110 """ - if not _common_service_clients_check(logical_line, physical_line, - filename): + if not _common_service_clients_check(logical_line, filename, noqa): return for line in lines[line_number:]: @@ -182,14 +180,13 @@ def get_resources_on_service_clients(physical_line, logical_line, filename, @core.flake8ext -def delete_resources_on_service_clients(physical_line, logical_line, filename, - line_number, lines): +def delete_resources_on_service_clients(logical_line, filename, + line_number, lines, noqa): """Check that service client names of DELETE should be consistent T111 """ - if not _common_service_clients_check(logical_line, physical_line, - filename): + if not _common_service_clients_check(logical_line, filename, noqa): return for line in lines[line_number:]: @@ -262,7 +259,7 @@ def dont_use_config_in_tempest_lib(logical_line, filename): 'oslo_config' in logical_line): msg = ('T114: tempest.lib can not have any dependency on tempest ' 'config.') - yield(0, msg) + yield (0, msg) @core.flake8ext @@ -281,7 +278,7 @@ def dont_put_admin_tests_on_nonadmin_path(logical_line, if not re.match(r'.\/tempest\/api\/.*\/admin\/.*', filename): msg = 'T115: All admin tests should exist under admin path.' - yield(0, msg) + yield (0, msg) @core.flake8ext @@ -293,11 +290,11 @@ def unsupported_exception_attribute_PY3(logical_line): result = EX_ATTRIBUTE.search(logical_line) msg = ("[T116] Unsupported 'message' Exception attribute in PY3") if result: - yield(0, msg) + yield (0, msg) @core.flake8ext -def negative_test_attribute_always_applied_to_negative_tests(physical_line, +def negative_test_attribute_always_applied_to_negative_tests(logical_line, filename): """Check ``@decorators.attr(type=['negative'])`` applied to negative tests. @@ -307,13 +304,13 @@ def negative_test_attribute_always_applied_to_negative_tests(physical_line, if re.match(r'.\/tempest\/api\/.*_negative.*', filename): - if NEGATIVE_TEST_DECORATOR.match(physical_line): + if NEGATIVE_TEST_DECORATOR.match(logical_line): _HAVE_NEGATIVE_DECORATOR = True return - if TEST_DEFINITION.match(physical_line): + if TEST_DEFINITION.match(logical_line): if not _HAVE_NEGATIVE_DECORATOR: - return ( + yield ( 0, "T117: Must apply `@decorators.attr(type=['negative'])`" " to all negative API tests" ) diff --git a/tempest/scenario/manager.py b/tempest/scenario/manager.py index 01c42c8b4b..369efcc18c 100644 --- a/tempest/scenario/manager.py +++ b/tempest/scenario/manager.py @@ -851,7 +851,7 @@ class ScenarioTest(tempest.test.BaseTestCase): kernel_img_path = os.path.join(extract_dir, fname) elif re.search(r'(.*-initrd.*|ari-.*/image$)', fname): ramdisk_img_path = os.path.join(extract_dir, fname) - elif re.search(f'(.*\\.img$|ami-.*/image$)', fname): + elif re.search(r'(.*\\.img$|ami-.*/image$)', fname): img_path = os.path.join(extract_dir, fname) # Create the kernel image. kparams = { @@ -1561,8 +1561,8 @@ class NetworkScenarioTest(ScenarioTest): floating_ip = (self.floating_ips_client. show_floatingip(floatingip_id)['floatingip']) if status == floating_ip['status']: - LOG.info("FloatingIP: {fp} is at status: {st}" - .format(fp=floating_ip, st=status)) + LOG.info("FloatingIP: %s is at status: %s", + floating_ip, status) return status == floating_ip['status'] if not test_utils.call_until_true(refresh, diff --git a/tempest/tests/test_hacking.py b/tempest/tests/test_hacking.py index 464e66a1fa..3f603e875b 100644 --- a/tempest/tests/test_hacking.py +++ b/tempest/tests/test_hacking.py @@ -51,25 +51,34 @@ class HackingTestCase(base.TestCase): def test_no_setup_teardown_class_for_tests(self): self.assertTrue(checks.no_setup_teardown_class_for_tests( - " def setUpClass(cls):", './tempest/tests/fake_test.py')) + " def setUpClass(cls):", './tempest/tests/fake_test.py', False)) self.assertIsNone(checks.no_setup_teardown_class_for_tests( - " def setUpClass(cls): # noqa", './tempest/tests/fake_test.py')) + " def setUpClass(cls):", './tempest/tests/fake_test.py', + True)) self.assertTrue(checks.no_setup_teardown_class_for_tests( - " def setUpClass(cls):", './tempest/api/fake_test.py')) + " def setUpClass(cls):", './tempest/api/fake_test.py', + False)) self.assertTrue(checks.no_setup_teardown_class_for_tests( - " def setUpClass(cls):", './tempest/scenario/fake_test.py')) + " def setUpClass(cls):", './tempest/scenario/fake_test.py', + False)) self.assertFalse(checks.no_setup_teardown_class_for_tests( - " def setUpClass(cls):", './tempest/test.py')) + " def setUpClass(cls):", './tempest/test.py', + False)) self.assertTrue(checks.no_setup_teardown_class_for_tests( - " def tearDownClass(cls):", './tempest/tests/fake_test.py')) + " def tearDownClass(cls):", './tempest/tests/fake_test.py', + False)) self.assertIsNone(checks.no_setup_teardown_class_for_tests( - " def tearDownClass(cls): # noqa", './tempest/tests/fake_test.py')) + " def tearDownClass(cls):", './tempest/tests/fake_test.py', + True)) self.assertTrue(checks.no_setup_teardown_class_for_tests( - " def tearDownClass(cls):", './tempest/api/fake_test.py')) + " def tearDownClass(cls):", './tempest/api/fake_test.py', + False)) self.assertTrue(checks.no_setup_teardown_class_for_tests( - " def tearDownClass(cls):", './tempest/scenario/fake_test.py')) + " def tearDownClass(cls):", './tempest/scenario/fake_test.py', + False)) self.assertFalse(checks.no_setup_teardown_class_for_tests( - " def tearDownClass(cls):", './tempest/test.py')) + " def tearDownClass(cls):", './tempest/test.py', + False)) def test_import_no_clients_in_api_and_scenario_tests(self): for client in checks.PYTHON_CLIENTS: @@ -198,22 +207,26 @@ class HackingTestCase(base.TestCase): # arbitrarily many decorators. These insert decorators above the # @decorators.attr(type=['negative']) decorator. for decorator in other_decorators: - self.assertIsNone(check(" %s" % decorator, filename)) + self.assertFalse( + list(check(" %s" % decorator, filename))) if with_negative_decorator: - self.assertIsNone( - check("@decorators.attr(type=['negative'])", filename)) + self.assertFalse( + list(check("@decorators.attr(type=['negative'])", filename))) if with_other_decorators: # Include multiple decorators to verify that this check works with # arbitrarily many decorators. These insert decorators between # the test and the @decorators.attr(type=['negative']) decorator. for decorator in other_decorators: - self.assertIsNone(check(" %s" % decorator, filename)) - final_result = check(" def test_some_negative_case", filename) + self.assertFalse( + list(check(" %s" % decorator, filename))) + final_result = list(check(" def test_some_negative_case", filename)) if expected_success: - self.assertIsNone(final_result) + self.assertFalse(final_result) else: - self.assertIsInstance(final_result, tuple) - self.assertFalse(final_result[0]) + self.assertEqual(1, len(final_result)) + self.assertIsInstance(final_result[0], tuple) + self.assertEqual(0, final_result[0][0]) + self.assertTrue(final_result[0][1]) def test_no_negatve_test_attribute_applied_to_negative_test(self): # Check negative filename, negative decorator passes diff --git a/tempest/tests/test_test.py b/tempest/tests/test_test.py index 80825a4896..7fb9bb39db 100644 --- a/tempest/tests/test_test.py +++ b/tempest/tests/test_test.py @@ -303,7 +303,7 @@ class TestTempestBaseTestClass(base.TestCase): # [0]: test, err, details [1] -> exc_info # Type, Exception, traceback [1] -> MultipleException found_exc = log[0][1][1] - self.assertTrue(isinstance(found_exc, testtools.MultipleExceptions)) + self.assertIsInstance(found_exc, testtools.MultipleExceptions) self.assertEqual(2, len(found_exc.args)) # Each arg is exc_info - match messages and order self.assertIn('mock3 resource', str(found_exc.args[0][1])) @@ -332,7 +332,7 @@ class TestTempestBaseTestClass(base.TestCase): # [0]: test, err, details [1] -> exc_info # Type, Exception, traceback [1] -> RuntimeError found_exc = log[0][1][1] - self.assertTrue(isinstance(found_exc, RuntimeError)) + self.assertIsInstance(found_exc, RuntimeError) self.assertIn(BadResourceCleanup.__name__, str(found_exc)) def test_super_skip_checks_not_invoked(self): diff --git a/test-requirements.txt b/test-requirements.txt index 17fa9f1086..bd4d77291f 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -1,8 +1,4 @@ -# 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>=3.0.1,<3.1.0;python_version>='3.5' # Apache-2.0 +hacking>=6.1.0,<6.2.0 coverage!=4.4,>=4.0 # Apache-2.0 oslotest>=3.2.0 # Apache-2.0 -pycodestyle>=2.0.0,<2.6.0 # MIT -flake8-import-order==0.11 # LGPLv3 +flake8-import-order>=0.18.0,<0.19.0 # LGPLv3 diff --git a/tox.ini b/tox.ini index e3c8fcfd48..d9d2bad83b 100644 --- a/tox.ini +++ b/tox.ini @@ -411,7 +411,8 @@ import_exceptions = tempest.services # E129 skipped because it is too limiting when combined with other rules # W504 skipped because it is overeager and unnecessary # H405 skipped because it arbitrarily forces doctring "title" lines -ignore = E125,E123,E129,W504,H405 +# I201 and I202 skipped because the rule does not allow new line between 3rd party modules and own modules +ignore = E125,E123,E129,W504,H405,I201,I202,T117 show-source = True exclude = .git,.venv,.tox,dist,doc,*egg,build enable-extensions = H106,H203,H904