From a566169757e5675cc1cdcaf355d2d34f2a8aabbf Mon Sep 17 00:00:00 2001 From: mulhern Date: Thu, 8 Aug 2024 17:20:26 -0400 Subject: [PATCH] Add bandit and pyright checks to test code Use tenacity where it is useful to fix pyright and bandit errors. Change pylint target to lint target in top level Makefile, since the target also includes using bandit and pyright. Signed-off-by: mulhern --- .githooks/pre-commit | 2 +- .github/workflows/main.yml | 7 +- .github/workflows/support.yml | 3 + .github/workflows/weekly.yml | 3 + Makefile | 6 +- tests/client-dbus/Makefile | 8 +++ .../stratisd_client_dbus/_implementation.py | 18 +++-- tests/client-dbus/tests/udev/_loopback.py | 4 ++ tests/client-dbus/tests/udev/_utils.py | 72 ++++++++++--------- tests/client-dbus/tests/udev/test_predict.py | 2 +- tests/client-dbus/tests/udev/test_revert.py | 23 +++--- 11 files changed, 90 insertions(+), 58 deletions(-) diff --git a/.githooks/pre-commit b/.githooks/pre-commit index 45f5a07ec6..972cf61439 100755 --- a/.githooks/pre-commit +++ b/.githooks/pre-commit @@ -12,7 +12,7 @@ make fmt-ci && make clippy && make yamllint && make tmtlint && - make pylint && + make lint && make check-typos || exit 1 export PYTHONPATH=$PWD/tests/client-dbus/src diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ca03810fa1..6e49559b0a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -192,11 +192,14 @@ jobs: - name: Install dependencies run: > dnf install -y + bandit make pylint python3-dbus - - name: Run pylint - run: make -f Makefile pylint + - name: Install pyright + run: pip install --user pyright + - name: Run lint + run: make -f Makefile lint python-based-tests: runs-on: ubuntu-22.04 diff --git a/.github/workflows/support.yml b/.github/workflows/support.yml index 0f7adcf087..56725e060a 100644 --- a/.github/workflows/support.yml +++ b/.github/workflows/support.yml @@ -39,6 +39,7 @@ jobs: matrix: include: - dependencies: > + bandit pylint python3-dbus-client-gen python3-dbus-python-client-gen @@ -65,6 +66,8 @@ jobs: make ncurses ${{ matrix.dependencies }} + - name: Install pyright + run: pip install --user pyright - name: Run test run: ${{ matrix.task }} working-directory: ${{ matrix.working-directory }} diff --git a/.github/workflows/weekly.yml b/.github/workflows/weekly.yml index ebc6d0c872..2d2afb6015 100644 --- a/.github/workflows/weekly.yml +++ b/.github/workflows/weekly.yml @@ -17,6 +17,7 @@ jobs: include: # PYTHON CHECKS ON NEXT FEDORA PYTHON AND PYTHON TOOLS VERSION - dependencies: > + bandit pylint python3-dbus-client-gen python3-dbus-python-client-gen @@ -49,6 +50,8 @@ jobs: ${{ matrix.dependencies }} - name: Display Python version run: python --version + - name: Install pyright + run: pip install --user pyright - name: ${{ matrix.task }} run: ${{ matrix.task }} working-directory: ${{ matrix.working-directory }} diff --git a/Makefile b/Makefile index 2edbbf197f..425ec7a6cd 100644 --- a/Makefile +++ b/Makefile @@ -430,8 +430,10 @@ clippy: clippy-macros clippy-min clippy-udev-utils clippy-no-ipc clippy-utils cl cargo clippy ${CLIPPY_OPTS} ## Lint Python parts of the source code -pylint: +lint: pylint --disable=invalid-name ./src/bin/utils/stratis-decode-dm + bandit ./src/bin/utils/stratis-decode-dm --skip B101 + pyright ./src/bin/utils/stratis-decode-dm .PHONY: audit @@ -477,7 +479,7 @@ pylint: install-udev-binaries install-udev-cfg license - pylint + lint test test-valgrind test-loop diff --git a/tests/client-dbus/Makefile b/tests/client-dbus/Makefile index 8a9cddbf7b..b0528087c0 100644 --- a/tests/client-dbus/Makefile +++ b/tests/client-dbus/Makefile @@ -1,9 +1,17 @@ UNITTEST_OPTS = --verbose +# Ignore bandit B404 errors. Any import of the subprocess module causes this +# error. We know what we are doing when we import that module and do not +# need to be warned. +BANDIT_SKIP = --skip B404,B603,B311 + .PHONY: lint lint: pylint src/stratisd_client_dbus --ignore=_introspect.py pylint tests --disable=R0801 + bandit --recursive ./src ${BANDIT_SKIP} + bandit --recursive ./tests ${BANDIT_SKIP},B101 + pyright .PHONY: startup-tests startup-tests: diff --git a/tests/client-dbus/src/stratisd_client_dbus/_implementation.py b/tests/client-dbus/src/stratisd_client_dbus/_implementation.py index 81dcb4e28b..ef680f0c64 100644 --- a/tests/client-dbus/src/stratisd_client_dbus/_implementation.py +++ b/tests/client-dbus/src/stratisd_client_dbus/_implementation.py @@ -16,7 +16,7 @@ """ # isort: STDLIB -import xml.etree.ElementTree as ET +import xml.etree.ElementTree as ET # nosec B405 # isort: FIRSTPARTY from dbus_client_gen import managed_object_class, mo_query_builder @@ -31,9 +31,9 @@ ) from ._introspect import SPECS -_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE]) -_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE]) -_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE]) +_POOL_SPEC = ET.fromstring(SPECS[POOL_INTERFACE]) # nosec B314 +_FILESYSTEM_SPEC = ET.fromstring(SPECS[FILESYSTEM_INTERFACE]) # nosec B314 +_BLOCKDEV_SPEC = ET.fromstring(SPECS[BLOCKDEV_INTERFACE]) # nosec B314 pools = mo_query_builder(_POOL_SPEC) filesystems = mo_query_builder(_FILESYSTEM_SPEC) @@ -46,11 +46,15 @@ ObjectManager = make_class( "ObjectManager", - ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]), + ET.fromstring(SPECS["org.freedesktop.DBus.ObjectManager"]), # nosec B314 TIME_OUT, ) -Report = make_class("Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT) -Manager = make_class("Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT) +Report = make_class( + "Report", ET.fromstring(SPECS[REPORT_INTERFACE]), TIME_OUT # nosec B314 +) +Manager = make_class( + "Manager", ET.fromstring(SPECS[MANAGER_INTERFACE]), TIME_OUT # nosec B314 +) Filesystem = make_class("Filesystem", _FILESYSTEM_SPEC, TIME_OUT) Pool = make_class("Pool", _POOL_SPEC, TIME_OUT) Blockdev = make_class("Blockdev", _BLOCKDEV_SPEC, TIME_OUT) diff --git a/tests/client-dbus/tests/udev/_loopback.py b/tests/client-dbus/tests/udev/_loopback.py index 94e0a27cca..9b4aafe8d9 100644 --- a/tests/client-dbus/tests/udev/_loopback.py +++ b/tests/client-dbus/tests/udev/_loopback.py @@ -105,6 +105,8 @@ def create_devices(self, number): :return: list of keys for the devices :rtype: list of uuid.UUID """ + assert self.dir is not None + tokens = [] first_creation = self.count == 0 @@ -220,6 +222,8 @@ def destroy_all(self): Detach all the devices and delete the file(s) and directory! :return: None """ + assert self.dir is not None + self.destroy_devices() os.rmdir(self.dir) self.dir = None diff --git a/tests/client-dbus/tests/udev/_utils.py b/tests/client-dbus/tests/udev/_utils.py index 40c4fcd935..35e142cadc 100644 --- a/tests/client-dbus/tests/udev/_utils.py +++ b/tests/client-dbus/tests/udev/_utils.py @@ -31,7 +31,14 @@ import dbus import psutil import pyudev -from tenacity import Retrying, retry_if_exception_type, stop_after_delay, wait_fixed +from tenacity import ( + RetryError, + Retrying, + retry_if_exception_type, + stop_after_attempt, + stop_after_delay, + wait_fixed, +) # isort: LOCAL from stratisd_client_dbus import ( @@ -235,7 +242,7 @@ def settle(): :return: None """ time.sleep(2) - subprocess.check_call(["udevadm", "settle"]) + subprocess.check_call(["/usr/bin/udevadm", "settle"]) def wait_for_udev_count(expected_num): @@ -288,25 +295,28 @@ def wait_for_udev(fs_type, expected_paths): :raises RuntimeError: if unexpected device nodes are found """ expected_devnodes = frozenset((os.path.realpath(x) for x in expected_paths)) - found_devnodes = None context = pyudev.Context() - end_time = time.time() + 10.0 - while time.time() < end_time and not expected_devnodes == found_devnodes: - found_devnodes = frozenset( + def find_devnodes(): + return frozenset( [ x.device_node for x in context.list_devices(subsystem="block", ID_FS_TYPE=fs_type) + if x.device_node is not None ] ) - time.sleep(1) - if expected_devnodes != found_devnodes: + try: + for attempt in Retrying(wait=wait_fixed(1), stop=stop_after_delay(10)): + with attempt: + if expected_devnodes == find_devnodes(): + break + except RetryError as err: raise RuntimeError( f'Found unexpected devnodes: expected devnodes: {", ".join(expected_devnodes)} ' - f'!= found_devnodes: {", ".join(found_devnodes)}' - ) + f'!= found_devnodes: {", ".join(find_devnodes())}' + ) from err def processes(name): @@ -538,32 +548,24 @@ def wait_for_pools(self, expected_num, *, name=None): :return: list of pool information found :rtype: list of (str * MOPool) """ - (count, limit, dbus_err, found_num, known_pools, start_time) = ( - 0, - expected_num + 1, - None, - None, - None, - time.time(), - ) - while count < limit and not expected_num == found_num: - try: - known_pools = get_pools(name=name) - except dbus.exceptions.DBusException as err: - dbus_err = err - - if known_pools is not None: - found_num = len(known_pools) - - time.sleep(3) - count += 1 - - if found_num is None and dbus_err is not None: + try: + for attempt in Retrying( + retry=(retry_if_exception_type(dbus.exceptions.DBusException)), + wait=wait_fixed(3), + stop=stop_after_attempt(expected_num + 1), + reraise=True, + ): + with attempt: + known_pools = get_pools(name=name) + if len(known_pools) == expected_num: + break + except dbus.exceptions.DBusException as err: raise RuntimeError( - f"After {time.time() - start_time:.2f} seconds, the only " - "response is a D-Bus exception" - ) from dbus_err + "Failed to obtain any information about pools from the D-Bus" + ) from err - self.assertEqual(found_num, expected_num) + time.sleep(3) # Wait for the D-Bus to add an additional pool + known_pools = get_pools(name=name) + self.assertEqual(len(known_pools), expected_num) return known_pools diff --git a/tests/client-dbus/tests/udev/test_predict.py b/tests/client-dbus/tests/udev/test_predict.py index c6d52e26db..214c64eeab 100644 --- a/tests/client-dbus/tests/udev/test_predict.py +++ b/tests/client-dbus/tests/udev/test_predict.py @@ -94,7 +94,7 @@ def _call_blockdev_size(dev): :rtype: str """ with subprocess.Popen( - ["blockdev", "--getsize64", dev], + ["/usr/sbin/blockdev", "--getsize64", dev], stdout=subprocess.PIPE, ) as command: outs, _ = command.communicate() diff --git a/tests/client-dbus/tests/udev/test_revert.py b/tests/client-dbus/tests/udev/test_revert.py index db010b5cd2..99bfe00106 100644 --- a/tests/client-dbus/tests/udev/test_revert.py +++ b/tests/client-dbus/tests/udev/test_revert.py @@ -42,6 +42,9 @@ settle, ) +_MOUNT = "/usr/bin/mount" +_UMOUNT = "/usr/bin/umount" + def write_file(mountdir, filename): """ @@ -99,7 +102,7 @@ def test_revert(self): # pylint: disable=too-many-locals settle() filepath = f"/dev/stratis/{pool_name}/{fs_name}" - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) file1 = "file1.txt" write_file(mountdir, file1) @@ -119,7 +122,7 @@ def test_revert(self): # pylint: disable=too-many-locals write_file(mountdir, file2) Filesystem.Properties.MergeScheduled.Set(get_object(snap_object_path), True) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}")) @@ -131,12 +134,12 @@ def test_revert(self): # pylint: disable=too-many-locals settle() - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) self.read_file(mountdir, file1) self.read_file(mountdir, file2) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) # Now stop the pool, which should tear down the devices (_, return_code, message) = Manager.Methods.StopPool( @@ -167,14 +170,14 @@ def test_revert(self): # pylint: disable=too-many-locals settle() - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) self.read_file(mountdir, file1) self.assertFalse(os.path.exists(os.path.join(mountdir, file2))) self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name}")) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals """ @@ -211,7 +214,7 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals settle() filepath = f"/dev/stratis/{pool_name}/{fs_name}" - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) file1 = "file1.txt" write_file(mountdir, file1) @@ -233,7 +236,7 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals Filesystem.Properties.MergeScheduled.Set( get_object(snap_object_path_1), True ) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir]) self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}")) @@ -285,7 +288,7 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals settle() - subprocess.check_call(["mount", filepath, mountdir]) + subprocess.check_call([_MOUNT, filepath, mountdir]) self.read_file(mountdir, file1) @@ -293,4 +296,4 @@ def test_revert_snapshot_chain(self): # pylint: disable=too-many-locals self.assertFalse(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_1}")) self.assertTrue(os.path.exists(f"/dev/stratis/{pool_name}/{snap_name_2}")) - subprocess.check_call(["umount", mountdir]) + subprocess.check_call([_UMOUNT, mountdir])