From 8cdf086c61d2a7995a982730298b5c4d28f3a8e2 Mon Sep 17 00:00:00 2001 From: Peter Harris Date: Tue, 7 Jan 2025 12:14:05 +0000 Subject: [PATCH] Code review cleanup --- .github/workflows/build_test.yaml | 5 ++ generator/generate_vulkan_common.py | 2 +- lgl_host_server.py | 2 +- lglpy/android/adb.py | 33 +++++++------ lglpy/android/filesystem.py | 39 +++++++++------ lglpy/android/test.py | 43 ++++++++++++++++- lglpy/android/utils.py | 75 ++++++++++++++--------------- lglpy/ui/test.py | 4 +- pylint.sh | 2 +- pytest_local.bat | 52 ++++++++++++++++++++ pytest_local.sh | 34 +++++++++++++ 11 files changed, 217 insertions(+), 74 deletions(-) create mode 100644 pytest_local.bat create mode 100755 pytest_local.sh diff --git a/.github/workflows/build_test.yaml b/.github/workflows/build_test.yaml index ce20eaa..cb1377d 100644 --- a/.github/workflows/build_test.yaml +++ b/.github/workflows/build_test.yaml @@ -39,6 +39,11 @@ jobs: python3 -m mypy ./lglpy python3 -m mypy ./generator + - name: Run unit tests + # Note: Only run tests that do not require a connected device + run: | + python3 -m lglpy.ui.test + build-ubuntu-x64-clang: name: Ubuntu x64 Clang runs-on: ubuntu-22.04 diff --git a/generator/generate_vulkan_common.py b/generator/generate_vulkan_common.py index 07d6976..9bf554e 100755 --- a/generator/generate_vulkan_common.py +++ b/generator/generate_vulkan_common.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 # SPDX-License-Identifier: MIT # ----------------------------------------------------------------------------- -# Copyright (c) 2024 Arm Limited +# Copyright (c) 2024-2025 Arm Limited # # Permission is hereby granted, free of charge, to any person obtaining a copy # of this software and associated documentation files (the "Software"), to diff --git a/lgl_host_server.py b/lgl_host_server.py index 5154d11..142c7ff 100755 --- a/lgl_host_server.py +++ b/lgl_host_server.py @@ -1,7 +1,7 @@ #!/bin/env python3 # SPDX-License-Identifier: MIT # ----------------------------------------------------------------------------- -# Copyright (c) 2024 Arm Limited +# Copyright (c) 2024-2025 Arm Limited # # Permission is hereby granted, free of charge, to any person obtaining a copy # of this software and associated documentation files (the 'Software'), to diff --git a/lglpy/android/adb.py b/lglpy/android/adb.py index f893fe3..09e1fce 100644 --- a/lglpy/android/adb.py +++ b/lglpy/android/adb.py @@ -38,6 +38,17 @@ class ADBConnect: A wrapper around adb which can be used to connect to a specific device, and run commands as a specific package. + - adb() runs a command using adb and waits for the result. + - adb_async() runs a command using adb and does not wait for the result. + - adb_run() runs a device shell command as the "shell" user and waits for + the result. + - adb_runas() runs a device shell command as the current package user and + waits for the result. + + The current device and package are attributes of the connection instance + which can be set at construction, or via the set_device() and set_package() + methods. + Attributes: device: The name of the connected device, or None for generic use. package: The name of the debuggable package, or None for generic use. @@ -159,9 +170,7 @@ def adb(self, *args: str, text: bool = True, shell: bool = False, # Invoke the command rep = sp.run(packed_commands, check=check, shell=shell, text=text, - stdin=sp.DEVNULL, - stdout=sp.PIPE, - stderr=sp.PIPE) + stdin=sp.DEVNULL, stdout=sp.PIPE, stderr=sp.PIPE) # Return the output return rep.stdout @@ -207,9 +216,7 @@ def adb_async(self, *args: str, text: bool = True, shell: bool = False, # pylint: disable=consider-using-with process = sp.Popen(packed_commands, text=text, shell=shell, - stdin=sp.DEVNULL, - stdout=output, - stderr=sp.DEVNULL) + stdin=sp.DEVNULL, stdout=output, stderr=sp.DEVNULL) # Return the output process a user can use to wait, if needed. return process @@ -244,10 +251,9 @@ def adb_run(self, *args: str, text: bool = True, shell: bool = False, packed_commands = self.pack_commands(commands, shell, quote) # Invoke the command - rep = sp.run(packed_commands, check=check, shell=shell, text=text, - stdin=sp.DEVNULL, - stdout=sp.PIPE, - stderr=sp.PIPE) + rep = sp.run(packed_commands, + check=check, shell=shell, text=text, + stdin=sp.DEVNULL, stdout=sp.PIPE, stderr=sp.PIPE) # Return the output return rep.stdout @@ -285,10 +291,9 @@ def adb_runas(self, *args: str, text: bool = True, shell: bool = False, packed_commands = self.pack_commands(commands, shell, quote) # Invoke the command - rep = sp.run(packed_commands, check=check, shell=shell, text=text, - stdin=sp.DEVNULL, - stdout=sp.PIPE, - stderr=sp.PIPE) + rep = sp.run(packed_commands, + check=check, shell=shell, text=text, + stdin=sp.DEVNULL, stdout=sp.PIPE, stderr=sp.PIPE) # Return the output return rep.stdout diff --git a/lglpy/android/filesystem.py b/lglpy/android/filesystem.py index 14ad82d..8c57c55 100644 --- a/lglpy/android/filesystem.py +++ b/lglpy/android/filesystem.py @@ -22,8 +22,8 @@ # ----------------------------------------------------------------------------- ''' -This module implements higher level Android queries and utilities, built on top -of the low level Android Debug Bridge wrapper. +This module implements higher level Android filesystem utilities, built on top +of the low level ADBConnect wrapper around Android Debug Bridge. ''' import os @@ -36,6 +36,11 @@ class AndroidFilesystem: ''' A library of utility methods for transferring files to/from a device. + + Attributes: + TEMP_DIR: The generally accessible device temp directory. + DATA_PERM: The file permissions to use for data files. + EXEC_PERM: The file permissions to use for executable files. ''' TEMP_DIR = '/data/local/tmp' @@ -49,12 +54,12 @@ def push_file_to_tmp( ''' Push a file to the device temp directory. - File will be copied to: /data/local/tmp/. + File will be copied to: TEMP_DIR/. Args: conn: The adb connection. host_path: The path of the file on the host file system. - executable: True if the file should be configured as executable. + executable: True if the file should have executable permissions. Returns: True if the file was copied, False otherwise. @@ -65,16 +70,17 @@ def push_file_to_tmp( try: # Remove old file to prevent false success - conn.adb('shell', 'rm', '-f', device_path) + conn.adb_run('rm', '-f', device_path) # Push new file conn.adb('push', host_path, device_path) # Check it actually copied - conn.adb('shell', 'ls', device_path) + conn.adb_run('ls', device_path) permission = cls.EXEC_PERM if executable else cls.DATA_PERM - conn.adb('shell', 'chmod', permission, device_path) + conn.adb_run('chmod', permission, device_path) + except sp.CalledProcessError: return False @@ -94,7 +100,7 @@ def pull_file_from_tmp( file_name: The name of the file in the tmp directory. host_path: The destination directory on the host file system. Host directory will be created if it doesn't exist. - delete: Delete the file on the device after copying it. + delete: Should the file on the device be deleted after copying? Returns: True if the file was copied, False otherwise. @@ -121,7 +127,7 @@ def delete_file_from_tmp( ''' Delete a file from the device temp directory. - File will be deleted from: /data/local/tmp/. + File will be deleted from: TEMP_DIR/. Args: conn: The adb connection. @@ -135,9 +141,9 @@ def delete_file_from_tmp( try: if error_ok: - conn.adb('shell', 'rm', '-f', device_path) + conn.adb_run('rm', '-f', device_path) else: - conn.adb('shell', 'rm', device_path) + conn.adb_run('rm', device_path) except sp.CalledProcessError: return False @@ -155,7 +161,7 @@ def push_file_to_package( Args: conn: The adb connection. host_path: The path of the file on the host file system. - executable: True if the file should be configured as executable. + executable: True if the file should have executable permissions. Returns: True if the file was copied, False otherwise. @@ -197,7 +203,7 @@ def pull_file_from_package( src_file: The name of the file in the tmp directory. host_path: The destination directory on the host file system. Host directory will be created if it doesn't exist. - delete: Delete the file on the device after copying it. + delete: Should the file on the device be deleted after copying? Returns: True if the file was copied, False otherwise. @@ -218,6 +224,7 @@ def pull_file_from_package( if delete: cls.delete_file_from_package(conn, src_file) + except sp.SubprocessError: return False @@ -227,13 +234,13 @@ def pull_file_from_package( def rename_file_in_package( cls, conn: ADBConnect, file_name: str, new_file_name: str) -> bool: ''' - Delete a file from the package directory. + Rename a file in the package directory. - File will be deleted from, e.g.: /data/user/0// + File will be renamed to, e.g.: /data/user/0// Args: conn: The adb connection. - file_name: The name of the file to rename. + file_name: The name of the existing file to rename. new_file_name: The new file name to use. Returns: diff --git a/lglpy/android/test.py b/lglpy/android/test.py index 134c00b..92ae605 100644 --- a/lglpy/android/test.py +++ b/lglpy/android/test.py @@ -40,7 +40,8 @@ from .utils import AndroidUtils from .filesystem import AndroidFilesystem -SLOW_TESTS = False # Set to True to enable slot tests, False to skip them + +SLOW_TESTS = True # Set to True to enable slow tests, False to skip them def get_script_relative_path(file_name: str) -> str: @@ -564,6 +565,46 @@ def test_util_copy_to_package_exec(self): success = AndroidFilesystem.delete_file_from_package(conn, test_file) self.assertTrue(success) + def test_util_rename_in_package(self): + ''' + Test filesystem rename in package data directory. + ''' + conn = ADBConnect() + + # Fetch some packages that we can use + packages = AndroidUtils.get_packages(conn, True, False) + self.assertGreater(len(packages), 0) + conn.set_package(packages[0]) + + test_file = 'test_data.txt' + test_path = get_script_relative_path(test_file) + + # Push the file + success = AndroidFilesystem.push_file_to_package(conn, test_path) + self.assertTrue(success) + + # Validate it pushed OK + data = conn.adb_runas('ls', test_file) + self.assertEqual(data.strip(), test_file) + + # Rename the file + new_test_file = 'test_data_2.txt' + success = AndroidFilesystem.rename_file_in_package( + conn, test_file, new_test_file) + self.assertTrue(success) + + # Validate it was moved - this should fail + with self.assertRaises(sp.CalledProcessError): + data = conn.adb_runas('ls', test_file) + + data = conn.adb_runas('ls', new_test_file) + self.assertEqual(data.strip(), new_test_file) + + # Cleanup tmp - this should fail because the file does not exist + success = AndroidFilesystem.delete_file_from_package( + conn, new_test_file) + self.assertTrue(success) + def test_util_copy_from_package(self): ''' Test filesystem copy from package data directory to host. diff --git a/lglpy/android/utils.py b/lglpy/android/utils.py index 60153f8..e2e4c9e 100644 --- a/lglpy/android/utils.py +++ b/lglpy/android/utils.py @@ -22,8 +22,8 @@ # ----------------------------------------------------------------------------- ''' -This module implements higher level Android queries and utilities, built on top -of the low level Android Debug Bridge wrapper. +This module implements higher level Android queries and utility functions, +built on top of the low level ADBConnect wrapper around Android Debug Bridge. ''' import re @@ -36,7 +36,7 @@ class AndroidUtils: ''' - A library of utility methods for querying device and package status. + A library of utility methods for querying device and package configuration. ''' @staticmethod @@ -104,7 +104,7 @@ def get_packages( command += f' | xargs -n1 sh -c {shlex.quote(sub)} 2> /dev/null' try: - package_list = conn.adb('shell', command).splitlines() + package_list = conn.adb_run(command).splitlines() except sp.CalledProcessError: return [] @@ -115,8 +115,8 @@ def get_packages( return package_list - @staticmethod - def get_os_version(conn: ADBConnect) -> Optional[str]: + @classmethod + def get_os_version(cls, conn: ADBConnect) -> Optional[str]: ''' Get the Android OS platform version of the target device. @@ -127,13 +127,12 @@ def get_os_version(conn: ADBConnect) -> Optional[str]: The Android platform version, or None on error. ''' try: - ver = conn.adb('shell', 'getprop', 'ro.build.version.release') - return ver + return cls.get_property(conn, 'ro.build.version.release') except sp.CalledProcessError: return None - @staticmethod - def get_os_sdk_version(conn: ADBConnect) -> Optional[int]: + @classmethod + def get_os_sdk_version(cls, conn: ADBConnect) -> Optional[int]: ''' Get the Android OS SDK version of the target device. @@ -143,14 +142,13 @@ def get_os_sdk_version(conn: ADBConnect) -> Optional[int]: Returns: The Android SDK version number, or None on error. ''' - try: - ver = conn.adb('shell', 'getprop', 'ro.build.version.sdk') - return int(ver) - except sp.CalledProcessError: + ver = cls.get_property(conn, 'ro.build.version.sdk') + if not ver: return None + return int(ver) - @staticmethod - def get_device_model(conn: ADBConnect) -> Optional[tuple[str, str]]: + @classmethod + def get_device_model(cls, conn: ADBConnect) -> Optional[tuple[str, str]]: ''' Get the vendor and model of the target device. @@ -160,17 +158,17 @@ def get_device_model(conn: ADBConnect) -> Optional[tuple[str, str]]: Returns: The device vendor and model strings, or None on error. ''' - try: - vendor = conn.adb('shell', 'getprop', 'ro.product.manufacturer') - vendor = vendor.strip() - - model = conn.adb('shell', 'getprop', 'ro.product.model') - model = model.strip() - - return (vendor, model) + vendor = cls.get_property(conn, 'ro.product.manufacturer') + if not vendor: + return None + vendor = vendor.strip() - except sp.CalledProcessError: + model = cls.get_property(conn, 'ro.product.model') + if not model: return None + model = model.strip() + + return (vendor, model) @staticmethod def is_package_debuggable(conn: ADBConnect, package: str) -> bool: @@ -187,14 +185,14 @@ def is_package_debuggable(conn: ADBConnect, package: str) -> bool: try: package = shlex.quote(package) command = f'if run-as {package} true ; then echo {package} ; fi' - log = conn.adb('shell', command) + log = conn.adb_run(command) return log.strip() == package except sp.CalledProcessError: return False - @staticmethod - def is_package_32bit(conn: ADBConnect, package: str) -> bool: + @classmethod + def is_package_32bit(cls, conn: ADBConnect, package: str) -> bool: ''' Test if a package prefers 32-bit ABI on the target device. @@ -211,7 +209,7 @@ def is_package_32bit(conn: ADBConnect, package: str) -> bool: # Try to match the primary ABI loaded by the application package = shlex.quote(package) command = f'pm dump {package} | grep primaryCpuAbi' - log = conn.adb('shell', command) + log = conn.adb_run(command) pattern = re.compile('primaryCpuAbi=(\\S+)') match = pattern.search(log) @@ -220,9 +218,10 @@ def is_package_32bit(conn: ADBConnect, package: str) -> bool: if log_abi != 'null': preferred_abi = log_abi - # If that fails match against the default device ABI + # If that fails match against the default system ABI if preferred_abi is None: - preferred_abi = conn.adb('shell', 'getprop ro.product.cpu.abi') + sys_abi = cls.get_property(conn, 'getprop ro.product.cpu.abi') + preferred_abi = sys_abi return preferred_abi in ('armeabi-v7a', 'armeabi') @@ -250,7 +249,7 @@ def get_package_data_dir(conn: ADBConnect): try: package = shlex.quote(conn.package) command = f'dumpsys package {package} | grep dataDir' - log = conn.adb('shell', command) + log = conn.adb_run(command) return log.replace('dataDir=', '').strip() except sp.CalledProcessError: @@ -270,7 +269,7 @@ def set_property(conn: ADBConnect, prop: str, value: str) -> bool: True on success, False otherwise. ''' try: - conn.adb('shell', 'setprop', prop, value) + conn.adb_run('setprop', prop, value) return True except sp.CalledProcessError: @@ -290,7 +289,7 @@ def get_property(conn: ADBConnect, prop: str) -> Optional[str]: deleted settings that do not exist will also return None. ''' try: - value = conn.adb('shell', 'getprop', prop) + value = conn.adb_run('getprop', prop) return value.strip() except sp.CalledProcessError: @@ -309,7 +308,7 @@ def clear_property(conn: ADBConnect, prop: str) -> bool: True on success, False otherwise. ''' try: - conn.adb('shell', 'setprop', prop, '""') + conn.adb_run('setprop', prop, '""') return True except sp.CalledProcessError: @@ -329,7 +328,7 @@ def set_setting(conn: ADBConnect, setting: str, value: str) -> bool: True on success, False otherwise. ''' try: - conn.adb('shell', 'settings', 'put', 'global', setting, value) + conn.adb_run('settings', 'put', 'global', setting, value) return True except sp.CalledProcessError: @@ -348,7 +347,7 @@ def get_setting(conn: ADBConnect, setting: str) -> Optional[str]: The value of the setting on success, None otherwise. ''' try: - value = conn.adb('shell', 'settings', 'get', 'global', setting) + value = conn.adb_run('settings', 'get', 'global', setting) value = value.strip() if value == 'null': @@ -372,7 +371,7 @@ def clear_setting(conn: ADBConnect, setting: str) -> bool: True on success, False otherwise. ''' try: - conn.adb('shell', 'settings', 'delete', 'global', setting) + conn.adb_run('settings', 'delete', 'global', setting) return True except sp.CalledProcessError: diff --git a/lglpy/ui/test.py b/lglpy/ui/test.py index 5978895..05acaef 100644 --- a/lglpy/ui/test.py +++ b/lglpy/ui/test.py @@ -77,9 +77,9 @@ def test_menu_select_bad_range(self, mock_get_input): self.assertEqual(selected_option, 1) @mock.patch('lglpy.ui.console.get_input', side_effect=['fox', '3']) - def test_menu_select_bad_formant(self, mock_get_input): + def test_menu_select_bad_formnt(self, mock_get_input): ''' - Test the user entering an out-of-bounds value in the menu. + Test the user entering a non-integer value in the menu. ''' options = self.make_options(3) selected_option = console.select_from_menu('Title', options) diff --git a/pylint.sh b/pylint.sh index bc92af7..14fee35 100755 --- a/pylint.sh +++ b/pylint.sh @@ -1,6 +1,6 @@ # SPDX-License-Identifier: MIT # ----------------------------------------------------------------------------- -# Copyright (c)2025 Arm Limited +# Copyright (c) 2025 Arm Limited # # Permission is hereby granted, free of charge, to any person obtaining a copy # of this software and associated documentation files (the 'Software'), to diff --git a/pytest_local.bat b/pytest_local.bat new file mode 100644 index 0000000..a1b0109 --- /dev/null +++ b/pytest_local.bat @@ -0,0 +1,52 @@ +:: SPDX-License-Identifier: MIT +:: ----------------------------------------------------------------------------- +:: Copyright (c) 2025 Arm Limited +:: +:: Permission is hereby granted, free of charge, to any person obtaining a copy +:: of this software and associated documentation files (the 'Software'), to +:: deal in the Software without restriction, including without limitation the +:: rights to use, copy, modify, merge, publish, distribute, sublicense, and/or +:: sell copies of the Software, and to permit persons to whom the Software is +:: furnished to do so, subject to the following conditions: +:: +:: The above copyright notice and this permission notice shall be included in +:: all copies or substantial portions of the Software. +:: +:: THE SOFTWARE IS PROVIDED 'AS IS', WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +:: IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +:: FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +:: AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +:: LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +:: OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +:: SOFTWARE. +:: ----------------------------------------------------------------------------- +@echo off +setlocal + +set total_errors = 0 +set total_runs = 0 + +echo = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = +echo Running tests without devices +echo = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = +echo: + +py -3 -m lglpy.ui.test +set /a total_runs+=1 +set /a total_errors=total_errors+%ERRORLEVEL% + +echo = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = +echo Running tests with devices +echo = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = +echo: + +py -3 -m lglpy.android.test +set /a total_runs+=1 +set /a total_errors=total_errors+%ERRORLEVEL% + +echo = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = +echo Test run summary statistics +echo = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = +echo: +echo - Total suites run = %total_runs% +echo - Total suites in error = %total_errors% diff --git a/pytest_local.sh b/pytest_local.sh new file mode 100755 index 0000000..de90237 --- /dev/null +++ b/pytest_local.sh @@ -0,0 +1,34 @@ +# SPDX-License-Identifier: MIT +# ----------------------------------------------------------------------------- +# Copyright (c) 2025 Arm Limited +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the 'Software'), to +# deal in the Software without restriction, including without limitation the +# rights to use, copy, modify, merge, publish, distribute, sublicense, and/or +# sell copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED 'AS IS', WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. +# ----------------------------------------------------------------------------- + +printf "= = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = \n" +printf "Running tests without devices\n" +printf "= = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = \n" +printf "\n" +python3 -m lglpy.ui.test + +printf "= = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = \n" +printf "Running tests with devices\n" +printf "= = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = = \n" +printf "\n" +python3 -m lglpy.android.test