From eb7c4c61a9253b99e9e658d27b4f0170c183d6ce Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 7 Nov 2025 12:41:29 +0000 Subject: [PATCH] Add new hacking rules To catch some obvious issues. Change-Id: Ic0ddc95100811e7b313b519aad7d687a1415020b Signed-off-by: Stephen Finucane --- .pre-commit-config.yaml | 3 +- hacking/checks.py | 108 ++++++++++++++++++ .../tests/unit/network/test_common.py | 14 +-- pyproject.toml | 1 + tox.ini | 11 +- 5 files changed, 127 insertions(+), 10 deletions(-) create mode 100644 hacking/checks.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 47903a90d6..b277bb059b 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -32,10 +32,11 @@ repos: - id: mypy additional_dependencies: - types-requests - # keep this in-sync with '[mypy] exclude' in 'setup.cfg' + # keep this in-sync with '[tool.mypy] exclude' in 'pyproject.toml' exclude: | (?x)( doc/.* | examples/.* + | hacking/.* | releasenotes/.* ) diff --git a/hacking/checks.py b/hacking/checks.py new file mode 100644 index 0000000000..98b40370df --- /dev/null +++ b/hacking/checks.py @@ -0,0 +1,108 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import os +import re + +from hacking import core + +""" +Guidelines for writing new hacking checks + + - Use only for python-openstackclient specific tests. OpenStack general tests + should be submitted to the common 'hacking' module. + - Pick numbers in the range O4xx. Find the current test with the highest + allocated number and then pick the next value. +""" + + +@core.flake8ext +def assert_no_oslo(logical_line): + """Check for use of oslo libraries. + + O400 + """ + if re.match(r'(from|import) oslo_.*', logical_line): + yield (0, "0400: oslo libraries should not be used in SDK projects") + + +@core.flake8ext +def assert_no_duplicated_setup(logical_line, filename): + """Check for use of various unnecessary test duplications. + + O401 + """ + if os.path.join('openstackclient', 'tests', 'unit') not in filename: + return + + if re.match(r'self.app = .*\(self.app, self.namespace\)', logical_line): + yield ( + 0, + 'O401: It is not necessary to create dummy Namespace objects', + ) + + if os.path.basename(filename) != 'fakes.py': + if re.match( + r'self.[a-z_]+_client = self.app.client_manager.*', logical_line + ): + yield ( + 0, + "O401: Aliases for mocks of the service client are already " + "provided by the respective service's FakeClientMixin class", + ) + + if match := re.match( + r'self.app.client_manager.([a-z_]+) = mock.Mock', logical_line + ): + service = match.group(1) + if service == 'auth_ref': + return + yield ( + 0, + f"O401: client_manager.{service} mocks are already provided by " + f"the {service} service's FakeClientMixin class", + ) + + +@core.flake8ext +def assert_use_of_client_aliases(logical_line, filename): + """Ensure we use $service_client instead of $sdk_connection.service. + + O402 + """ + # we should expand the list of services as we drop legacy clients + if match := re.match( + r'self\.app\.client_manager\.sdk_connnection\.(compute|network|image)', + logical_line, + ): + service = match.group(1) + yield (0, f"0402: prefer {service}_client to sdk_connection.{service}") + + if match := re.match( + r'(self\.app\.client_manager\.(compute|network|image)+\.[a-z_]+) = mock.Mock', + logical_line, + ): + yield ( + 0, + f"O402: {match.group(1)} is already a mock: there's no need to " + f"assign a new mock.Mock instance.", + ) + + if match := re.match( + r'(self\.(compute|network|image)_client\.[a-z_]+) = mock.Mock', + logical_line, + ): + yield ( + 0, + f"O402: {match.group(1)} is already a mock: there's no need to " + f"assign a new mock.Mock instance.", + ) diff --git a/openstackclient/tests/unit/network/test_common.py b/openstackclient/tests/unit/network/test_common.py index 02cb021ef6..cc84c8bdf0 100644 --- a/openstackclient/tests/unit/network/test_common.py +++ b/openstackclient/tests/unit/network/test_common.py @@ -126,12 +126,12 @@ class TestNetworkAndCompute(utils.TestCommand): # Create client mocks. Note that we intentionally do not use specced # mocks since we want to test fake methods. - self.app.client_manager.network = mock.Mock() - self.network_client = self.app.client_manager.network + self.app.client_manager.network = mock.Mock() # noqa: O401 + self.network_client = self.app.client_manager.network # noqa: O401 self.network_client.network_action.return_value = 'take_action_network' - self.app.client_manager.compute = mock.Mock() - self.compute_client = self.app.client_manager.compute + self.app.client_manager.compute = mock.Mock() # noqa: O401 + self.compute_client = self.app.client_manager.compute # noqa: O401 self.compute_client.compute_action.return_value = 'take_action_compute' self.cmd = FakeNetworkAndComputeCommand(self.app, None) @@ -201,9 +201,9 @@ class TestNeutronCommandWithExtraArgs(utils.TestCommand): # Create client mocks. Note that we intentionally do not use specced # mocks since we want to test fake methods. - self.app.client_manager.network = mock.Mock() - self.network_client = self.app.client_manager.network - self.network_client.test_create_action = mock.Mock() + self.app.client_manager.network = mock.Mock() # noqa: O401 + self.network_client = self.app.client_manager.network # noqa: O401 + self.network_client.test_create_action = mock.Mock() # noqa: O402 # Subclasses can override the command object to test. self.cmd = FakeCreateNeutronCommandWithExtraArgs(self.app, None) diff --git a/pyproject.toml b/pyproject.toml index 74f93ccb2f..2ebe0a52cd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -732,6 +732,7 @@ exclude = ''' (?x)( doc | examples + | hacking | releasenotes ) ''' diff --git a/tox.ini b/tox.ini index 7b3d951d20..2ee2dc2cad 100644 --- a/tox.ini +++ b/tox.ini @@ -113,9 +113,16 @@ commands = [flake8] show-source = true exclude = .venv,.git,.tox,dist,doc,*lib/python*,*egg,build,tools,releasenotes -# We only enable the hacking (H) checks -select = H +# We only enable the hacking (H) and openstackclient (O) checks +select = H,O # H301 Black will put commas after imports that can't fit on one line ignore = H301 import-order-style = pep8 application_import_names = openstackclient + +[flake8:local-plugins] +extension = + O400 = checks:assert_no_oslo + O401 = checks:assert_no_duplicated_setup + O402 = checks:assert_use_of_client_aliases +paths = ./hacking