From 117544f62bd1fa7510260b2c1d23d0c02f4e8507 Mon Sep 17 00:00:00 2001 From: Andreas Jaeger Date: Thu, 2 Apr 2020 09:41:25 +0200 Subject: [PATCH] Update hacking for Python3 The repo is Python 3 now, so update hacking to version 3.0 which supports Python 3. Fix problems found. Update local hacking checks for new flake8. Note that the repo has been running hacking 2 for some time due to uncapped setup, and thus fixes are needed for N350 - the uncapped setup disabled the local hacking rules. They are now enabled again. Change-Id: I25e5201933f65ba4d3045e6445f0e6419ea57f3e --- .../task/scenarios/mistral/utils.py | 10 +- .../task/ui/charts/osprofilerchart.py | 4 +- test-requirements.txt | 2 +- tests/hacking/checks.py | 104 +++++++++--------- .../unit/task/scenarios/mistral/test_utils.py | 12 +- .../unit/verification/tempest/test_context.py | 4 +- tox.ini | 28 ++++- 7 files changed, 94 insertions(+), 70 deletions(-) diff --git a/rally_openstack/task/scenarios/mistral/utils.py b/rally_openstack/task/scenarios/mistral/utils.py index c8c025fd..be560e7a 100644 --- a/rally_openstack/task/scenarios/mistral/utils.py +++ b/rally_openstack/task/scenarios/mistral/utils.py @@ -37,7 +37,7 @@ class MistralScenario(scenario.OpenStackScenario): return self.clients("mistral").workbooks.list() @atomic.action_timer("mistral.create_workbook") - def _create_workbook(self, definition, namespace=''): + def _create_workbook(self, definition, namespace=""): """Create a new workbook. :param definition: workbook description in string @@ -56,7 +56,7 @@ class MistralScenario(scenario.OpenStackScenario): ) @atomic.action_timer("mistral.delete_workbook") - def _delete_workbook(self, wb_name, namespace=''): + def _delete_workbook(self, wb_name, namespace=""): """Delete the given workbook. :param wb_name: the name of workbook that would be deleted. @@ -68,7 +68,7 @@ class MistralScenario(scenario.OpenStackScenario): ) @atomic.action_timer("mistral.create_workflow") - def _create_workflow(self, definition, namespace=''): + def _create_workflow(self, definition, namespace=""): """creates a workflow in the given namespace. :param definition: the definition of workflow @@ -80,7 +80,7 @@ class MistralScenario(scenario.OpenStackScenario): ) @atomic.action_timer("mistral.delete_workflow") - def _delete_workflow(self, workflow_identifier, namespace=''): + def _delete_workflow(self, workflow_identifier, namespace=""): """Delete the given workflow. :param workflow_identifier: the identifier of workflow @@ -105,7 +105,7 @@ class MistralScenario(scenario.OpenStackScenario): @atomic.action_timer("mistral.create_execution") def _create_execution(self, workflow_identifier, wf_input=None, - namespace='', **params): + namespace="", **params): """Create a new execution. :param workflow_identifier: name or id of the workflow to execute diff --git a/rally_openstack/task/ui/charts/osprofilerchart.py b/rally_openstack/task/ui/charts/osprofilerchart.py index a5fb3545..2eab24f3 100644 --- a/rally_openstack/task/ui/charts/osprofilerchart.py +++ b/rally_openstack/task/ui/charts/osprofilerchart.py @@ -58,9 +58,9 @@ class OSProfilerChart(charts.OutputEmbeddedChart, from osprofiler.drivers import base from osprofiler import opts as osprofiler_opts - opts.register_opts(osprofiler_opts.list_opts()) + opts.register_opts(osprofiler_opts.list_opts()) # noqa - try: + try: # noqa engine = base.get_driver(connection_str) except Exception: msg = "Error while fetching OSProfiler results." diff --git a/test-requirements.txt b/test-requirements.txt index de41ada4..0542fedc 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -2,7 +2,7 @@ # of appearance. Changing the order has an impact on the overall integration # process, which may cause wedges in the gate later. -hacking>=0.12.0,!=0.13.0 # Apache Software License +hacking>=3.0,<3.1.0 # Apache-2.0 pytest<=5.3 # MIT # py.test plugin for measuring coverage. diff --git a/tests/hacking/checks.py b/tests/hacking/checks.py index d2d67e3c..a7f78c88 100644 --- a/tests/hacking/checks.py +++ b/tests/hacking/checks.py @@ -28,6 +28,8 @@ import functools import re import tokenize +from hacking import core + re_assert_equal_end_with_true_or_false = re.compile( r"assertEqual\(.*?, \s+(True|False)\)$") @@ -79,12 +81,12 @@ re_log_warn = re.compile(r"(.)*LOG\.(warn)\(\s*('|\"|_)") def skip_ignored_lines(func): @functools.wraps(func) - def wrapper(logical_line, physical_line, filename): + def wrapper(physical_line, logical_line, filename): line = physical_line.strip() if not line or line.startswith("#") or line.endswith("# noqa"): return try: - for res in func(logical_line, physical_line, filename): + for res in func(physical_line, logical_line, filename): yield res except StopIteration: return @@ -105,8 +107,9 @@ def _parse_assert_mock_str(line): return None, None, None +@core.flake8ext @skip_ignored_lines -def check_assert_methods_from_mock(logical_line, physical_line, filename): +def check_assert_methods_from_mock(physical_line, logical_line, filename): """Ensure that ``assert_*`` methods from ``mock`` library is used correctly N301 - base error number @@ -158,8 +161,9 @@ def check_assert_methods_from_mock(logical_line, physical_line, filename): "custom_msg": custom_msg}) +@core.flake8ext @skip_ignored_lines -def check_import_of_logging(logical_line, physical_line, filename): +def check_import_of_logging(physical_line, logical_line, filename): """Check correctness import of logging module N310 @@ -181,8 +185,9 @@ def check_import_of_logging(logical_line, physical_line, filename): "use `rally.common.logging` instead.") +@core.flake8ext @skip_ignored_lines -def check_import_of_config(logical_line, physical_line, filename): +def check_import_of_config(physical_line, logical_line, filename): """Check correctness import of config module N311 @@ -200,8 +205,9 @@ def check_import_of_config(logical_line, physical_line, filename): "use `rally.common.cfg` instead.") +@core.flake8ext @skip_ignored_lines -def no_use_conf_debug_check(logical_line, physical_line, filename): +def no_use_conf_debug_check(physical_line, logical_line, filename): """Check for "cfg.CONF.debug" Rally has two DEBUG level: @@ -220,8 +226,9 @@ def no_use_conf_debug_check(logical_line, physical_line, filename): "should be used instead.") +@core.flake8ext @skip_ignored_lines -def assert_true_instance(logical_line, physical_line, filename): +def assert_true_instance(physical_line, logical_line, filename): """Check for assertTrue(isinstance(a, b)) sentences N320 @@ -231,8 +238,9 @@ def assert_true_instance(logical_line, physical_line, filename): "you should use assertIsInstance(a, b) instead.") +@core.flake8ext @skip_ignored_lines -def assert_equal_type(logical_line, physical_line, filename): +def assert_equal_type(physical_line, logical_line, filename): """Check for assertEqual(type(A), B) sentences N321 @@ -242,8 +250,9 @@ def assert_equal_type(logical_line, physical_line, filename): "you should use assertIsInstance(a, b) instead.") +@core.flake8ext @skip_ignored_lines -def assert_equal_none(logical_line, physical_line, filename): +def assert_equal_none(physical_line, logical_line, filename): """Check for assertEqual(A, None) or assertEqual(None, A) sentences N322 @@ -256,8 +265,9 @@ def assert_equal_none(logical_line, physical_line, filename): "instead.") +@core.flake8ext @skip_ignored_lines -def assert_true_or_false_with_in(logical_line, physical_line, filename): +def assert_true_or_false_with_in(physical_line, logical_line, filename): """Check assertTrue/False(A in/not in B) with collection contents Check for assertTrue/False(A in B), assertTrue/False(A not in B), @@ -275,8 +285,9 @@ def assert_true_or_false_with_in(logical_line, physical_line, filename): " instead.") +@core.flake8ext @skip_ignored_lines -def assert_equal_in(logical_line, physical_line, filename): +def assert_equal_in(physical_line, logical_line, filename): """Check assertEqual(A in/not in B, True/False) with collection contents Check for assertEqual(A in B, True/False), assertEqual(True/False, A in B), @@ -293,8 +304,9 @@ def assert_equal_in(logical_line, physical_line, filename): "collection contents.") +@core.flake8ext @skip_ignored_lines -def assert_not_equal_none(logical_line, physical_line, filename): +def assert_not_equal_none(physical_line, logical_line, filename): """Check for assertNotEqual(A, None) or assertEqual(None, A) sentences N325 @@ -307,8 +319,9 @@ def assert_not_equal_none(logical_line, physical_line, filename): "instead.") +@core.flake8ext @skip_ignored_lines -def assert_equal_true_or_false(logical_line, physical_line, filename): +def assert_equal_true_or_false(physical_line, logical_line, filename): """Check for assertEqual(A, True/False) sentences Check for assertEqual(A, True/False) sentences or @@ -324,8 +337,9 @@ def assert_equal_true_or_false(logical_line, physical_line, filename): "you should use assertTrue(A) or assertFalse(A) instead.") +@core.flake8ext @skip_ignored_lines -def check_no_direct_rally_objects_import(logical_line, physical_line, +def check_no_direct_rally_objects_import(physical_line, logical_line, filename): """Check if rally.common.objects are properly imported. @@ -347,8 +361,9 @@ def check_no_direct_rally_objects_import(logical_line, physical_line, "After that you can use directly objects e.g. objects.Task") +@core.flake8ext @skip_ignored_lines -def check_no_oslo_deprecated_import(logical_line, physical_line, filename): +def check_no_oslo_deprecated_import(physical_line, logical_line, filename): """Check if oslo.foo packages are not imported instead of oslo_foo ones. Libraries from oslo.foo namespace are deprecated because of namespace @@ -363,8 +378,9 @@ def check_no_oslo_deprecated_import(logical_line, physical_line, filename): "instead") +@core.flake8ext @skip_ignored_lines -def check_quotes(logical_line, physical_line, filename): +def check_quotes(physical_line, logical_line, filename): """Check that single quotation marks are not used N350 @@ -416,8 +432,9 @@ def check_quotes(logical_line, physical_line, filename): yield i, "N350 Remove Single quotes" +@core.flake8ext @skip_ignored_lines -def check_no_constructor_data_struct(logical_line, physical_line, filename): +def check_no_constructor_data_struct(physical_line, logical_line, filename): """Check that data structs (lists, dicts) are declared using literals N351 @@ -431,6 +448,7 @@ def check_no_constructor_data_struct(logical_line, physical_line, filename): yield 0, "N351 Remove list() construct and use literal []" +@core.flake8ext def check_dict_formatting_in_string(logical_line, tokens): """Check that strings do not use dict-formatting with a single replacement @@ -498,8 +516,9 @@ def check_dict_formatting_in_string(logical_line, tokens): current_string = "" +@core.flake8ext @skip_ignored_lines -def check_using_unicode(logical_line, physical_line, filename): +def check_using_unicode(physical_line, logical_line, filename): """Check crosspython unicode usage N353 @@ -510,7 +529,8 @@ def check_using_unicode(logical_line, physical_line, filename): "use 'str' instead.") -def check_raises(logical_line, physical_line, filename): +@core.flake8ext +def check_raises(physical_line, logical_line, filename): """Check raises usage N354 @@ -524,8 +544,9 @@ def check_raises(logical_line, physical_line, filename): "in docstrings.") +@core.flake8ext @skip_ignored_lines -def check_old_type_class(logical_line, physical_line, filename): +def check_old_type_class(physical_line, logical_line, filename): """Use new-style Python classes N355 @@ -537,8 +558,9 @@ def check_old_type_class(logical_line, physical_line, filename): "``object`` or another new-style class.") +@core.flake8ext @skip_ignored_lines -def check_datetime_alias(logical_line, physical_line, filename): +def check_datetime_alias(physical_line, logical_line, filename): """Ensure using ``dt`` as alias for ``datetime`` N356 @@ -547,8 +569,9 @@ def check_datetime_alias(logical_line, physical_line, filename): yield 0, "N356 Please use ``dt`` as alias for ``datetime``." +@core.flake8ext @skip_ignored_lines -def check_db_imports_in_cli(logical_line, physical_line, filename): +def check_db_imports_in_cli(physical_line, logical_line, filename): """Ensure that CLI modules do not use ``rally.common.db`` N360 @@ -561,8 +584,9 @@ def check_db_imports_in_cli(logical_line, physical_line, filename): "`rally.common.db``.") +@core.flake8ext @skip_ignored_lines -def check_objects_imports_in_cli(logical_line, physical_line, filename): +def check_objects_imports_in_cli(physical_line, logical_line, filename): """Ensure that CLI modules do not use ``rally.common.objects`` N361 @@ -574,14 +598,16 @@ def check_objects_imports_in_cli(logical_line, physical_line, filename): "`rally.common.objects``.") +@core.flake8ext @skip_ignored_lines -def check_log_warn(logical_line, physical_line, filename): +def check_log_warn(physical_line, logical_line, filename): if re_log_warn.search(logical_line): yield 0, "N313 LOG.warn is deprecated, please use LOG.warning" +@core.flake8ext @skip_ignored_lines -def check_opts_import_path(logical_line, physical_line, filename): +def check_opts_import_path(physical_line, logical_line, filename): """Ensure that we load opts from correct paths only N342 @@ -590,7 +616,7 @@ def check_opts_import_path(logical_line, physical_line, filename): "./rally/task/context.py", "./rally/task/scenario.py", "./rally/common/opts.py"] - forbidden_methods = [".register_opts("] + forbidden_methods = [r".register_opts("] if filename not in excluded_files: for forbidden_method in forbidden_methods: @@ -598,29 +624,3 @@ def check_opts_import_path(logical_line, physical_line, filename): yield (0, "N342 All options should be loaded from correct " "paths only. For 'openstack' " "its './rally/plugin/openstack/cfg'") - - -def factory(register): - register(check_assert_methods_from_mock) - register(check_import_of_logging) - register(check_import_of_config) - register(no_use_conf_debug_check) - register(assert_true_instance) - register(assert_equal_type) - register(assert_equal_none) - register(assert_true_or_false_with_in) - register(assert_equal_in) - register(assert_equal_true_or_false) - register(check_no_direct_rally_objects_import) - register(check_no_oslo_deprecated_import) - register(check_quotes) - register(check_no_constructor_data_struct) - register(check_dict_formatting_in_string) - register(check_using_unicode) - register(check_raises) - register(check_datetime_alias) - register(check_db_imports_in_cli) - register(check_objects_imports_in_cli) - register(check_old_type_class) - register(check_log_warn) - register(check_opts_import_path) diff --git a/tests/unit/task/scenarios/mistral/test_utils.py b/tests/unit/task/scenarios/mistral/test_utils.py index c5ceb4e1..b575a604 100644 --- a/tests/unit/task/scenarios/mistral/test_utils.py +++ b/tests/unit/task/scenarios/mistral/test_utils.py @@ -54,7 +54,7 @@ class MistralScenarioTestCase(test.ScenarioTestCase): scenario._delete_workbook("wb_name") self.clients("mistral").workbooks.delete.assert_called_once_with( "wb_name", - namespace='' + namespace="" ) self._test_atomic_action_timer( scenario.atomic_actions(), @@ -76,7 +76,7 @@ class MistralScenarioTestCase(test.ScenarioTestCase): def test_create_execution(self): scenario = utils.MistralScenario(context=self.context) - namespace = 'namespace' + namespace = "namespace" wf_name = "fake_wf_name" mock_wait_for_status = self.mock_wait_for_status.mock @@ -118,7 +118,7 @@ class MistralScenarioTestCase(test.ScenarioTestCase): mock_create_exec.assert_called_once_with( wf_name, workflow_input=INPUT_EXAMPLE, - namespace='' + namespace="" ) def test_create_execution_with_params(self): @@ -136,7 +136,7 @@ class MistralScenarioTestCase(test.ScenarioTestCase): mock_create_exec.assert_called_once_with( wf_name, workflow_input=None, - namespace='', + namespace="", **PARAMS_EXAMPLE ) @@ -189,8 +189,8 @@ wf: ) def test_delete_workflow(self): - wf_identifier = 'wf_identifier' - namespace = 'delete_wf_test' + wf_identifier = "wf_identifier" + namespace = "delete_wf_test" scenario = utils.MistralScenario(context=self.context) scenario._delete_workflow(wf_identifier, namespace=namespace) diff --git a/tests/unit/verification/tempest/test_context.py b/tests/unit/verification/tempest/test_context.py index c477bc58..4aa29ac1 100644 --- a/tests/unit/verification/tempest/test_context.py +++ b/tests/unit/verification/tempest/test_context.py @@ -292,8 +292,8 @@ class TempestContextTestCase(test.TestCase): _name, pos, _kwargs = client.create_router.mock_calls[0] router = pos[0]["router"] external_gateway_info = router["external_gateway_info"] - self.assertEqual('my-uuid', external_gateway_info["network_id"]) - self.assertEqual(True, external_gateway_info["enable_snat"]) + self.assertEqual("my-uuid", external_gateway_info["network_id"]) + self.assertTrue(external_gateway_info["enable_snat"]) def test__cleanup_tempest_roles(self): self.context._created_roles = [fakes.FakeRole(), fakes.FakeRole()] diff --git a/tox.ini b/tox.ini index f6adc5b5..dbcc549c 100644 --- a/tox.ini +++ b/tox.ini @@ -76,8 +76,32 @@ ignore = H105,E731,W503 show-source = true exclude=.venv,.git,.tox,dist,*lib/python*,*egg,tools,build,setup.py -[hacking] -local-check-factory = tests.hacking.checks.factory +[flake8:local-plugins] +extension = + N301 = checks:check_assert_methods_from_mock + N310 = checks:check_import_of_logging + N311 = checks:check_import_of_config + N312 = checks:no_use_conf_debug_check + N313 = checks:check_log_warn + N320 = checks:assert_true_instance + N321 = checks:assert_equal_type + N322 = checks:assert_equal_none + N323 = checks:assert_true_or_false_with_in + N324 = checks:assert_equal_in + N326 = checks:assert_equal_true_or_false + N340 = checks:check_no_direct_rally_objects_import + N341 = checks:check_no_oslo_deprecated_import + N342 = checks:check_opts_import_path + N350 = checks:check_quotes + N351 = checks:check_no_constructor_data_struct + N352 = checks:check_dict_formatting_in_string + N353 = checks:check_using_unicode + N354 = checks:check_raises + N355 = checks:check_old_type_class + N356 = checks:check_datetime_alias + N360 = checks:check_db_imports_in_cli + N361 = checks:check_objects_imports_in_cli +paths = ./tests/hacking [testenv:bindep] # Do not install any requirements. We want this to be fast and work even if