From ecb8245dbdfea12107cee9f70e8dc8438f2d2bef Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Thu, 27 Jun 2024 11:49:18 +0800 Subject: [PATCH 01/36] feat: initial migration --- providers/base/bin/reboot_check_test.py | 403 ++++++++++++++++++++++++ providers/base/units/stress/boot.pxu | 10 +- 2 files changed, 408 insertions(+), 5 deletions(-) create mode 100755 providers/base/bin/reboot_check_test.py diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py new file mode 100755 index 0000000000..81c0f8fad1 --- /dev/null +++ b/providers/base/bin/reboot_check_test.py @@ -0,0 +1,403 @@ +#!/usr/bin/env python3 + +# TODO: Match python 3.5 syntax + + +import argparse +import os +import subprocess +import re +import shutil +import filecmp +import sys +import typing as T +from subprocess import PIPE + + +# Checkbox could run in a snap container, so we need to prepend this root path +RUNTIME_ROOT = os.getenv("CHECKBOX_RUNTIME", default="") +# Snap mount point, see https://snapcraft.io/docs/environment-variables#heading--snap +SNAP = os.getenv("SNAP", default="") + + +def is_fwts_supported() -> bool: + return shutil.which("fwts") is not None + + +def fwts_log_check_passed(output_directory: str) -> bool: + """Check if fwts logs passes the checks specified in sleep_test_log_check.py. + This script live in the same directory + + :param output_directory: where the output of fwts should be redirected to + :type output_directory: str + :return: whether sleep_test_log_check.py returned 0 (success) + :rtype: bool + """ + log_file_path = "{}/fwts_klog_oops.log".format(output_directory) + subprocess.run( + [ + "fwts", + "-r", + log_file_path, + "klog", + "oops", + ] + ) + result = subprocess.run( + [ + "./sleep_test_log_check.py", + "-v", + "--ignore-warning", + "-t", + "all", + log_file_path, + ] + ) + + return result.returncode == 0 + + +def compare_device_lists(expected_dir: str, actual_dir: str) -> bool: + """Compares the list of devices in expected_dir against actual_dir + + :param expected_dir: a directory of files containing the expected values the device list + :param actual_dir: a directory of files containing the actual device list + :return: whether the device list matches + """ + print("Comparing devices...") + for name in ["lspci", "iw", "lsusb"]: + # file paths of the expected and actual device lists + expected = "{}/{}_log".format(expected_dir, name) + actual = "{}/{}_log".format(actual_dir, name) + if not filecmp.cmp(expected, actual): + print( + "The output of {} differs from the original list gathered at the beginning of the session!".format( + name + ) + ) + return False + + return True + + +def get_failed_services() ->T.List[str]: + """Counts the number of failed services listed in systemctl + + :return: number of failed services + :rtype: int + """ + command = [ + "systemctl", + "list-units", + "--system", + "--no-ask-password", + "--no-pager", + "--no-legend", + "--state=failed", + ] # only print the names of the services that failed + output = subprocess.run(command).stdout.decode() + + return output.split() + + +def dump_device_info(output_directory: str) -> None: + """Writes information of PCI, wireless, and USB devices to the specified directory + + :param output_directory: where the output should be written to. + If this directory doesn't already exist, this function will create it. + :type output_directory: str + """ + + print("Dumping the devices information to {}".format(output_directory)) + + # specifying exist_ok=True for now to mimic mkdir -p's behavior, + # TODO: check if this is intended + os.makedirs(output_directory, exist_ok=True) + + print("Checking PCI devices...") + with open("{}/lspci_log".format(output_directory), "w") as f: + lspci_out = subprocess.run( + ["lspci", "-i", "{}/usr/share/misc/pci.ids".format(SNAP)], + stdout=PIPE, + stderr=PIPE, + ) + f.write(lspci_out.stdout.decode()) + + print("Checking wireless connections...") + with open("{}/iw_log".format(output_directory), "w") as f: + iw_out = subprocess.run(["iw", "dev"], stdout=PIPE, stderr=PIPE) + lines = iw_out.stdout.decode().splitlines() + lines_to_write = list( + filter( + lambda line: "addr" in line + or "Interface" in line + or "ssid" in line, + sorted(lines), + ) + ) + f.write("\n".join(map(lambda line: line.strip(), lines_to_write))) + + print("Checking USB devices...") + with open("{}/lsusb_log".format(output_directory), "w") as f: + lsusb_out = subprocess.run( + [ + "checkbox-support-lsusb", + "-f", + '"{}"/var/lib/usbutils/usb.ids'.format(RUNTIME_ROOT), + "-s", + ], + stdout=PIPE, + stderr=PIPE, + ) + f.write(lsusb_out.stdout.decode()) + + os.sync() # force disk write + print("Finished dumping device info!") + + +def parse_arguments(): + parser = argparse.ArgumentParser( + prog="Reboot tests", + description="This script is used to collect device information and to check for differences between reboots.", + ) + parser.add_argument( + "-d", + dest="output_directory", + help="Absolute path to the output directory", + ) + parser.add_argument( + "-c", + dest="comparison_directory", + help="Absolute path to the comparison directory", + ) + parser.add_argument( + "-s", + default=False, + dest="do_service_check", + action="store_true", + help="Whether the script should check if all system services are running", + ) + parser.add_argument( + "-f", + default=False, + dest="do_fwts_check", + action="store_true", + help="Whether the script should look for fwts log errors", + ) + parser.add_argument( + "-g", + default=False, + dest="do_renderer_check", + action="store_true", + help="Whether the script should check if hardware rendering is being used", + ) + + return parser.parse_args() # type: ignore , checked manually + + +def remove_color_code(string: str) -> str: + """Removes ANSI color escape sequences from string + + :param string: the string that you would like to remove color code + credit: Hanhsuan Lee + """ + return re.sub(r"\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])", "", string) + + +def get_display_id() -> T.Optional[str]: + """Returns the active display id + https://github.com/canonical/checkbox/blob/main/contrib/pc-sanity/bin/renderer-mesa-driver-check.sh + + :return: the display id, usually ":0" if there's only 1 display + :rtype: str + """ + DISPLAY = os.getenv("DISPLAY", default="") + XDG_SESSION_TYPE = os.getenv("XDG_SESSION_TYPE", default="") + + if DISPLAY != "": + return DISPLAY # use the environment var if non-empty + + session_id = ( + subprocess.run(["loginctl", "list-sessions", "--no-legend"]).stdout + .decode() + .split()[0] + .strip() + # string is guaranteed to be non-empty, at least 1 user is logged in + ) + + display_server_type = ( + subprocess.run( + ["loginctl", "show-session", session_id, "-p", "Type"] + ) + .stdout + .decode() + .split("=")[1] + .strip() # it should look like Type=wayland, split and take 2nd word + ) + + print("{} =? {}".format(XDG_SESSION_TYPE, display_server_type)) + + if display_server_type == "wayland": + # search for a process called xwayland, take the 3rd arg, which is the display id + return ( + subprocess.run(["pgrep", "-a", 'Xwayland']) + .stdout + .decode() + .split()[2] + ) + + if display_server_type == "x11": + return ( + subprocess.run(["w", "--no-header"]).stdout.decode().split()[2] + ) + + return None # Unsupported window system + + +def has_DRM_file_nodes() -> bool: + """Checks of there's anything user/sys/class/drm + + :return: True if there are more items than just "version" + """ + + # look for GPU file nodes first + possible_gpu_nodes = os.listdir("/sys/class/drm") + if len(possible_gpu_nodes) == 0 or possible_gpu_nodes == ["version"]: + # kernel doesn't see any GPU nodes + print( + "There's nothing under /sys/class/drm", + "if an external GPU is connected, check if the connection is loose", + ) + return False + + print("These nodes", possible_gpu_nodes, "exist") + return True + + +def is_hardware_renderer_available() -> bool: + """Checks if hardware rendering is being used + + :return: True if a hardware renderer is active or none exists, otherwise return False + :rtype: bool + """ + + # Now we know some kind of display exists, run unity_support_test + display_id = get_display_id() + print("Checking display id: {}".format(display_id)) + if display_id is None: + # No display id was found + return False + + unity_support_output = subprocess.run( + [ + "{}/usr/lib/nux/unity_support_test".format(RUNTIME_ROOT), + "-p", + "-display", + display_id, + ], + stdout=PIPE, + stderr=PIPE, + ) + if unity_support_output.returncode != 0: + return False + + is_hardware_rendered = ( + parse_unity_support_output(unity_support_output.stdout.decode()).get( + "Not software rendered" + ) + == "yes" + ) + if not is_hardware_rendered: + return False + + print("This machine is using a hardware renderer!") + return True + + +def parse_unity_support_output(output_string: str) -> T.Dict[str, str]: + """Parses the output of `unity_support_test` into a dictionary + + :param output_string: the raw output from running `unity_support_test -p` + :type output_string: str + :return: string key-value pairs that mirror the output of unity_support_test. + Left hand side of the first colon are the keys; right hand side are the values. + :rtype: dict[str, str] + """ + + output = {} # type: dict[str, str] + for line in output_string.split("\n"): + # max_split=1 to prevent splitting the string after the 1st colon + words = line.split(":", maxsplit=1) + if len(words) == 2: + key = words[0] + value = remove_color_code(words[1].strip()) + output[key] = value + + return output + + +def main() -> int: + """Main routine + + :return: an return code for checkbox to consume, 1 = failed, 0 = success + :rtype: int + """ + + args = parse_arguments() + + fwts_passed = False + device_comparison_passed=False + has_failed_services=False + + + + if args.output_directory is not None: + dump_device_info(args.output_directory) + supports_fwts = is_fwts_supported() + if supports_fwts and not fwts_log_check_passed(args.output_directory): + return 1 + + if args.comparison_directory is not None: + if args.output_directory is None: + print( + "Error: Please specify an output directory with the -d flag.", + file=sys.stderr, + ) + else: + compare_device_lists( + args.comparison_directory, args.output_directory + ) + + if args.do_service_check: + print("Checking for failed system services...") + failed_services = get_failed_services() + if len(failed_services) > 0: + print("These services failed: {}".format(failed_services), file=sys.stderr) + return 1 + + if args.do_fwts_check and ( + args.output_directory is None + or not fwts_log_check_passed(args.output_directory) + ): + return 1 + + if args.do_renderer_check: + if not has_DRM_file_nodes(): + return 0 # skip gpu test if there's no GPU + return 0 if is_hardware_renderer_available() else 1 + + # if [[ -n "$service_opt" && "$service_check" == "false" ]] || \ + # [[ -n "$compare_dir" && "$device_check" == "false" ]] || \ + # [[ -n "$fwts_opt" && "$fwts_check" == "false" ]]; then + # exit 1 + # fi + + return 0 + + +if __name__ == "__main__": + print(os.getcwd()) + return_code = main() + exit(return_code) + # dump_device_info("testoo") + # is_hardware_renderer_available() diff --git a/providers/base/units/stress/boot.pxu b/providers/base/units/stress/boot.pxu index 71dbb4dbb5..f2b4a00552 100644 --- a/providers/base/units/stress/boot.pxu +++ b/providers/base/units/stress/boot.pxu @@ -46,7 +46,7 @@ _purpose: This creates baseline data sets which will be considered the master unit: job plugin: shell command: - reboot_check_test.sh -d "$PLAINBOX_SESSION_SHARE/before_reboot" + reboot_check_test.py -g -d "$PLAINBOX_SESSION_SHARE/before_reboot" environ: LD_LIBRARY_PATH user: root estimated_duration: 1s @@ -94,7 +94,7 @@ unit: job plugin: shell environ: LD_LIBRARY_PATH command: - reboot_check_test.sh -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/cold_reboot_cycle1" -s -f + reboot_check_test.py -g -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/cold_reboot_cycle1" -s -f user: root flags: preserve-locale estimated_duration: 1.0 @@ -111,7 +111,7 @@ template-unit: job plugin: shell environ: LD_LIBRARY_PATH command: - reboot_check_test.sh -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/cold_reboot_cycle{reboot_id}" -s -f + reboot_check_test.py -g -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/cold_reboot_cycle{reboot_id}" -s -f user: root flags: preserve-locale estimated_duration: 1.0 @@ -158,7 +158,7 @@ unit: job plugin: shell environ: LD_LIBRARY_PATH command: - reboot_check_test.sh -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/warm_reboot_cycle1" -s -f + reboot_check_test.py -g -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/warm_reboot_cycle1" -s -f -g user: root flags: preserve-locale estimated_duration: 1.0 @@ -175,7 +175,7 @@ template-unit: job plugin: shell environ: LD_LIBRARY_PATH command: - reboot_check_test.sh -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/warm_reboot_cycle{reboot_id}" -s -f + reboot_check_test.py -g -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/warm_reboot_cycle{reboot_id}" -s -f user: root flags: preserve-locale estimated_duration: 1.0 From 9071c654e4683ac8910be134d16b0b8da4f62ec8 Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Thu, 27 Jun 2024 15:00:08 +0800 Subject: [PATCH 02/36] feat: created a wrapper for subprocess.run() --- providers/base/bin/reboot_check_test.py | 165 +++++++++++++----------- 1 file changed, 90 insertions(+), 75 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 81c0f8fad1..97915983f3 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -11,7 +11,6 @@ import filecmp import sys import typing as T -from subprocess import PIPE # Checkbox could run in a snap container, so we need to prepend this root path @@ -20,12 +19,30 @@ SNAP = os.getenv("SNAP", default="") +class ShellResult: + def __init__(self, return_code: int, stdout: str, stderr: str) -> None: + self.return_code = return_code + self.stdout = stdout + self.stderr = stderr + + +def run_command(args: T.List[str]) -> ShellResult: + # PIPE is needed for subprocess.run to capture stdout and stderr (<=3.7 behavior) + out = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + return ShellResult( + return_code=out.returncode, + # if there's nothing on stdout, .stdout is None, so we need a default value + stdout=(out.stdout or b"").decode(), + stderr=(out.stderr or b"").decode(), + ) # This could throw on non-UTF8 decodable byte strings, but that should be rare + + def is_fwts_supported() -> bool: return shutil.which("fwts") is not None def fwts_log_check_passed(output_directory: str) -> bool: - """Check if fwts logs passes the checks specified in sleep_test_log_check.py. + """Check if fwts logs passes the checks specified in sleep_test_log_check.py. This script live in the same directory :param output_directory: where the output of fwts should be redirected to @@ -45,7 +62,7 @@ def fwts_log_check_passed(output_directory: str) -> bool: ) result = subprocess.run( [ - "./sleep_test_log_check.py", + "sleep_test_log_check.py", "-v", "--ignore-warning", "-t", @@ -80,11 +97,10 @@ def compare_device_lists(expected_dir: str, actual_dir: str) -> bool: return True -def get_failed_services() ->T.List[str]: +def get_failed_services() -> T.List[str]: """Counts the number of failed services listed in systemctl :return: number of failed services - :rtype: int """ command = [ "systemctl", @@ -95,9 +111,8 @@ def get_failed_services() ->T.List[str]: "--no-legend", "--state=failed", ] # only print the names of the services that failed - output = subprocess.run(command).stdout.decode() - return output.split() + return run_command(command).stdout.split() def dump_device_info(output_directory: str) -> None: @@ -116,17 +131,15 @@ def dump_device_info(output_directory: str) -> None: print("Checking PCI devices...") with open("{}/lspci_log".format(output_directory), "w") as f: - lspci_out = subprocess.run( + lspci_out = run_command( ["lspci", "-i", "{}/usr/share/misc/pci.ids".format(SNAP)], - stdout=PIPE, - stderr=PIPE, ) - f.write(lspci_out.stdout.decode()) + f.write(lspci_out.stdout) print("Checking wireless connections...") with open("{}/iw_log".format(output_directory), "w") as f: - iw_out = subprocess.run(["iw", "dev"], stdout=PIPE, stderr=PIPE) - lines = iw_out.stdout.decode().splitlines() + iw_out = run_command(["iw", "dev"]) + lines = iw_out.stdout.splitlines() lines_to_write = list( filter( lambda line: "addr" in line @@ -139,17 +152,15 @@ def dump_device_info(output_directory: str) -> None: print("Checking USB devices...") with open("{}/lsusb_log".format(output_directory), "w") as f: - lsusb_out = subprocess.run( + lsusb_out = run_command( [ "checkbox-support-lsusb", "-f", '"{}"/var/lib/usbutils/usb.ids'.format(RUNTIME_ROOT), "-s", - ], - stdout=PIPE, - stderr=PIPE, + ] ) - f.write(lsusb_out.stdout.decode()) + f.write(lsusb_out.stdout) os.sync() # force disk write print("Finished dumping device info!") @@ -218,66 +229,73 @@ def get_display_id() -> T.Optional[str]: return DISPLAY # use the environment var if non-empty session_id = ( - subprocess.run(["loginctl", "list-sessions", "--no-legend"]).stdout - .decode() - .split()[0] + run_command(["loginctl", "list-sessions", "--no-legend"]) + .stdout.split()[0] .strip() # string is guaranteed to be non-empty, at least 1 user is logged in ) display_server_type = ( - subprocess.run( - ["loginctl", "show-session", session_id, "-p", "Type"] - ) - .stdout - .decode() - .split("=")[1] + run_command(["loginctl", "show-session", session_id, "-p", "Type"]) + .stdout.split("=")[1] .strip() # it should look like Type=wayland, split and take 2nd word ) print("{} =? {}".format(XDG_SESSION_TYPE, display_server_type)) if display_server_type == "wayland": - # search for a process called xwayland, take the 3rd arg, which is the display id + pgrep_out = run_command(["pgrep", "-a", "Xwayland"]) + # search for a process called Xwayland, take the 3rd arg, which is the display id return ( - subprocess.run(["pgrep", "-a", 'Xwayland']) - .stdout - .decode() - .split()[2] + pgrep_out.stdout.split()[2] if pgrep_out.return_code == 0 else None ) if display_server_type == "x11": - return ( - subprocess.run(["w", "--no-header"]).stdout.decode().split()[2] - ) + w_out = run_command(['w', '--no-header']) + return w_out.stdout.split()[2] if w_out.stdout != '' else None return None # Unsupported window system -def has_DRM_file_nodes() -> bool: - """Checks of there's anything user/sys/class/drm +def has_display_connection() -> bool: + """Checks if a display is connected. :return: True if there are more items than just "version" """ - + # look for GPU file nodes first - possible_gpu_nodes = os.listdir("/sys/class/drm") + DRM_PATH = "/sys/class/drm" + possible_gpu_nodes = os.listdir(DRM_PATH) if len(possible_gpu_nodes) == 0 or possible_gpu_nodes == ["version"]: # kernel doesn't see any GPU nodes print( - "There's nothing under /sys/class/drm", + "There's nothing under {}".format(DRM_PATH), "if an external GPU is connected, check if the connection is loose", ) return False print("These nodes", possible_gpu_nodes, "exist") - return True + print("Checking for display connection...") + + for gpu in possible_gpu_nodes: + # for each gpu, check for connection, return true if anything is connected + try: + status_file = open("{}/{}/status".format(DRM_PATH, gpu)) + if status_file.read().strip().lower() == "connected": + print("{} is connected to display!".format(gpu)) + return True + except FileNotFoundError: + pass # this just means we don't have a status file => no connection + except Exception as e: + print(e, file=sys.stderr) + + return False def is_hardware_renderer_available() -> bool: - """Checks if hardware rendering is being used + """Checks if hardware rendering is being used. THIS ASSUMES A DRM CONNECTION EXISTS - :return: True if a hardware renderer is active or none exists, otherwise return False + :return: True if a hardware renderer is active, otherwise return False :rtype: bool """ @@ -288,21 +306,19 @@ def is_hardware_renderer_available() -> bool: # No display id was found return False - unity_support_output = subprocess.run( + unity_support_output = run_command( [ "{}/usr/lib/nux/unity_support_test".format(RUNTIME_ROOT), "-p", "-display", display_id, - ], - stdout=PIPE, - stderr=PIPE, + ] ) - if unity_support_output.returncode != 0: + if unity_support_output.return_code != 0: return False is_hardware_rendered = ( - parse_unity_support_output(unity_support_output.stdout.decode()).get( + parse_unity_support_output(unity_support_output.stdout).get( "Not software rendered" ) == "yes" @@ -344,18 +360,19 @@ def main() -> int: """ args = parse_arguments() - - fwts_passed = False - device_comparison_passed=False - has_failed_services=False - + # all 4 tests pass by default + # they only fail if their respective flags are specified + fwts_passed = True + device_comparison_passed = True + renderer_test_passed = True + service_check_passed = True if args.output_directory is not None: dump_device_info(args.output_directory) supports_fwts = is_fwts_supported() - if supports_fwts and not fwts_log_check_passed(args.output_directory): - return 1 + if supports_fwts and fwts_log_check_passed(args.output_directory): + fwts_passed = True if args.comparison_directory is not None: if args.output_directory is None: @@ -370,33 +387,31 @@ def main() -> int: if args.do_service_check: print("Checking for failed system services...") + failed_services = get_failed_services() if len(failed_services) > 0: - print("These services failed: {}".format(failed_services), file=sys.stderr) - return 1 + print( + "These services failed: {}".format(failed_services), + file=sys.stderr, + ) + service_check_passed = False + + if args.do_renderer_check and has_display_connection(): + # skip renderer test if there's no display + renderer_test_passed = is_hardware_renderer_available() - if args.do_fwts_check and ( - args.output_directory is None - or not fwts_log_check_passed(args.output_directory) + if ( + fwts_passed + and device_comparison_passed + and renderer_test_passed + and service_check_passed ): + return 0 + else: return 1 - if args.do_renderer_check: - if not has_DRM_file_nodes(): - return 0 # skip gpu test if there's no GPU - return 0 if is_hardware_renderer_available() else 1 - - # if [[ -n "$service_opt" && "$service_check" == "false" ]] || \ - # [[ -n "$compare_dir" && "$device_check" == "false" ]] || \ - # [[ -n "$fwts_opt" && "$fwts_check" == "false" ]]; then - # exit 1 - # fi - - return 0 - if __name__ == "__main__": - print(os.getcwd()) return_code = main() exit(return_code) # dump_device_info("testoo") From da8ad43509a44202a945efb1d7e2b7c33a7cc96f Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Thu, 27 Jun 2024 16:07:02 +0800 Subject: [PATCH 03/36] fix: incorrect main function logic --- providers/base/bin/reboot_check_test.py | 42 +++++++++++++++---------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 97915983f3..0c5a8329d9 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -81,7 +81,11 @@ def compare_device_lists(expected_dir: str, actual_dir: str) -> bool: :param actual_dir: a directory of files containing the actual device list :return: whether the device list matches """ - print("Comparing devices...") + print( + "Comparing device list files in (expected){} against (actual){}...".format( + expected_dir, actual_dir + ) + ) for name in ["lspci", "iw", "lsusb"]: # file paths of the expected and actual device lists expected = "{}/{}_log".format(expected_dir, name) @@ -129,14 +133,14 @@ def dump_device_info(output_directory: str) -> None: # TODO: check if this is intended os.makedirs(output_directory, exist_ok=True) - print("Checking PCI devices...") + print("Collecting PCI devices...") with open("{}/lspci_log".format(output_directory), "w") as f: lspci_out = run_command( ["lspci", "-i", "{}/usr/share/misc/pci.ids".format(SNAP)], ) f.write(lspci_out.stdout) - print("Checking wireless connections...") + print("Collecting wireless connections...") with open("{}/iw_log".format(output_directory), "w") as f: iw_out = run_command(["iw", "dev"]) lines = iw_out.stdout.splitlines() @@ -150,7 +154,7 @@ def dump_device_info(output_directory: str) -> None: ) f.write("\n".join(map(lambda line: line.strip(), lines_to_write))) - print("Checking USB devices...") + print("Collecting USB devices...") with open("{}/lsusb_log".format(output_directory), "w") as f: lsusb_out = run_command( [ @@ -251,8 +255,8 @@ def get_display_id() -> T.Optional[str]: ) if display_server_type == "x11": - w_out = run_command(['w', '--no-header']) - return w_out.stdout.split()[2] if w_out.stdout != '' else None + w_out = run_command(["w", "--no-header"]) + return w_out.stdout.split()[2] if w_out.stdout != "" else None return None # Unsupported window system @@ -287,8 +291,12 @@ def has_display_connection() -> bool: except FileNotFoundError: pass # this just means we don't have a status file => no connection except Exception as e: - print(e, file=sys.stderr) + print("Unexpected error: ", e, file=sys.stderr) + print( + "No display is connected. This case will fail.", + "Maybe the display cable is not connected?", + ) return False @@ -363,28 +371,32 @@ def main() -> int: # all 4 tests pass by default # they only fail if their respective flags are specified + # if no flags are specified, calling this script is a no-op fwts_passed = True device_comparison_passed = True renderer_test_passed = True service_check_passed = True - if args.output_directory is not None: - dump_device_info(args.output_directory) - supports_fwts = is_fwts_supported() - if supports_fwts and fwts_log_check_passed(args.output_directory): - fwts_passed = True - if args.comparison_directory is not None: if args.output_directory is None: print( - "Error: Please specify an output directory with the -d flag.", + "Error: Please also specify an output directory with the -d flag.", file=sys.stderr, ) else: + dump_device_info(args.output_directory) compare_device_lists( args.comparison_directory, args.output_directory ) + # dump if only output is specified + if args.output_directory is not None and args.comparison_directory is None: + dump_device_info(args.output_directory) + + if args.do_fwts_check: + if is_fwts_supported() and not fwts_log_check_passed(args.output_directory): + fwts_passed = False + if args.do_service_check: print("Checking for failed system services...") @@ -414,5 +426,3 @@ def main() -> int: if __name__ == "__main__": return_code = main() exit(return_code) - # dump_device_info("testoo") - # is_hardware_renderer_available() From daed1f7e2e405dff610af6bf449672c6e836937b Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Fri, 28 Jun 2024 09:52:46 +0800 Subject: [PATCH 04/36] style: add more error messages --- providers/base/bin/reboot_check_test.py | 58 ++++++++++++++++++++----- 1 file changed, 47 insertions(+), 11 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 0c5a8329d9..740a7987cd 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -27,6 +27,13 @@ def __init__(self, return_code: int, stdout: str, stderr: str) -> None: def run_command(args: T.List[str]) -> ShellResult: + """Wrapper around subprocess.run + + :param args: same args that goes to subprocess.run + :type args: T.List[str] + :return: return code, stdout and stderr, all non-null + :rtype: ShellResult + """ # PIPE is needed for subprocess.run to capture stdout and stderr (<=3.7 behavior) out = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) return ShellResult( @@ -94,10 +101,20 @@ def compare_device_lists(expected_dir: str, actual_dir: str) -> bool: print( "The output of {} differs from the original list gathered at the beginning of the session!".format( name - ) + ), + file=sys.stderr, ) return False + if not filecmp.cmp( + "{}/drm_log".format(expected_dir), "{}/drm_log".format(actual_dir) + ): + print( + "[WARN] Items under /sys/class/drm has changed.", + "If this machine dynamically switches between GPUs, this might be expected", + file=sys.stderr, + ) + return True @@ -166,6 +183,11 @@ def dump_device_info(output_directory: str) -> None: ) f.write(lsusb_out.stdout) + print("Collecting /sys/class/drm...") + with open("{}/drm_log".format(output_directory), "w") as f: + drm_contents = os.listdir("/sys/class/drm") + f.write(str(sorted(drm_contents))) + os.sync() # force disk write print("Finished dumping device info!") @@ -178,12 +200,12 @@ def parse_arguments(): parser.add_argument( "-d", dest="output_directory", - help="Absolute path to the output directory", + help="Absolute path to the output directory. Device info-dumps will be written here.", ) parser.add_argument( "-c", dest="comparison_directory", - help="Absolute path to the comparison directory", + help="Absolute path to the comparison directory. This should contain the ground-truth.", ) parser.add_argument( "-s", @@ -230,6 +252,7 @@ def get_display_id() -> T.Optional[str]: XDG_SESSION_TYPE = os.getenv("XDG_SESSION_TYPE", default="") if DISPLAY != "": + print("Using $DISPLAY env: {}".format(DISPLAY)) return DISPLAY # use the environment var if non-empty session_id = ( @@ -245,8 +268,16 @@ def get_display_id() -> T.Optional[str]: .strip() # it should look like Type=wayland, split and take 2nd word ) - print("{} =? {}".format(XDG_SESSION_TYPE, display_server_type)) + if XDG_SESSION_TYPE != display_server_type: + print( + "[WARN] XDG_SESSION_TYPE: {} != display server from loginctl: {}".format( + XDG_SESSION_TYPE, display_server_type + ), + file=sys.stderr, + ) + # NOTE: Xwayland doesn't immediately start after a reboot + # This could return None even if a display is active if display_server_type == "wayland": pgrep_out = run_command(["pgrep", "-a", "Xwayland"]) # search for a process called Xwayland, take the 3rd arg, which is the display id @@ -309,10 +340,12 @@ def is_hardware_renderer_available() -> bool: # Now we know some kind of display exists, run unity_support_test display_id = get_display_id() - print("Checking display id: {}".format(display_id)) if display_id is None: + print("No display id was found.", file=sys.stderr) # No display id was found return False + + print("Checking display id: {}".format(display_id)) unity_support_output = run_command( [ @@ -331,11 +364,11 @@ def is_hardware_renderer_available() -> bool: ) == "yes" ) - if not is_hardware_rendered: - return False + if is_hardware_rendered: + print("This machine is using a hardware renderer!") + return True - print("This machine is using a hardware renderer!") - return True + return False def parse_unity_support_output(output_string: str) -> T.Dict[str, str]: @@ -389,12 +422,14 @@ def main() -> int: args.comparison_directory, args.output_directory ) - # dump if only output is specified + # dump if only output_directory is specified if args.output_directory is not None and args.comparison_directory is None: dump_device_info(args.output_directory) if args.do_fwts_check: - if is_fwts_supported() and not fwts_log_check_passed(args.output_directory): + if is_fwts_supported() and not fwts_log_check_passed( + args.output_directory + ): fwts_passed = False if args.do_service_check: @@ -424,5 +459,6 @@ def main() -> int: if __name__ == "__main__": + # print(get_display_id()) return_code = main() exit(return_code) From ba87ddddfec94f08662dfd394def8d37b3706756 Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Fri, 28 Jun 2024 09:57:14 +0800 Subject: [PATCH 05/36] fix: no display connectio n-> skipped --- providers/base/bin/reboot_check_test.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 740a7987cd..95ee2621ca 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -147,7 +147,6 @@ def dump_device_info(output_directory: str) -> None: print("Dumping the devices information to {}".format(output_directory)) # specifying exist_ok=True for now to mimic mkdir -p's behavior, - # TODO: check if this is intended os.makedirs(output_directory, exist_ok=True) print("Collecting PCI devices...") @@ -325,7 +324,7 @@ def has_display_connection() -> bool: print("Unexpected error: ", e, file=sys.stderr) print( - "No display is connected. This case will fail.", + "No display is connected. This case will be skipped.", "Maybe the display cable is not connected?", ) return False @@ -422,7 +421,7 @@ def main() -> int: args.comparison_directory, args.output_directory ) - # dump if only output_directory is specified + # dump (no checks) if only output_directory is specified if args.output_directory is not None and args.comparison_directory is None: dump_device_info(args.output_directory) @@ -459,6 +458,5 @@ def main() -> int: if __name__ == "__main__": - # print(get_display_id()) return_code = main() exit(return_code) From 9913476cf3044509ff0389d7dab840096dcf1fbd Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Fri, 5 Jul 2024 11:34:48 +0800 Subject: [PATCH 06/36] refactor: move device comparison tests to a class --- providers/base/bin/reboot_check_test.py | 236 +++++++++++++----------- 1 file changed, 132 insertions(+), 104 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 95ee2621ca..379739d7db 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -1,8 +1,5 @@ #!/usr/bin/env python3 -# TODO: Match python 3.5 syntax - - import argparse import os import subprocess @@ -10,6 +7,7 @@ import shutil import filecmp import sys +import enum import typing as T @@ -20,7 +18,9 @@ class ShellResult: - def __init__(self, return_code: int, stdout: str, stderr: str) -> None: + """Wrapper class around the return value of run_command, guarantees non-null""" + + def __init__(self, return_code: int, stdout: str, stderr: str): self.return_code = return_code self.stdout = stdout self.stderr = stderr @@ -41,14 +41,18 @@ def run_command(args: T.List[str]) -> ShellResult: # if there's nothing on stdout, .stdout is None, so we need a default value stdout=(out.stdout or b"").decode(), stderr=(out.stderr or b"").decode(), - ) # This could throw on non-UTF8 decodable byte strings, but that should be rare + ) + # This could throw on non-UTF8 decodable byte strings, but that should be rare + # since utf-8 is backwards compatible with ascii def is_fwts_supported() -> bool: return shutil.which("fwts") is not None -def fwts_log_check_passed(output_directory: str) -> bool: +def fwts_log_check_passed( + output_directory: str, fwts_arguments=["klog", "oops"] +) -> bool: """Check if fwts logs passes the checks specified in sleep_test_log_check.py. This script live in the same directory @@ -57,16 +61,10 @@ def fwts_log_check_passed(output_directory: str) -> bool: :return: whether sleep_test_log_check.py returned 0 (success) :rtype: bool """ - log_file_path = "{}/fwts_klog_oops.log".format(output_directory) - subprocess.run( - [ - "fwts", - "-r", - log_file_path, - "klog", - "oops", - ] + log_file_path = "{}/fwts_{}.log".format( + output_directory, "_".join(fwts_arguments) ) + subprocess.run(["fwts", "-r", log_file_path, *fwts_arguments]) result = subprocess.run( [ "sleep_test_log_check.py", @@ -81,43 +79,6 @@ def fwts_log_check_passed(output_directory: str) -> bool: return result.returncode == 0 -def compare_device_lists(expected_dir: str, actual_dir: str) -> bool: - """Compares the list of devices in expected_dir against actual_dir - - :param expected_dir: a directory of files containing the expected values the device list - :param actual_dir: a directory of files containing the actual device list - :return: whether the device list matches - """ - print( - "Comparing device list files in (expected){} against (actual){}...".format( - expected_dir, actual_dir - ) - ) - for name in ["lspci", "iw", "lsusb"]: - # file paths of the expected and actual device lists - expected = "{}/{}_log".format(expected_dir, name) - actual = "{}/{}_log".format(actual_dir, name) - if not filecmp.cmp(expected, actual): - print( - "The output of {} differs from the original list gathered at the beginning of the session!".format( - name - ), - file=sys.stderr, - ) - return False - - if not filecmp.cmp( - "{}/drm_log".format(expected_dir), "{}/drm_log".format(actual_dir) - ): - print( - "[WARN] Items under /sys/class/drm has changed.", - "If this machine dynamically switches between GPUs, this might be expected", - file=sys.stderr, - ) - - return True - - def get_failed_services() -> T.List[str]: """Counts the number of failed services listed in systemctl @@ -136,28 +97,27 @@ def get_failed_services() -> T.List[str]: return run_command(command).stdout.split() -def dump_device_info(output_directory: str) -> None: - """Writes information of PCI, wireless, and USB devices to the specified directory +class DeviceInfoCollector: - :param output_directory: where the output should be written to. - If this directory doesn't already exist, this function will create it. - :type output_directory: str - """ - - print("Dumping the devices information to {}".format(output_directory)) + class Device(enum.Enum): + PCI = "pci" + WIRELESS = "wireless" + USB = "usb" + DRM = "drm" - # specifying exist_ok=True for now to mimic mkdir -p's behavior, - os.makedirs(output_directory, exist_ok=True) + DEFAULT_DEVICES = { + "required": [ + Device.WIRELESS, + Device.PCI, + Device.USB, + ], # these can fail the test case + "optional": [Device.DRM], # these only produce warnings + } # used for comparison and dump calls - print("Collecting PCI devices...") - with open("{}/lspci_log".format(output_directory), "w") as f: - lspci_out = run_command( - ["lspci", "-i", "{}/usr/share/misc/pci.ids".format(SNAP)], - ) - f.write(lspci_out.stdout) + def get_drm_info(self) -> str: + return str(os.listdir("/sys/class/drm")) - print("Collecting wireless connections...") - with open("{}/iw_log".format(output_directory), "w") as f: + def get_wireless_info(self) -> str: iw_out = run_command(["iw", "dev"]) lines = iw_out.stdout.splitlines() lines_to_write = list( @@ -168,27 +128,93 @@ def dump_device_info(output_directory: str) -> None: sorted(lines), ) ) - f.write("\n".join(map(lambda line: line.strip(), lines_to_write))) + return "\n".join(map(lambda line: line.strip(), lines_to_write)) - print("Collecting USB devices...") - with open("{}/lsusb_log".format(output_directory), "w") as f: - lsusb_out = run_command( + def get_usb_info(self) -> str: + return run_command( [ "checkbox-support-lsusb", "-f", '"{}"/var/lib/usbutils/usb.ids'.format(RUNTIME_ROOT), "-s", ] + ).stdout + + def get_pci_info(self) -> str: + return run_command( + ["lspci", "-i", "{}/usr/share/misc/pci.ids".format(SNAP)], + ).stdout + + def compare_device_lists( + self, expected_dir: str, actual_dir: str, devices=DEFAULT_DEVICES + ) -> bool: + """Compares the list of devices in expected_dir against actual_dir + + :param expected_dir: a directory of files containing the expected values the device list + :param actual_dir: a directory of files containing the actual device list + :return: whether the device list matches + """ + print( + "Comparing device list files in (expected){} against (actual){}...".format( + expected_dir, actual_dir + ) ) - f.write(lsusb_out.stdout) + for device in devices["required"]: + # file paths of the expected and actual device lists + expected = "{}/{}_log".format(expected_dir, device) + actual = "{}/{}_log".format(actual_dir, device) + if not filecmp.cmp(expected, actual): + print( + "The output of {} differs from the list gathered at the beginning of the session!".format( + device + ), + file=sys.stderr, + ) + return False + + for device in devices["optional"]: + expected = "{}/{}_log".format(expected_dir, device) + actual = "{}/{}_log".format(actual_dir, device) + if not filecmp.cmp(expected, actual): + print( + "[WARN] Items under {} has changed.".format(actual), + "If this machine dynamically switches between GPUs, this might be expected", + file=sys.stderr, + ) + + return True + + def dump( + self, + output_directory: str, + devices: T.Dict[str, T.List[Device]] = DEFAULT_DEVICES + ) -> None: + - print("Collecting /sys/class/drm...") - with open("{}/drm_log".format(output_directory), "w") as f: - drm_contents = os.listdir("/sys/class/drm") - f.write(str(sorted(drm_contents))) + os.makedirs(output_directory, exist_ok=True) + # add extra behavior if necessary + for device in devices["required"]: + with open( + "{}/{}_log".format(output_directory, device.value), "w" + ) as file: + file.write(self.dump_function[device]()) - os.sync() # force disk write - print("Finished dumping device info!") + for device in devices["optional"]: + with open( + "{}/{}_log".format(output_directory, device.value), "w" + ) as file: + file.write(self.dump_function[device]()) + + os.sync() + + def __init__(self) -> None: + # self.output_directory = output_directory + self.dump_function = { + self.Device.PCI: self.get_pci_info, + self.Device.DRM: self.get_drm_info, + self.Device.USB: self.get_usb_info, + self.Device.WIRELESS: self.get_wireless_info, + } def parse_arguments(): @@ -198,16 +224,20 @@ def parse_arguments(): ) parser.add_argument( "-d", + "--dump-to", + required=True, dest="output_directory", help="Absolute path to the output directory. Device info-dumps will be written here.", ) parser.add_argument( "-c", + "--compare-to", dest="comparison_directory", help="Absolute path to the comparison directory. This should contain the ground-truth.", ) parser.add_argument( "-s", + "--service-check", default=False, dest="do_service_check", action="store_true", @@ -215,6 +245,7 @@ def parse_arguments(): ) parser.add_argument( "-f", + "--fwts-check", default=False, dest="do_fwts_check", action="store_true", @@ -222,13 +253,14 @@ def parse_arguments(): ) parser.add_argument( "-g", + "--graphics", default=False, dest="do_renderer_check", action="store_true", help="Whether the script should check if hardware rendering is being used", ) - return parser.parse_args() # type: ignore , checked manually + return parser.parse_args() def remove_color_code(string: str) -> str: @@ -248,10 +280,9 @@ def get_display_id() -> T.Optional[str]: :rtype: str """ DISPLAY = os.getenv("DISPLAY", default="") - XDG_SESSION_TYPE = os.getenv("XDG_SESSION_TYPE", default="") if DISPLAY != "": - print("Using $DISPLAY env: {}".format(DISPLAY)) + print("Using $DISPLAY env variable: {}".format(DISPLAY)) return DISPLAY # use the environment var if non-empty session_id = ( @@ -267,34 +298,30 @@ def get_display_id() -> T.Optional[str]: .strip() # it should look like Type=wayland, split and take 2nd word ) - if XDG_SESSION_TYPE != display_server_type: - print( - "[WARN] XDG_SESSION_TYPE: {} != display server from loginctl: {}".format( - XDG_SESSION_TYPE, display_server_type - ), - file=sys.stderr, - ) - # NOTE: Xwayland doesn't immediately start after a reboot - # This could return None even if a display is active + # For now, we will assume :0 exists and use it as the display id if display_server_type == "wayland": pgrep_out = run_command(["pgrep", "-a", "Xwayland"]) # search for a process called Xwayland, take the 3rd arg, which is the display id - return ( - pgrep_out.stdout.split()[2] if pgrep_out.return_code == 0 else None - ) + if pgrep_out.return_code == 0: + return pgrep_out.stdout.split()[2] + else: + print('[WARN] Waylad session detected, but Xwayland process is not found. Assuming :0 display') + return ':0' if display_server_type == "x11": w_out = run_command(["w", "--no-header"]) - return w_out.stdout.split()[2] if w_out.stdout != "" else None + if len(w_out.stdout) != 0: + return w_out.stdout.split()[2] + return None return None # Unsupported window system def has_display_connection() -> bool: - """Checks if a display is connected. + """Checks if a display is connected by searching the /sys/class/drm directory - :return: True if there are more items than just "version" + :return: True if there's at least 1 node that is connected """ # look for GPU file nodes first @@ -343,7 +370,7 @@ def is_hardware_renderer_available() -> bool: print("No display id was found.", file=sys.stderr) # No display id was found return False - + print("Checking display id: {}".format(display_id)) unity_support_output = run_command( @@ -364,7 +391,7 @@ def is_hardware_renderer_available() -> bool: == "yes" ) if is_hardware_rendered: - print("This machine is using a hardware renderer!") + print("[ OK ] This machine is using a hardware renderer!") return True return False @@ -416,14 +443,15 @@ def main() -> int: file=sys.stderr, ) else: - dump_device_info(args.output_directory) - compare_device_lists( + collector = DeviceInfoCollector() + collector.dump(args.output_directory) + collector.compare_device_lists( args.comparison_directory, args.output_directory ) # dump (no checks) if only output_directory is specified if args.output_directory is not None and args.comparison_directory is None: - dump_device_info(args.output_directory) + DeviceInfoCollector().dump(args.output_directory) if args.do_fwts_check: if is_fwts_supported() and not fwts_log_check_passed( From aa90b1e830802667c78863e820bb390ed236a01b Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Mon, 8 Jul 2024 09:59:09 +0800 Subject: [PATCH 07/36] test: unit tests for reboot_check_test --- .../base/tests/test_reboot_check_test.py | 168 ++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 providers/base/tests/test_reboot_check_test.py diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py new file mode 100644 index 0000000000..ff02534663 --- /dev/null +++ b/providers/base/tests/test_reboot_check_test.py @@ -0,0 +1,168 @@ +import shutil +from unittest.mock import mock_open, patch +import reboot_check_test as RCT +import unittest +import os +import typing as T + + +# manually override the return value based on arguments +def run_command_side_effect(args: T.List[str]) -> RCT.ShellResult: + stdout = "" + if args[0] == "iw": + stdout = """ + addr some address + Interface some interface + ssid some ssid + """ + elif args[0] == "checkbox-support-lsusb": + stdout = """ + usb1 + usb2 + usb3 + """ + elif args[0] == "lspci": + stdout = """ + pci1 + pci2 + pci3 + """ + else: + raise Exception("Unexpected use of this mock") + + return RCT.ShellResult(0, stdout, "") + + +class RebootCheckTestTests(unittest.TestCase): + + @classmethod + def setUpClass(cls): + cls.shared_resource = 1 + + def test_parse_ok_unity_support_string(self): + OK_UNITY_STRING = """ + OpenGL vendor string: Intel + OpenGL renderer string: Mesa Intel(R) UHD Graphics (ICL GT1) + OpenGL version string: 4.6 (Compatibility Profile) Mesa 23.2.1-1ubuntu3.1~22.04.2 + + Not software rendered: yes + Not blacklisted: yes + GLX fbconfig: yes + GLX texture from pixmap: yes + GL npot or rect textures: yes + GL vertex program: yes + GL fragment program: yes + GL vertex buffer object: no + GL framebuffer object: yes + GL version is 1.4+: yes + + Unity 3D supported: yes + """ + + expected = { + "OpenGL vendor string": "Intel", + "OpenGL renderer string": "Mesa Intel(R) UHD Graphics (ICL GT1)", + "OpenGL version string": "4.6 (Compatibility Profile) Mesa 23.2.1-1ubuntu3.1~22.04.2", + "Not software rendered": "yes", + "Not blacklisted": "yes", + "GLX fbconfig": "yes", + "GLX texture from pixmap": "yes", + "GL npot or rect textures": "yes", + "GL vertex program": "yes", + "GL fragment program": "yes", + "GL vertex buffer object": "no", + "GL framebuffer object": "yes", + "GL version is 1.4+": "yes", + "Unity 3D supported": "yes", + } + + actual = RCT.parse_unity_support_output(OK_UNITY_STRING) + self.assertDictEqual(expected, actual) + + def test_parse_bad_unity_support_string(self): + BAD_UNITY_STRING = """ + OpenGL vendor string Intel + OpenGL renderer string: Mesa Intel(R) UHD Graphics (ICL GT1) + OpenGL version string 4.6 (Compatibility Profile) Mesa 23.2.1-1ubuntu3.1~22.04.2 + GL version is 1.4+% yes + """ + actual = RCT.parse_unity_support_output(BAD_UNITY_STRING) + + expected = { + "OpenGL renderer string": "Mesa Intel(R) UHD Graphics (ICL GT1)", + } + + self.assertDictEqual(expected, actual) + + ARBITRARY_STRING = "askjaskdnasdn" + # should return empty dict if input string literally doesn't make sense + self.assertEqual(RCT.parse_unity_support_output(ARBITRARY_STRING), {}) + + @patch("reboot_check_test.run_command") + def test_info_dump_only_happy_path(self, mock_run): + print(os.getcwd()) + + # manually override the return value based on arguments + def run_command_side_effect(args: T.List[str]) -> RCT.ShellResult: + stdout = "" + if args[0] == "iw": + stdout = """ + addr some address + Interface some interface + ssid some ssid + """ + elif args[0] == "checkbox-support-lsusb": + stdout = """ + usb1 + usb2 + usb3 + """ + elif args[0] == "lspci": + stdout = """ + pci1 + pci2 + pci3 + """ + else: + raise Exception("Unexpected use of this mock") + + return RCT.ShellResult(0, stdout, "") + + mock_run.side_effect = run_command_side_effect + + RCT.DeviceInfoCollector().dump( + "{}/temporary_dump_directory".format(os.getcwd()) + ) + + def test_display_check_happy_path(self): + with patch("os.listdir", return_value=["card0", "card1"]), patch( + "builtins.open", + new_callable=mock_open, + read_data="connected", + ): + self.assertTrue(RCT.has_display_connection()) + + def test_display_check_no_display_path(self): + with patch("os.listdir", return_value=["version"]): + self.assertFalse(RCT.has_display_connection()) + with patch("os.listdir", return_value=["card0", "card1"]), patch( + "builtins.open", + new_callable=mock_open, + read_data="not connected", + ): + self.assertFalse(RCT.has_display_connection()) + + def test_is_hardware_renderer_available(self): + self.assertTrue(RCT.is_hardware_renderer_available()) + + def test_main_function_logic(self): + # this test only validates the main function logic(if it picks out the correct tests to run) + do_nothing= lambda: None + with patch( + "reboot_check_test.DeviceInfoCollector.dump", new=do_nothing + ): + ... + + @classmethod + def tearDownClass(cls): + shutil.rmtree("{}/temporary_dump_directory".format(os.getcwd())) From c9ff59eb7c925aab39712e1fbb5dbdf02c5dc1e7 Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Mon, 8 Jul 2024 13:50:18 +0800 Subject: [PATCH 08/36] fix: remove any direct calls to subprocess.run --- providers/base/bin/reboot_check_test.py | 39 +++++++++++++------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 379739d7db..72cdfded4e 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -64,8 +64,8 @@ def fwts_log_check_passed( log_file_path = "{}/fwts_{}.log".format( output_directory, "_".join(fwts_arguments) ) - subprocess.run(["fwts", "-r", log_file_path, *fwts_arguments]) - result = subprocess.run( + run_command(["fwts", "-r", log_file_path, *fwts_arguments]) + result = run_command( [ "sleep_test_log_check.py", "-v", @@ -76,7 +76,7 @@ def fwts_log_check_passed( ] ) - return result.returncode == 0 + return result.return_code == 0 def get_failed_services() -> T.List[str]: @@ -113,6 +113,7 @@ class Device(enum.Enum): ], # these can fail the test case "optional": [Device.DRM], # these only produce warnings } # used for comparison and dump calls + # to modify, add more values in the enum and reference them in required/optional respectively def get_drm_info(self) -> str: return str(os.listdir("/sys/class/drm")) @@ -186,11 +187,9 @@ def compare_device_lists( def dump( self, - output_directory: str, - devices: T.Dict[str, T.List[Device]] = DEFAULT_DEVICES + output_directory: str, + devices=DEFAULT_DEVICES, ) -> None: - - os.makedirs(output_directory, exist_ok=True) # add extra behavior if necessary for device in devices["required"]: @@ -208,7 +207,6 @@ def dump( os.sync() def __init__(self) -> None: - # self.output_directory = output_directory self.dump_function = { self.Device.PCI: self.get_pci_info, self.Device.DRM: self.get_drm_info, @@ -217,7 +215,7 @@ def __init__(self) -> None: } -def parse_arguments(): +def create_parser(): parser = argparse.ArgumentParser( prog="Reboot tests", description="This script is used to collect device information and to check for differences between reboots.", @@ -260,7 +258,7 @@ def parse_arguments(): help="Whether the script should check if hardware rendering is being used", ) - return parser.parse_args() + return parser def remove_color_code(string: str) -> str: @@ -306,12 +304,15 @@ def get_display_id() -> T.Optional[str]: if pgrep_out.return_code == 0: return pgrep_out.stdout.split()[2] else: - print('[WARN] Waylad session detected, but Xwayland process is not found. Assuming :0 display') - return ':0' + print( + "[WARN] Waylad session detected, but Xwayland process is not found. Assuming :0 display", + file=sys.stderr, + ) + return ":0" if display_server_type == "x11": w_out = run_command(["w", "--no-header"]) - if len(w_out.stdout) != 0: + if w_out.return_code == 0 and len(w_out.stdout) != 0: return w_out.stdout.split()[2] return None @@ -346,7 +347,8 @@ def has_display_connection() -> bool: print("{} is connected to display!".format(gpu)) return True except FileNotFoundError: - pass # this just means we don't have a status file => no connection + # this just means we don't have a status file => no connection, continue to the next + pass except Exception as e: print("Unexpected error: ", e, file=sys.stderr) @@ -397,7 +399,7 @@ def is_hardware_renderer_available() -> bool: return False -def parse_unity_support_output(output_string: str) -> T.Dict[str, str]: +def parse_unity_support_output(unity_output_string: str) -> T.Dict[str, str]: """Parses the output of `unity_support_test` into a dictionary :param output_string: the raw output from running `unity_support_test -p` @@ -408,11 +410,11 @@ def parse_unity_support_output(output_string: str) -> T.Dict[str, str]: """ output = {} # type: dict[str, str] - for line in output_string.split("\n"): + for line in unity_output_string.split("\n"): # max_split=1 to prevent splitting the string after the 1st colon words = line.split(":", maxsplit=1) if len(words) == 2: - key = words[0] + key = words[0].strip() value = remove_color_code(words[1].strip()) output[key] = value @@ -426,7 +428,8 @@ def main() -> int: :rtype: int """ - args = parse_arguments() + args = create_parser().parse_args() + print(args) # all 4 tests pass by default # they only fail if their respective flags are specified From 2800dc3a7d0223f5ff0c10c40be952ed174d0adf Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Mon, 8 Jul 2024 13:51:51 +0800 Subject: [PATCH 09/36] test: add a test for the all-flags-enabled case --- .../base/tests/test_reboot_check_test.py | 145 +++++++++++------- 1 file changed, 90 insertions(+), 55 deletions(-) diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index ff02534663..ed0766a590 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -1,43 +1,21 @@ import shutil -from unittest.mock import mock_open, patch +from shlex import split as sh_split +from unittest.mock import MagicMock, mock_open, patch import reboot_check_test as RCT import unittest import os import typing as T -# manually override the return value based on arguments -def run_command_side_effect(args: T.List[str]) -> RCT.ShellResult: - stdout = "" - if args[0] == "iw": - stdout = """ - addr some address - Interface some interface - ssid some ssid - """ - elif args[0] == "checkbox-support-lsusb": - stdout = """ - usb1 - usb2 - usb3 - """ - elif args[0] == "lspci": - stdout = """ - pci1 - pci2 - pci3 - """ - else: - raise Exception("Unexpected use of this mock") - - return RCT.ShellResult(0, stdout, "") +def do_nothing_run_cmd(_: T.List[str]): + return RCT.ShellResult(0, "", "") class RebootCheckTestTests(unittest.TestCase): @classmethod def setUpClass(cls): - cls.shared_resource = 1 + cls.temp_directory = "{}/temporary_dump_directory".format(os.getcwd()) def test_parse_ok_unity_support_string(self): OK_UNITY_STRING = """ @@ -45,18 +23,18 @@ def test_parse_ok_unity_support_string(self): OpenGL renderer string: Mesa Intel(R) UHD Graphics (ICL GT1) OpenGL version string: 4.6 (Compatibility Profile) Mesa 23.2.1-1ubuntu3.1~22.04.2 - Not software rendered: yes - Not blacklisted: yes - GLX fbconfig: yes - GLX texture from pixmap: yes - GL npot or rect textures: yes - GL vertex program: yes - GL fragment program: yes - GL vertex buffer object: no - GL framebuffer object: yes - GL version is 1.4+: yes - - Unity 3D supported: yes + Not software rendered: \x1B[033myes\x1B[0m + Not blacklisted: \x1B[033myes\x1B[0m + GLX fbconfig: \x1B[033myes\x1B[0m + GLX texture from pixmap: \x1B[033myes\x1B[0m + GL npot or rect textures: \x1B[033myes\x1B[0m + GL vertex program: \x1B[033myes\x1B[0m + GL fragment program: \x1B[033myes\x1B[0m + GL vertex buffer object: \x1B[033mno\x1B[0m + GL framebuffer object: \x1B[033myes\x1B[0m + GL version is 1.4+: \x1B[033myes\x1B[0m + + Unity 3D supported: \x1B[033myes\x1B[0m """ expected = { @@ -84,7 +62,7 @@ def test_parse_bad_unity_support_string(self): OpenGL vendor string Intel OpenGL renderer string: Mesa Intel(R) UHD Graphics (ICL GT1) OpenGL version string 4.6 (Compatibility Profile) Mesa 23.2.1-1ubuntu3.1~22.04.2 - GL version is 1.4+% yes + GL version is 1.4+% \x1B[033myes\x1B[0m """ actual = RCT.parse_unity_support_output(BAD_UNITY_STRING) @@ -99,26 +77,24 @@ def test_parse_bad_unity_support_string(self): self.assertEqual(RCT.parse_unity_support_output(ARBITRARY_STRING), {}) @patch("reboot_check_test.run_command") - def test_info_dump_only_happy_path(self, mock_run): - print(os.getcwd()) - + def test_info_dump_only_happy_path(self, mock_run: MagicMock): # manually override the return value based on arguments - def run_command_side_effect(args: T.List[str]) -> RCT.ShellResult: + def mock_run_command(args: T.List[str]) -> RCT.ShellResult: stdout = "" if args[0] == "iw": - stdout = """ + stdout = """\ addr some address Interface some interface ssid some ssid """ elif args[0] == "checkbox-support-lsusb": - stdout = """ + stdout = """\ usb1 usb2 usb3 """ elif args[0] == "lspci": - stdout = """ + stdout = """\ pci1 pci2 pci3 @@ -128,11 +104,10 @@ def run_command_side_effect(args: T.List[str]) -> RCT.ShellResult: return RCT.ShellResult(0, stdout, "") - mock_run.side_effect = run_command_side_effect + # wrap over run_command's return value + mock_run.side_effect = mock_run_command - RCT.DeviceInfoCollector().dump( - "{}/temporary_dump_directory".format(os.getcwd()) - ) + RCT.DeviceInfoCollector().dump(self.temp_directory) def test_display_check_happy_path(self): with patch("os.listdir", return_value=["card0", "card1"]), patch( @@ -155,14 +130,74 @@ def test_display_check_no_display_path(self): def test_is_hardware_renderer_available(self): self.assertTrue(RCT.is_hardware_renderer_available()) - def test_main_function_logic(self): + @patch("reboot_check_test.run_command") + def test_main_function_logic(self, mock_run: MagicMock): # this test only validates the main function logic(if it picks out the correct tests to run) - do_nothing= lambda: None + mock_run.side_effect = do_nothing_run_cmd + with patch( - "reboot_check_test.DeviceInfoCollector.dump", new=do_nothing + "sys.argv", + sh_split("reboot_check_test.py -d {}".format(self.temp_directory)), ): - ... + RCT.main() + self.assertEqual( + mock_run.call_count, + len(RCT.DeviceInfoCollector.DEFAULT_DEVICES["required"]), + ) + + with patch( + "sys.argv", + sh_split( + 'reboot_check_test.py -d "{}" -c "{}"'.format( + self.temp_directory, "some dir" + ) + ), + ), patch( + "reboot_check_test.DeviceInfoCollector.compare_device_lists" + ) as mock_compare: + RCT.main() + print(mock_run.call_count, mock_run.call_args_list) + print(mock_compare.call_count, mock_compare.call_args_list) + @patch("reboot_check_test.run_command") + def test_main_function_full(self, mock_run: MagicMock): + mock_run.side_effect = do_nothing_run_cmd + # Full suite + with patch( + "sys.argv", + sh_split( + 'reboot_check_test.py -d "{}" -c "{}" -f -s -g'.format( + self.temp_directory, "some dir" + ) + ), + ), patch( + "reboot_check_test.DeviceInfoCollector.compare_device_lists" + ) as mock_compare, patch( + "reboot_check_test.is_fwts_supported" + ) as mock_fwts_support_check: + mock_fwts_support_check.side_effect = lambda: True + RCT.main() + self.assertTrue(mock_compare.called) + self.assertTrue(mock_fwts_support_check.called) + + expected_commands = { + "systemctl", + "sleep_test_log_check.py", + } + + actual = set() + for call in mock_run.call_args_list: + # it's really ugly, but [0] takes the 1st from (command, args, kwargs), + # then take tha actual list from args + # then take the 1st element, which is the command name + actual.add(call[0][0][0]) + + # value doesn't matter, we just want to compare the keys + self.assertDictContainsSubset( + dict.fromkeys(expected_commands), dict.fromkeys(actual) + ) + + @classmethod def tearDownClass(cls): shutil.rmtree("{}/temporary_dump_directory".format(os.getcwd())) From ab37b6907f4307b7280034241c108d868a2c9e57 Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Mon, 8 Jul 2024 15:15:49 +0800 Subject: [PATCH 10/36] style: reorganize declarations --- providers/base/bin/reboot_check_test.py | 142 ++++++++++++------------ 1 file changed, 71 insertions(+), 71 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 72cdfded4e..fdba47c302 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -26,77 +26,6 @@ def __init__(self, return_code: int, stdout: str, stderr: str): self.stderr = stderr -def run_command(args: T.List[str]) -> ShellResult: - """Wrapper around subprocess.run - - :param args: same args that goes to subprocess.run - :type args: T.List[str] - :return: return code, stdout and stderr, all non-null - :rtype: ShellResult - """ - # PIPE is needed for subprocess.run to capture stdout and stderr (<=3.7 behavior) - out = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) - return ShellResult( - return_code=out.returncode, - # if there's nothing on stdout, .stdout is None, so we need a default value - stdout=(out.stdout or b"").decode(), - stderr=(out.stderr or b"").decode(), - ) - # This could throw on non-UTF8 decodable byte strings, but that should be rare - # since utf-8 is backwards compatible with ascii - - -def is_fwts_supported() -> bool: - return shutil.which("fwts") is not None - - -def fwts_log_check_passed( - output_directory: str, fwts_arguments=["klog", "oops"] -) -> bool: - """Check if fwts logs passes the checks specified in sleep_test_log_check.py. - This script live in the same directory - - :param output_directory: where the output of fwts should be redirected to - :type output_directory: str - :return: whether sleep_test_log_check.py returned 0 (success) - :rtype: bool - """ - log_file_path = "{}/fwts_{}.log".format( - output_directory, "_".join(fwts_arguments) - ) - run_command(["fwts", "-r", log_file_path, *fwts_arguments]) - result = run_command( - [ - "sleep_test_log_check.py", - "-v", - "--ignore-warning", - "-t", - "all", - log_file_path, - ] - ) - - return result.return_code == 0 - - -def get_failed_services() -> T.List[str]: - """Counts the number of failed services listed in systemctl - - :return: number of failed services - """ - command = [ - "systemctl", - "list-units", - "--system", - "--no-ask-password", - "--no-pager", - "--no-legend", - "--state=failed", - ] # only print the names of the services that failed - - return run_command(command).stdout.split() - - class DeviceInfoCollector: class Device(enum.Enum): @@ -215,6 +144,77 @@ def __init__(self) -> None: } +def run_command(args: T.List[str]) -> ShellResult: + """Wrapper around subprocess.run + + :param args: same args that goes to subprocess.run + :type args: T.List[str] + :return: return code, stdout and stderr, all non-null + :rtype: ShellResult + """ + # PIPE is needed for subprocess.run to capture stdout and stderr (<=3.7 behavior) + out = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + return ShellResult( + return_code=out.returncode, + # if there's nothing on stdout, .stdout is None, so we need a default value + stdout=(out.stdout or b"").decode(), + stderr=(out.stderr or b"").decode(), + ) + # This could throw on non-UTF8 decodable byte strings, but that should be rare + # since utf-8 is backwards compatible with ascii + + +def is_fwts_supported() -> bool: + return shutil.which("fwts") is not None + + +def fwts_log_check_passed( + output_directory: str, fwts_arguments=["klog", "oops"] +) -> bool: + """Check if fwts logs passes the checks specified in sleep_test_log_check.py. + This script live in the same directory + + :param output_directory: where the output of fwts should be redirected to + :type output_directory: str + :return: whether sleep_test_log_check.py returned 0 (success) + :rtype: bool + """ + log_file_path = "{}/fwts_{}.log".format( + output_directory, "_".join(fwts_arguments) + ) + run_command(["fwts", "-r", log_file_path, *fwts_arguments]) + result = run_command( + [ + "sleep_test_log_check.py", + "-v", + "--ignore-warning", + "-t", + "all", + log_file_path, + ] + ) + + return result.return_code == 0 + + +def get_failed_services() -> T.List[str]: + """Counts the number of failed services listed in systemctl + + :return: number of failed services + """ + command = [ + "systemctl", + "list-units", + "--system", + "--no-ask-password", + "--no-pager", + "--no-legend", + "--state=failed", + ] # only print the names of the services that failed + + return run_command(command).stdout.split() + + def create_parser(): parser = argparse.ArgumentParser( prog="Reboot tests", From 5cb060ed4437d6c13c408b8c2cd66eca00432e83 Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Mon, 8 Jul 2024 16:15:39 +0800 Subject: [PATCH 11/36] fix: missing .value when using enums --- providers/base/bin/reboot_check_test.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index fdba47c302..7fc06613dd 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -76,7 +76,10 @@ def get_pci_info(self) -> str: ).stdout def compare_device_lists( - self, expected_dir: str, actual_dir: str, devices=DEFAULT_DEVICES + self, + expected_dir: str, + actual_dir: str, + devices=DEFAULT_DEVICES, # type: dict[str, list[Device]] ) -> bool: """Compares the list of devices in expected_dir against actual_dir @@ -91,8 +94,8 @@ def compare_device_lists( ) for device in devices["required"]: # file paths of the expected and actual device lists - expected = "{}/{}_log".format(expected_dir, device) - actual = "{}/{}_log".format(actual_dir, device) + expected = "{}/{}_log".format(expected_dir, device.value) + actual = "{}/{}_log".format(actual_dir, device.value) if not filecmp.cmp(expected, actual): print( "The output of {} differs from the list gathered at the beginning of the session!".format( @@ -103,8 +106,8 @@ def compare_device_lists( return False for device in devices["optional"]: - expected = "{}/{}_log".format(expected_dir, device) - actual = "{}/{}_log".format(actual_dir, device) + expected = "{}/{}_log".format(expected_dir, device.value) + actual = "{}/{}_log".format(actual_dir, device.value) if not filecmp.cmp(expected, actual): print( "[WARN] Items under {} has changed.".format(actual), From bcd525732d52cc1193a55e7338908341cd8311bc Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Mon, 8 Jul 2024 16:16:04 +0800 Subject: [PATCH 12/36] refactor: separate tests into more classes --- .../base/tests/test_reboot_check_test.py | 172 ++++++++++++------ 1 file changed, 121 insertions(+), 51 deletions(-) diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index ed0766a590..68b629c07d 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -11,14 +11,9 @@ def do_nothing_run_cmd(_: T.List[str]): return RCT.ShellResult(0, "", "") -class RebootCheckTestTests(unittest.TestCase): - - @classmethod - def setUpClass(cls): - cls.temp_directory = "{}/temporary_dump_directory".format(os.getcwd()) - +class UnitySupportParserTests(unittest.TestCase): def test_parse_ok_unity_support_string(self): - OK_UNITY_STRING = """ + OK_UNITY_STRING = """\ OpenGL vendor string: Intel OpenGL renderer string: Mesa Intel(R) UHD Graphics (ICL GT1) OpenGL version string: 4.6 (Compatibility Profile) Mesa 23.2.1-1ubuntu3.1~22.04.2 @@ -76,39 +71,8 @@ def test_parse_bad_unity_support_string(self): # should return empty dict if input string literally doesn't make sense self.assertEqual(RCT.parse_unity_support_output(ARBITRARY_STRING), {}) - @patch("reboot_check_test.run_command") - def test_info_dump_only_happy_path(self, mock_run: MagicMock): - # manually override the return value based on arguments - def mock_run_command(args: T.List[str]) -> RCT.ShellResult: - stdout = "" - if args[0] == "iw": - stdout = """\ - addr some address - Interface some interface - ssid some ssid - """ - elif args[0] == "checkbox-support-lsusb": - stdout = """\ - usb1 - usb2 - usb3 - """ - elif args[0] == "lspci": - stdout = """\ - pci1 - pci2 - pci3 - """ - else: - raise Exception("Unexpected use of this mock") - - return RCT.ShellResult(0, stdout, "") - - # wrap over run_command's return value - mock_run.side_effect = mock_run_command - - RCT.DeviceInfoCollector().dump(self.temp_directory) +class DisplayConnectionTests(unittest.TestCase): def test_display_check_happy_path(self): with patch("os.listdir", return_value=["card0", "card1"]), patch( "builtins.open", @@ -131,7 +95,111 @@ def test_is_hardware_renderer_available(self): self.assertTrue(RCT.is_hardware_renderer_available()) @patch("reboot_check_test.run_command") - def test_main_function_logic(self, mock_run: MagicMock): + def test_get_display_id(self, mock_run: MagicMock): + with patch.dict(os.environ, {"DISPLAY": ":0"}): + self.assertEqual(RCT.get_display_id(), ":0") + + def create_side_effect( + display_server_name: 'T.Literal["wayland", "x11", "tty"]', + ): + def side_effect(args: T.List[str]): + stdout = "" + if args[0] == "loginctl": + stdout = "Type={}".format(display_server_name) + if args[0] == "pgrep": + stdout = "75632 /usr/bin/Xwayland :0 -rootless -noreset -accessx -core -auth /run/user/1000/.mutter-Xwaylandauth.FFE5P2" + if args[0] == "w": + stdout = "ubuntu :0 :0 13:43 13:55m 0.01s 0.00s /usr/libexec/gdm-wayland-session en" + + return RCT.ShellResult(0, stdout, "") + + return side_effect + + with patch.dict(os.environ, {"DISPLAY": ""}): + mock_run.side_effect = create_side_effect("wayland") + self.assertEqual(RCT.get_display_id(), ":0") + + mock_run.reset_mock() + mock_run.side_effect = create_side_effect("x11") + self.assertEqual(RCT.get_display_id(), ":0") + + +class InfoDumpTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.temp_output_dir = "{}/temp_output_dir".format(os.getcwd()) + cls.temp_comparison_dir = "{}/temp_comparison_dir".format(os.getcwd()) + + def mock_run_command(self, args: T.List[str]) -> RCT.ShellResult: + stdout = "" + if args[0] == "iw": + stdout = """\ + addr some address + Interface some interface + ssid some ssid + """ + elif args[0] == "checkbox-support-lsusb": + stdout = """\ + usb1 + usb2 + usb3 + """ + elif args[0] == "lspci": + stdout = """\ + pci1 + pci2 + pci3 + """ + else: + raise Exception("Unexpected use of this mock") + + return RCT.ShellResult(0, stdout, "") + + @patch("reboot_check_test.run_command") + def test_info_dump_only_happy_path(self, mock_run: MagicMock): + # wrap over run_command's return value + mock_run.side_effect = self.mock_run_command + RCT.DeviceInfoCollector().dump(self.temp_output_dir) + + @patch("reboot_check_test.run_command") + def test_info_dump_and_comparison_happy_path(self, mock_run: MagicMock): + mock_run.side_effect = self.mock_run_command + + collector = RCT.DeviceInfoCollector() + + collector.dump(self.temp_comparison_dir) + collector.dump(self.temp_output_dir) + + self.assertTrue( + collector.compare_device_lists( + self.temp_comparison_dir, self.temp_output_dir + ) + ) + + with open( + "{}/wireless_log".format(self.temp_comparison_dir), "a" + ) as f: + f.write("extra text that shouldn't be there") + + self.assertFalse( + collector.compare_device_lists( + self.temp_comparison_dir, self.temp_output_dir + ) + ) + + @classmethod + def tearDownClass(cls): + shutil.rmtree(cls.temp_output_dir) + shutil.rmtree(cls.temp_comparison_dir) + + +class MainFunctionTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.temp_directory = "{}/temporary_dump_directory".format(os.getcwd()) + + @patch("reboot_check_test.run_command") + def test_partial_main(self, mock_run: MagicMock): # this test only validates the main function logic(if it picks out the correct tests to run) mock_run.side_effect = do_nothing_run_cmd @@ -156,12 +224,16 @@ def test_main_function_logic(self, mock_run: MagicMock): "reboot_check_test.DeviceInfoCollector.compare_device_lists" ) as mock_compare: RCT.main() - print(mock_run.call_count, mock_run.call_args_list) - print(mock_compare.call_count, mock_compare.call_args_list) + # print(mock_run.call_count, mock_run.call_args_list) + # print(mock_compare.call_count, mock_compare.call_args_list) + @patch("reboot_check_test.get_display_id") @patch("reboot_check_test.run_command") - def test_main_function_full(self, mock_run: MagicMock): + def test_main_function_full( + self, mock_run: MagicMock, mock_get_display_id: MagicMock + ): mock_run.side_effect = do_nothing_run_cmd + mock_get_display_id.return_value = ":0" # Full suite with patch( "sys.argv", @@ -175,8 +247,9 @@ def test_main_function_full(self, mock_run: MagicMock): ) as mock_compare, patch( "reboot_check_test.is_fwts_supported" ) as mock_fwts_support_check: - mock_fwts_support_check.side_effect = lambda: True + mock_fwts_support_check.return_value = True RCT.main() + self.assertTrue(mock_compare.called) self.assertTrue(mock_fwts_support_check.called) @@ -187,17 +260,14 @@ def test_main_function_full(self, mock_run: MagicMock): actual = set() for call in mock_run.call_args_list: - # it's really ugly, but [0] takes the 1st from (command, args, kwargs), + # [0] takes the 1st from (args, kwargs, ) = call, # then take tha actual list from args # then take the 1st element, which is the command name actual.add(call[0][0][0]) - # value doesn't matter, we just want to compare the keys - self.assertDictContainsSubset( - dict.fromkeys(expected_commands), dict.fromkeys(actual) - ) + # <= is an overloaded operator for sets that checks the isSubset relation + self.assertLessEqual(expected_commands, actual) - @classmethod def tearDownClass(cls): - shutil.rmtree("{}/temporary_dump_directory".format(os.getcwd())) + shutil.rmtree(cls.temp_directory) From af61d190109768edb3c4feff082bdfbe57f96d4e Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Mon, 8 Jul 2024 16:48:09 +0800 Subject: [PATCH 13/36] refactor: add clenup methods --- .../base/tests/test_reboot_check_test.py | 41 +++++++++++++++---- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index 68b629c07d..0e30df72a2 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -130,6 +130,10 @@ def setUpClass(cls): cls.temp_output_dir = "{}/temp_output_dir".format(os.getcwd()) cls.temp_comparison_dir = "{}/temp_comparison_dir".format(os.getcwd()) + def tearDown(self): + shutil.rmtree(self.temp_output_dir, ignore_errors=True) + shutil.rmtree(self.temp_comparison_dir, ignore_errors=True) + def mock_run_command(self, args: T.List[str]) -> RCT.ShellResult: stdout = "" if args[0] == "iw": @@ -176,8 +180,13 @@ def test_info_dump_and_comparison_happy_path(self, mock_run: MagicMock): ) ) + # required with open( - "{}/wireless_log".format(self.temp_comparison_dir), "a" + "{}/{}_log".format( + self.temp_comparison_dir, + RCT.DeviceInfoCollector.Device.WIRELESS.value, + ), + "w", ) as f: f.write("extra text that shouldn't be there") @@ -186,11 +195,23 @@ def test_info_dump_and_comparison_happy_path(self, mock_run: MagicMock): self.temp_comparison_dir, self.temp_output_dir ) ) + + collector.dump(self.temp_comparison_dir) - @classmethod - def tearDownClass(cls): - shutil.rmtree(cls.temp_output_dir) - shutil.rmtree(cls.temp_comparison_dir) + # optional + with open( + "{}/{}_log".format( + self.temp_comparison_dir, RCT.DeviceInfoCollector.Device.DRM.value + ), + "w", + ) as f: + f.write("extra text that shouldn't be there") + + self.assertTrue( + collector.compare_device_lists( + self.temp_comparison_dir, self.temp_output_dir + ) + ) class MainFunctionTests(unittest.TestCase): @@ -213,6 +234,8 @@ def test_partial_main(self, mock_run: MagicMock): len(RCT.DeviceInfoCollector.DEFAULT_DEVICES["required"]), ) + mock_run.reset_mock() + with patch( "sys.argv", sh_split( @@ -224,8 +247,12 @@ def test_partial_main(self, mock_run: MagicMock): "reboot_check_test.DeviceInfoCollector.compare_device_lists" ) as mock_compare: RCT.main() - # print(mock_run.call_count, mock_run.call_args_list) - # print(mock_compare.call_count, mock_compare.call_args_list) + + self.assertEqual( + mock_run.call_count, + len(RCT.DeviceInfoCollector.DEFAULT_DEVICES["required"]), + ) # only lspci, lsusb, iw calls + self.assertEqual(mock_compare.call_count, 1) @patch("reboot_check_test.get_display_id") @patch("reboot_check_test.run_command") From b8d5ada728cb9625ce5b401ab163d0a441d02808 Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Tue, 9 Jul 2024 11:03:36 +0800 Subject: [PATCH 14/36] refactor: group tests into classes --- providers/base/bin/reboot_check_test.py | 328 ++++++++++++------------ 1 file changed, 165 insertions(+), 163 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 7fc06613dd..6c65d8bcfd 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -10,7 +10,6 @@ import enum import typing as T - # Checkbox could run in a snap container, so we need to prepend this root path RUNTIME_ROOT = os.getenv("CHECKBOX_RUNTIME", default="") # Snap mount point, see https://snapcraft.io/docs/environment-variables#heading--snap @@ -99,7 +98,7 @@ def compare_device_lists( if not filecmp.cmp(expected, actual): print( "The output of {} differs from the list gathered at the beginning of the session!".format( - device + device.value ), file=sys.stderr, ) @@ -110,8 +109,7 @@ def compare_device_lists( actual = "{}/{}_log".format(actual_dir, device.value) if not filecmp.cmp(expected, actual): print( - "[WARN] Items under {} has changed.".format(actual), - "If this machine dynamically switches between GPUs, this might be expected", + "[ WARN ] Items under {} has changed.".format(actual), file=sys.stderr, ) @@ -167,37 +165,37 @@ def run_command(args: T.List[str]) -> ShellResult: # since utf-8 is backwards compatible with ascii -def is_fwts_supported() -> bool: - return shutil.which("fwts") is not None - +class FwtsTester: + def is_fwts_supported(self) -> bool: + return shutil.which("fwts") is not None -def fwts_log_check_passed( - output_directory: str, fwts_arguments=["klog", "oops"] -) -> bool: - """Check if fwts logs passes the checks specified in sleep_test_log_check.py. - This script live in the same directory + def fwts_log_check_passed( + self, output_directory: str, fwts_arguments=["klog", "oops"] + ) -> bool: + """Check if fwts logs passes the checks specified in sleep_test_log_check.py. + This script live in the same directory - :param output_directory: where the output of fwts should be redirected to - :type output_directory: str - :return: whether sleep_test_log_check.py returned 0 (success) - :rtype: bool - """ - log_file_path = "{}/fwts_{}.log".format( - output_directory, "_".join(fwts_arguments) - ) - run_command(["fwts", "-r", log_file_path, *fwts_arguments]) - result = run_command( - [ - "sleep_test_log_check.py", - "-v", - "--ignore-warning", - "-t", - "all", - log_file_path, - ] - ) + :param output_directory: where the output of fwts should be redirected to + :type output_directory: str + :return: whether sleep_test_log_check.py returned 0 (success) + :rtype: bool + """ + log_file_path = "{}/fwts_{}.log".format( + output_directory, "_".join(fwts_arguments) + ) + run_command(["fwts", "-r", log_file_path, *fwts_arguments]) + result = run_command( + [ + "sleep_test_log_check.py", + "-v", + "--ignore-warning", + "-t", + "all", + log_file_path, + ] + ) - return result.return_code == 0 + return result.return_code == 0 def get_failed_services() -> T.List[str]: @@ -215,7 +213,7 @@ def get_failed_services() -> T.List[str]: "--state=failed", ] # only print the names of the services that failed - return run_command(command).stdout.split() + return run_command(command).stdout.splitlines() def create_parser(): @@ -273,155 +271,157 @@ def remove_color_code(string: str) -> str: return re.sub(r"\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])", "", string) -def get_display_id() -> T.Optional[str]: - """Returns the active display id - https://github.com/canonical/checkbox/blob/main/contrib/pc-sanity/bin/renderer-mesa-driver-check.sh +class HardwareRendererTester: - :return: the display id, usually ":0" if there's only 1 display - :rtype: str - """ - DISPLAY = os.getenv("DISPLAY", default="") + def get_display_id(self) -> T.Optional[str]: + """Returns the active display id + https://github.com/canonical/checkbox/blob/main/contrib/pc-sanity/bin/renderer-mesa-driver-check.sh - if DISPLAY != "": - print("Using $DISPLAY env variable: {}".format(DISPLAY)) - return DISPLAY # use the environment var if non-empty + :return: the display id, usually ":0" if there's only 1 display + :rtype: str | None when there's no display + """ + DISPLAY = os.getenv("DISPLAY", default="") - session_id = ( - run_command(["loginctl", "list-sessions", "--no-legend"]) - .stdout.split()[0] - .strip() - # string is guaranteed to be non-empty, at least 1 user is logged in - ) + if DISPLAY != "": + print("Using $DISPLAY env variable: {}".format(DISPLAY)) + return DISPLAY # use the environment var if non-empty - display_server_type = ( - run_command(["loginctl", "show-session", session_id, "-p", "Type"]) - .stdout.split("=")[1] - .strip() # it should look like Type=wayland, split and take 2nd word - ) + session_id = ( + run_command(["loginctl", "list-sessions", "--no-legend"]) + .stdout.split()[0] + .strip() + # string is guaranteed to be non-empty, at least 1 user is logged in + ) - # NOTE: Xwayland doesn't immediately start after a reboot - # For now, we will assume :0 exists and use it as the display id - if display_server_type == "wayland": - pgrep_out = run_command(["pgrep", "-a", "Xwayland"]) - # search for a process called Xwayland, take the 3rd arg, which is the display id - if pgrep_out.return_code == 0: - return pgrep_out.stdout.split()[2] - else: - print( - "[WARN] Waylad session detected, but Xwayland process is not found. Assuming :0 display", - file=sys.stderr, - ) - return ":0" + display_server_type = ( + run_command(["loginctl", "show-session", session_id, "-p", "Type"]) + .stdout.split("=")[1] + .strip() # it should look like Type=wayland, split and take 2nd word + ) - if display_server_type == "x11": - w_out = run_command(["w", "--no-header"]) - if w_out.return_code == 0 and len(w_out.stdout) != 0: - return w_out.stdout.split()[2] - return None + # NOTE: Xwayland won't start until the first x11 app runs + # For now, we will assume :0 exists and use it as the display id + if display_server_type == "wayland": + pgrep_out = run_command(["pgrep", "-a", "Xwayland"]) + pgrep_args = pgrep_out.stdout.split() + # search for a process called Xwayland, take the 3rd arg, which is the display id + if pgrep_out.return_code == 0 and len(pgrep_args) >= 3: + return pgrep_args[2] + else: + print( + "[ WARN ] Waylad session detected, but Xwayland process is not found. Assuming :0 display", + file=sys.stderr, + ) + return ":0" - return None # Unsupported window system + if display_server_type == "x11": + w_out = run_command(["w", "--no-header"]) + w_out_split = w_out.stdout.split() + if w_out.return_code == 0 and len(w_out_split) >= 3: + return w_out.stdout.split()[2] + return None # Unsupported window system, or it's tty -def has_display_connection() -> bool: - """Checks if a display is connected by searching the /sys/class/drm directory + def has_display_connection(self) -> bool: + """Checks if a display is connected by searching the /sys/class/drm directory - :return: True if there's at least 1 node that is connected - """ + :return: True if there's at least 1 node that is connected + """ + + # look for GPU file nodes first + DRM_PATH = "/sys/class/drm" + possible_gpu_nodes = os.listdir(DRM_PATH) + if len(possible_gpu_nodes) == 0 or possible_gpu_nodes == ["version"]: + # kernel doesn't see any GPU nodes + print( + "There's nothing under {}".format(DRM_PATH), + "if an external GPU is connected, check if the connection is loose", + ) + return False + + print("These nodes", possible_gpu_nodes, "exist") + print("Checking for display connection...") + + for gpu in possible_gpu_nodes: + # for each gpu, check for connection, return true if anything is connected + try: + with open("{}/{}/status".format(DRM_PATH, gpu)) as status_file: + if status_file.read().strip().lower() == "connected": + print("{} is connected to display!".format(gpu)) + return True + except FileNotFoundError: + # this just means we don't have a status file => no connection, continue to the next + pass + except Exception as e: + print("Unexpected error: ", e, file=sys.stderr) - # look for GPU file nodes first - DRM_PATH = "/sys/class/drm" - possible_gpu_nodes = os.listdir(DRM_PATH) - if len(possible_gpu_nodes) == 0 or possible_gpu_nodes == ["version"]: - # kernel doesn't see any GPU nodes print( - "There's nothing under {}".format(DRM_PATH), - "if an external GPU is connected, check if the connection is loose", + "No display is connected. This case will be skipped.", + "Maybe the display cable is not connected?", ) return False - print("These nodes", possible_gpu_nodes, "exist") - print("Checking for display connection...") - - for gpu in possible_gpu_nodes: - # for each gpu, check for connection, return true if anything is connected - try: - status_file = open("{}/{}/status".format(DRM_PATH, gpu)) - if status_file.read().strip().lower() == "connected": - print("{} is connected to display!".format(gpu)) - return True - except FileNotFoundError: - # this just means we don't have a status file => no connection, continue to the next - pass - except Exception as e: - print("Unexpected error: ", e, file=sys.stderr) - - print( - "No display is connected. This case will be skipped.", - "Maybe the display cable is not connected?", - ) - return False - + def is_hardware_renderer_available(self) -> bool: + """Checks if hardware rendering is being used. THIS ASSUMES A DRM CONNECTION EXISTS -def is_hardware_renderer_available() -> bool: - """Checks if hardware rendering is being used. THIS ASSUMES A DRM CONNECTION EXISTS - - :return: True if a hardware renderer is active, otherwise return False - :rtype: bool - """ - - # Now we know some kind of display exists, run unity_support_test - display_id = get_display_id() - if display_id is None: - print("No display id was found.", file=sys.stderr) - # No display id was found - return False + :return: True if a hardware renderer is active, otherwise return False + :rtype: bool + """ - print("Checking display id: {}".format(display_id)) + # Now we know some kind of display exists, run unity_support_test + display_id = self.get_display_id() + if display_id is None: + print("No display id was found.", file=sys.stderr) + # No display id was found + return False - unity_support_output = run_command( - [ - "{}/usr/lib/nux/unity_support_test".format(RUNTIME_ROOT), - "-p", - "-display", - display_id, - ] - ) - if unity_support_output.return_code != 0: - return False + print("Checking display id: {}".format(display_id)) - is_hardware_rendered = ( - parse_unity_support_output(unity_support_output.stdout).get( - "Not software rendered" + unity_support_output = run_command( + [ + "{}/usr/lib/nux/unity_support_test".format(RUNTIME_ROOT), + "-p", + "-display", + display_id, + ] ) - == "yes" - ) - if is_hardware_rendered: - print("[ OK ] This machine is using a hardware renderer!") - return True + if unity_support_output.return_code != 0: + return False - return False + is_hardware_rendered = ( + self.parse_unity_support_output(unity_support_output.stdout).get( + "Not software rendered" + ) + == "yes" + ) + if is_hardware_rendered: + print("[ OK ] This machine is using a hardware renderer!") + return True + return False -def parse_unity_support_output(unity_output_string: str) -> T.Dict[str, str]: - """Parses the output of `unity_support_test` into a dictionary + def parse_unity_support_output( + self, unity_output_string: str + ) -> T.Dict[str, str]: + """Parses the output of `unity_support_test` into a dictionary - :param output_string: the raw output from running `unity_support_test -p` - :type output_string: str - :return: string key-value pairs that mirror the output of unity_support_test. - Left hand side of the first colon are the keys; right hand side are the values. - :rtype: dict[str, str] - """ + :param output_string: the raw output from running `unity_support_test -p` + :type output_string: str + :return: string key-value pairs that mirror the output of unity_support_test. + Left hand side of the first colon are the keys; right hand side are the values. + :rtype: dict[str, str] + """ - output = {} # type: dict[str, str] - for line in unity_output_string.split("\n"): - # max_split=1 to prevent splitting the string after the 1st colon - words = line.split(":", maxsplit=1) - if len(words) == 2: - key = words[0].strip() - value = remove_color_code(words[1].strip()) - output[key] = value + output = {} # type: dict[str, str] + for line in unity_output_string.split("\n"): + # max_split=1 to prevent splitting the string after the 1st colon + words = line.split(":", maxsplit=1) + if len(words) == 2: + key = words[0].strip() + value = remove_color_code(words[1].strip()) + output[key] = value - return output + return output def main() -> int: @@ -432,7 +432,6 @@ def main() -> int: """ args = create_parser().parse_args() - print(args) # all 4 tests pass by default # they only fail if their respective flags are specified @@ -460,14 +459,13 @@ def main() -> int: DeviceInfoCollector().dump(args.output_directory) if args.do_fwts_check: - if is_fwts_supported() and not fwts_log_check_passed( + tester = FwtsTester() + if tester.is_fwts_supported() and not tester.fwts_log_check_passed( args.output_directory ): fwts_passed = False if args.do_service_check: - print("Checking for failed system services...") - failed_services = get_failed_services() if len(failed_services) > 0: print( @@ -476,9 +474,13 @@ def main() -> int: ) service_check_passed = False - if args.do_renderer_check and has_display_connection(): - # skip renderer test if there's no display - renderer_test_passed = is_hardware_renderer_available() + print("[ OK ] All system services are running!") + + if args.do_renderer_check: + tester = HardwareRendererTester() + if tester.has_display_connection(): + # skip renderer test if there's no display + renderer_test_passed = tester.is_hardware_renderer_available() if ( fwts_passed From b819e3ad14987e82b068af6a86e90fb072ad4d55 Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Tue, 9 Jul 2024 14:03:03 +0800 Subject: [PATCH 15/36] fix: update tests to reflect changes in reboot_check_test --- .../base/tests/test_reboot_check_test.py | 173 ++++++++++++------ 1 file changed, 122 insertions(+), 51 deletions(-) diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index 0e30df72a2..4147a10dc3 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -7,11 +7,14 @@ import typing as T -def do_nothing_run_cmd(_: T.List[str]): +def do_nothing(_: T.List[str]): return RCT.ShellResult(0, "", "") class UnitySupportParserTests(unittest.TestCase): + def setUp(self): + self.tester = RCT.HardwareRendererTester() + def test_parse_ok_unity_support_string(self): OK_UNITY_STRING = """\ OpenGL vendor string: Intel @@ -49,7 +52,7 @@ def test_parse_ok_unity_support_string(self): "Unity 3D supported": "yes", } - actual = RCT.parse_unity_support_output(OK_UNITY_STRING) + actual = self.tester.parse_unity_support_output(OK_UNITY_STRING) self.assertDictEqual(expected, actual) def test_parse_bad_unity_support_string(self): @@ -59,7 +62,7 @@ def test_parse_bad_unity_support_string(self): OpenGL version string 4.6 (Compatibility Profile) Mesa 23.2.1-1ubuntu3.1~22.04.2 GL version is 1.4+% \x1B[033myes\x1B[0m """ - actual = RCT.parse_unity_support_output(BAD_UNITY_STRING) + actual = self.tester.parse_unity_support_output(BAD_UNITY_STRING) expected = { "OpenGL renderer string": "Mesa Intel(R) UHD Graphics (ICL GT1)", @@ -69,59 +72,99 @@ def test_parse_bad_unity_support_string(self): ARBITRARY_STRING = "askjaskdnasdn" # should return empty dict if input string literally doesn't make sense - self.assertEqual(RCT.parse_unity_support_output(ARBITRARY_STRING), {}) + self.assertEqual( + self.tester.parse_unity_support_output(ARBITRARY_STRING), + {}, + ) class DisplayConnectionTests(unittest.TestCase): + + def setUp(self) -> None: + self.tester = RCT.HardwareRendererTester() + + def create_side_effect( + self, + display_server_name: 'T.Literal["wayland", "x11", "tty"]', + happy_path=True, + ): + def side_effect(args: T.List[str]): + stdout = "" + if args[0] == "loginctl": + stdout = "Type={}".format(display_server_name) + if args[0] == "pgrep": + if happy_path: + stdout = "75632 /usr/bin/Xwayland :0 -rootless -noreset -accessx -core -auth /run/user/1000/.mutter-Xwaylandauth.FFE5P2" + else: + stdout = "" # Xwayland is not guaranteed to exist + if args[0] == "w": + if happy_path: + stdout = "ubuntu :0 :0 13:43 13:55m 0.01s 0.00s /usr/libexec/gdm-wayland-session en" + else: + stdout = "ubuntu tty2 tty2 Mon13 35:51m 0.01s 0.00s /usr/libexec/gdm-wayland-session en" + + return RCT.ShellResult(0, stdout, "") + + return side_effect + def test_display_check_happy_path(self): - with patch("os.listdir", return_value=["card0", "card1"]), patch( + with patch( + "os.listdir", return_value=["fakeCard0", "fakeCard1"] + ), patch( "builtins.open", new_callable=mock_open, read_data="connected", ): - self.assertTrue(RCT.has_display_connection()) + self.assertTrue(self.tester.has_display_connection()) def test_display_check_no_display_path(self): with patch("os.listdir", return_value=["version"]): - self.assertFalse(RCT.has_display_connection()) - with patch("os.listdir", return_value=["card0", "card1"]), patch( + self.assertFalse(self.tester.has_display_connection()) + with patch( + "os.listdir", return_value=["fakeCard0", "fakeCard1"] + ), patch( "builtins.open", new_callable=mock_open, read_data="not connected", ): - self.assertFalse(RCT.has_display_connection()) + self.assertFalse(self.tester.has_display_connection()) - def test_is_hardware_renderer_available(self): - self.assertTrue(RCT.is_hardware_renderer_available()) + @patch("reboot_check_test.HardwareRendererTester.get_display_id") + @patch( + "reboot_check_test.HardwareRendererTester.parse_unity_support_output" + ) + @patch("reboot_check_test.run_command") + def test_is_hardware_renderer_available( + self, + mock_run: MagicMock, + mock_parse: MagicMock, + mock_get_display_id: MagicMock, + ): + mock_run.side_effect = do_nothing + mock_get_display_id.return_value = ":0" + mock_parse.return_value = { + "Not software rendered": "yes", + } + tester = RCT.HardwareRendererTester() + self.assertTrue(tester.is_hardware_renderer_available()) @patch("reboot_check_test.run_command") def test_get_display_id(self, mock_run: MagicMock): with patch.dict(os.environ, {"DISPLAY": ":0"}): - self.assertEqual(RCT.get_display_id(), ":0") - - def create_side_effect( - display_server_name: 'T.Literal["wayland", "x11", "tty"]', - ): - def side_effect(args: T.List[str]): - stdout = "" - if args[0] == "loginctl": - stdout = "Type={}".format(display_server_name) - if args[0] == "pgrep": - stdout = "75632 /usr/bin/Xwayland :0 -rootless -noreset -accessx -core -auth /run/user/1000/.mutter-Xwaylandauth.FFE5P2" - if args[0] == "w": - stdout = "ubuntu :0 :0 13:43 13:55m 0.01s 0.00s /usr/libexec/gdm-wayland-session en" + self.assertEqual(self.tester.get_display_id(), ":0") - return RCT.ShellResult(0, stdout, "") + with patch.dict(os.environ, {"DISPLAY": ""}): + mock_run.side_effect = self.create_side_effect("wayland") + self.assertEqual(self.tester.get_display_id(), ":0") - return side_effect + mock_run.side_effect = self.create_side_effect("wayland", False) + self.assertEqual(self.tester.get_display_id(), ":0") - with patch.dict(os.environ, {"DISPLAY": ""}): - mock_run.side_effect = create_side_effect("wayland") - self.assertEqual(RCT.get_display_id(), ":0") + mock_run.side_effect = self.create_side_effect("x11") + self.assertEqual(self.tester.get_display_id(), ":0") - mock_run.reset_mock() - mock_run.side_effect = create_side_effect("x11") - self.assertEqual(RCT.get_display_id(), ":0") + mock_run.side_effect = self.create_side_effect("x11", False) + self.assertIsNone(self.tester.get_display_id()) class InfoDumpTests(unittest.TestCase): @@ -195,13 +238,14 @@ def test_info_dump_and_comparison_happy_path(self, mock_run: MagicMock): self.temp_comparison_dir, self.temp_output_dir ) ) - + collector.dump(self.temp_comparison_dir) # optional with open( "{}/{}_log".format( - self.temp_comparison_dir, RCT.DeviceInfoCollector.Device.DRM.value + self.temp_comparison_dir, + RCT.DeviceInfoCollector.Device.DRM.value, ), "w", ) as f: @@ -214,19 +258,45 @@ def test_info_dump_and_comparison_happy_path(self, mock_run: MagicMock): ) +class FailedServiceCheckerTests(unittest.TestCase): + + @patch("reboot_check_test.run_command") + def test_get_failed_services_happy_path(self, mock_run: MagicMock): + mock_run.return_value = RCT.ShellResult(0, "", "") + self.assertEqual(RCT.get_failed_services(), []) + + @patch("reboot_check_test.run_command") + def test_get_failed_services_with_failed_services( + self, mock_run: MagicMock + ): + mock_run.return_value = RCT.ShellResult( + 0, + "snap.checkbox.agent.service loaded failed failed Service for snap applictaion checkbox.agent", + "", + ) + self.assertEqual(RCT.get_failed_services(), [mock_run.return_value.stdout]) + + class MainFunctionTests(unittest.TestCase): @classmethod def setUpClass(cls): - cls.temp_directory = "{}/temporary_dump_directory".format(os.getcwd()) + cls.temp_output_dir = "{}/temp_output_dir".format(os.getcwd()) + cls.temp_comparison_dir = "{}/temp_comparison_dir".format(os.getcwd()) + + def tearDown(self): + shutil.rmtree(self.temp_output_dir, ignore_errors=True) + shutil.rmtree(self.temp_comparison_dir, ignore_errors=True) @patch("reboot_check_test.run_command") def test_partial_main(self, mock_run: MagicMock): # this test only validates the main function logic(if it picks out the correct tests to run) - mock_run.side_effect = do_nothing_run_cmd + mock_run.side_effect = do_nothing with patch( "sys.argv", - sh_split("reboot_check_test.py -d {}".format(self.temp_directory)), + sh_split( + "reboot_check_test.py -d {}".format(self.temp_output_dir) + ), ): RCT.main() self.assertEqual( @@ -240,7 +310,7 @@ def test_partial_main(self, mock_run: MagicMock): "sys.argv", sh_split( 'reboot_check_test.py -d "{}" -c "{}"'.format( - self.temp_directory, "some dir" + self.temp_output_dir, self.temp_comparison_dir ) ), ), patch( @@ -254,35 +324,38 @@ def test_partial_main(self, mock_run: MagicMock): ) # only lspci, lsusb, iw calls self.assertEqual(mock_compare.call_count, 1) - @patch("reboot_check_test.get_display_id") + @patch("reboot_check_test.HardwareRendererTester.get_display_id") @patch("reboot_check_test.run_command") def test_main_function_full( self, mock_run: MagicMock, mock_get_display_id: MagicMock ): - mock_run.side_effect = do_nothing_run_cmd + mock_run.side_effect = do_nothing mock_get_display_id.return_value = ":0" # Full suite with patch( "sys.argv", sh_split( 'reboot_check_test.py -d "{}" -c "{}" -f -s -g'.format( - self.temp_directory, "some dir" + self.temp_output_dir, self.temp_comparison_dir ) ), ), patch( "reboot_check_test.DeviceInfoCollector.compare_device_lists" ) as mock_compare, patch( - "reboot_check_test.is_fwts_supported" - ) as mock_fwts_support_check: - mock_fwts_support_check.return_value = True + "reboot_check_test.FwtsTester.is_fwts_supported" + ) as mock_is_fwts_supported: + mock_is_fwts_supported.return_value = True + mock_compare.return_value = True + RCT.main() - self.assertTrue(mock_compare.called) - self.assertTrue(mock_fwts_support_check.called) + # self.assertTrue(mock_compare.called) + self.assertTrue(mock_is_fwts_supported.called) expected_commands = { "systemctl", "sleep_test_log_check.py", + "fwts", } actual = set() @@ -293,8 +366,6 @@ def test_main_function_full( actual.add(call[0][0][0]) # <= is an overloaded operator for sets that checks the isSubset relation - self.assertLessEqual(expected_commands, actual) - - @classmethod - def tearDownClass(cls): - shutil.rmtree(cls.temp_directory) + self.assertLessEqual( + expected_commands, actual, "should be a subset" + ) \ No newline at end of file From be80ed91eab0a4372ccec4e314579f8b1ae0c235 Mon Sep 17 00:00:00 2001 From: Zhongning Li Date: Tue, 9 Jul 2024 14:09:59 +0800 Subject: [PATCH 16/36] style: formatting, comments --- providers/base/bin/reboot_check_test.py | 31 +++++++++++++++++-------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 6c65d8bcfd..ce861013ae 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -40,7 +40,7 @@ class Device(enum.Enum): Device.USB, ], # these can fail the test case "optional": [Device.DRM], # these only produce warnings - } # used for comparison and dump calls + } # type: dict[T.Literal['required','optional'], list[Device]] # to modify, add more values in the enum and reference them in required/optional respectively def get_drm_info(self) -> str: @@ -78,7 +78,7 @@ def compare_device_lists( self, expected_dir: str, actual_dir: str, - devices=DEFAULT_DEVICES, # type: dict[str, list[Device]] + devices=DEFAULT_DEVICES, ) -> bool: """Compares the list of devices in expected_dir against actual_dir @@ -109,7 +109,7 @@ def compare_device_lists( actual = "{}/{}_log".format(actual_dir, device.value) if not filecmp.cmp(expected, actual): print( - "[ WARN ] Items under {} has changed.".format(actual), + "[ WARN ] Items under {} have changed.".format(actual), file=sys.stderr, ) @@ -211,7 +211,8 @@ def get_failed_services() -> T.List[str]: "--no-pager", "--no-legend", "--state=failed", - ] # only print the names of the services that failed + "--plain", # plaintext, otherwise it includes color codes + ] return run_command(command).stdout.splitlines() @@ -305,7 +306,11 @@ def get_display_id(self) -> T.Optional[str]: pgrep_out = run_command(["pgrep", "-a", "Xwayland"]) pgrep_args = pgrep_out.stdout.split() # search for a process called Xwayland, take the 3rd arg, which is the display id - if pgrep_out.return_code == 0 and len(pgrep_args) >= 3: + if ( + pgrep_out.return_code == 0 + and len(pgrep_args) >= 3 + and ":" in pgrep_args[2] + ): return pgrep_args[2] else: print( @@ -317,8 +322,12 @@ def get_display_id(self) -> T.Optional[str]: if display_server_type == "x11": w_out = run_command(["w", "--no-header"]) w_out_split = w_out.stdout.split() - if w_out.return_code == 0 and len(w_out_split) >= 3: - return w_out.stdout.split()[2] + if ( + w_out.return_code == 0 + and len(w_out_split) >= 3 + and ":" in w_out_split[2] + ): + return w_out_split[2] return None # Unsupported window system, or it's tty @@ -340,7 +349,6 @@ def has_display_connection(self) -> bool: return False print("These nodes", possible_gpu_nodes, "exist") - print("Checking for display connection...") for gpu in possible_gpu_nodes: # for each gpu, check for connection, return true if anything is connected @@ -371,7 +379,10 @@ def is_hardware_renderer_available(self) -> bool: # Now we know some kind of display exists, run unity_support_test display_id = self.get_display_id() if display_id is None: - print("No display id was found.", file=sys.stderr) + print( + "No display id was found. Does this system have a desktop environment?", + file=sys.stderr, + ) # No display id was found return False @@ -469,7 +480,7 @@ def main() -> int: failed_services = get_failed_services() if len(failed_services) > 0: print( - "These services failed: {}".format(failed_services), + "These services failed: {}".format("\n".join(failed_services)), file=sys.stderr, ) service_check_passed = False From e523901a2d8b18d7f16fc5b6498a9adc09c921c2 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Tue, 9 Jul 2024 14:50:29 +0800 Subject: [PATCH 17/36] fix: extra -g flag, add comments --- providers/base/bin/reboot_check_test.py | 5 +++-- providers/base/units/stress/boot.pxu | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index ce861013ae..bd6019b753 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -201,8 +201,8 @@ def fwts_log_check_passed( def get_failed_services() -> T.List[str]: """Counts the number of failed services listed in systemctl - :return: number of failed services - """ + :return: a list of failed services as they appear in systemctl + """ command = [ "systemctl", "list-units", @@ -371,6 +371,7 @@ def has_display_connection(self) -> bool: def is_hardware_renderer_available(self) -> bool: """Checks if hardware rendering is being used. THIS ASSUMES A DRM CONNECTION EXISTS + - self.has_display_connection() should be called first if unsure :return: True if a hardware renderer is active, otherwise return False :rtype: bool diff --git a/providers/base/units/stress/boot.pxu b/providers/base/units/stress/boot.pxu index f2b4a00552..96d80bae6a 100644 --- a/providers/base/units/stress/boot.pxu +++ b/providers/base/units/stress/boot.pxu @@ -46,7 +46,7 @@ _purpose: This creates baseline data sets which will be considered the master unit: job plugin: shell command: - reboot_check_test.py -g -d "$PLAINBOX_SESSION_SHARE/before_reboot" + reboot_check_test.py -d "$PLAINBOX_SESSION_SHARE/before_reboot" environ: LD_LIBRARY_PATH user: root estimated_duration: 1s From 19ac79b05d3d8e045a9c3f5ee2bf73d658d37c98 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Fri, 12 Jul 2024 14:50:25 +0800 Subject: [PATCH 18/36] fix: remove get_display_id due to too many edge cases --- providers/base/bin/reboot_check_test.py | 102 +++++++----------------- 1 file changed, 27 insertions(+), 75 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index bd6019b753..f728c9c7f7 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -10,9 +10,11 @@ import enum import typing as T + # Checkbox could run in a snap container, so we need to prepend this root path RUNTIME_ROOT = os.getenv("CHECKBOX_RUNTIME", default="") -# Snap mount point, see https://snapcraft.io/docs/environment-variables#heading--snap +# Snap mount point, see +# https://snapcraft.io/docs/environment-variables#heading--snap SNAP = os.getenv("SNAP", default="") @@ -157,7 +159,7 @@ def run_command(args: T.List[str]) -> ShellResult: out = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) return ShellResult( return_code=out.returncode, - # if there's nothing on stdout, .stdout is None, so we need a default value + # if there's nothing on stdout, .stdout is None (<=3.7 behavior), so we need a default value stdout=(out.stdout or b"").decode(), stderr=(out.stderr or b"").decode(), ) @@ -202,7 +204,7 @@ def get_failed_services() -> T.List[str]: """Counts the number of failed services listed in systemctl :return: a list of failed services as they appear in systemctl - """ + """ command = [ "systemctl", "list-units", @@ -225,7 +227,7 @@ def create_parser(): parser.add_argument( "-d", "--dump-to", - required=True, + required=False, dest="output_directory", help="Absolute path to the output directory. Device info-dumps will be written here.", ) @@ -274,62 +276,16 @@ def remove_color_code(string: str) -> str: class HardwareRendererTester: - def get_display_id(self) -> T.Optional[str]: - """Returns the active display id - https://github.com/canonical/checkbox/blob/main/contrib/pc-sanity/bin/renderer-mesa-driver-check.sh - - :return: the display id, usually ":0" if there's only 1 display - :rtype: str | None when there's no display - """ - DISPLAY = os.getenv("DISPLAY", default="") - - if DISPLAY != "": - print("Using $DISPLAY env variable: {}".format(DISPLAY)) - return DISPLAY # use the environment var if non-empty - - session_id = ( - run_command(["loginctl", "list-sessions", "--no-legend"]) - .stdout.split()[0] - .strip() - # string is guaranteed to be non-empty, at least 1 user is logged in - ) - - display_server_type = ( - run_command(["loginctl", "show-session", session_id, "-p", "Type"]) - .stdout.split("=")[1] - .strip() # it should look like Type=wayland, split and take 2nd word - ) - - # NOTE: Xwayland won't start until the first x11 app runs - # For now, we will assume :0 exists and use it as the display id - if display_server_type == "wayland": - pgrep_out = run_command(["pgrep", "-a", "Xwayland"]) - pgrep_args = pgrep_out.stdout.split() - # search for a process called Xwayland, take the 3rd arg, which is the display id - if ( - pgrep_out.return_code == 0 - and len(pgrep_args) >= 3 - and ":" in pgrep_args[2] - ): - return pgrep_args[2] - else: - print( - "[ WARN ] Waylad session detected, but Xwayland process is not found. Assuming :0 display", - file=sys.stderr, - ) - return ":0" - - if display_server_type == "x11": - w_out = run_command(["w", "--no-header"]) - w_out_split = w_out.stdout.split() - if ( - w_out.return_code == 0 - and len(w_out_split) >= 3 - and ":" in w_out_split[2] - ): - return w_out_split[2] - - return None # Unsupported window system, or it's tty + def is_desktop_image(self) -> bool: + if not shutil.which("dpkg"): + # core and server image doesn't have dpkg + return False + # if we found any of these packages, we are on desktop + if run_command(["dpkg", "-l", "ubuntu-desktop"]).stdout == 0: + return True + if run_command(["dpkg", "-l", "ubuntu-desktop-minimal"]).stdout == 0: + return True + return False def has_display_connection(self) -> bool: """Checks if a display is connected by searching the /sys/class/drm directory @@ -339,8 +295,12 @@ def has_display_connection(self) -> bool: # look for GPU file nodes first DRM_PATH = "/sys/class/drm" - possible_gpu_nodes = os.listdir(DRM_PATH) - if len(possible_gpu_nodes) == 0 or possible_gpu_nodes == ["version"]: + possible_gpu_nodes = [ + directory + for directory in os.listdir(DRM_PATH) + if directory != "version" + ] + if len(possible_gpu_nodes) == 0: # kernel doesn't see any GPU nodes print( "There's nothing under {}".format(DRM_PATH), @@ -377,24 +337,16 @@ def is_hardware_renderer_available(self) -> bool: :rtype: bool """ - # Now we know some kind of display exists, run unity_support_test - display_id = self.get_display_id() - if display_id is None: - print( - "No display id was found. Does this system have a desktop environment?", - file=sys.stderr, + print( + "Checking display id: {}".format( + os.getenv("DISPLAY", ":0 (assume)") ) - # No display id was found - return False - - print("Checking display id: {}".format(display_id)) + ) unity_support_output = run_command( [ "{}/usr/lib/nux/unity_support_test".format(RUNTIME_ROOT), "-p", - "-display", - display_id, ] ) if unity_support_output.return_code != 0: @@ -490,7 +442,7 @@ def main() -> int: if args.do_renderer_check: tester = HardwareRendererTester() - if tester.has_display_connection(): + if tester.has_display_connection() and tester.is_desktop_image(): # skip renderer test if there's no display renderer_test_passed = tester.is_hardware_renderer_available() From 95d84665e9337003604c25b3770a53f442edaa01 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Fri, 12 Jul 2024 16:39:15 +0800 Subject: [PATCH 19/36] fix: extra wrapper for non-existent commands --- providers/base/bin/reboot_check_test.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index f728c9c7f7..2acb1fcdf0 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -156,7 +156,11 @@ def run_command(args: T.List[str]) -> ShellResult: :rtype: ShellResult """ # PIPE is needed for subprocess.run to capture stdout and stderr (<=3.7 behavior) - out = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + try: + out = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + except FileNotFoundError as e: + return ShellResult(1, '', "Command {} not found".format(args[0], e)) + return ShellResult( return_code=out.returncode, # if there's nothing on stdout, .stdout is None (<=3.7 behavior), so we need a default value @@ -277,14 +281,18 @@ def remove_color_code(string: str) -> str: class HardwareRendererTester: def is_desktop_image(self) -> bool: + print("Checking if this system is using a desktop image...", end='') if not shutil.which("dpkg"): # core and server image doesn't have dpkg return False # if we found any of these packages, we are on desktop - if run_command(["dpkg", "-l", "ubuntu-desktop"]).stdout == 0: + if run_command(["dpkg", "-l", "ubuntu-desktop"]).return_code == 0: + print('ok') return True - if run_command(["dpkg", "-l", "ubuntu-desktop-minimal"]).stdout == 0: + if run_command(["dpkg", "-l", "ubuntu-desktop-minimal"]).return_code == 0: + print('ok') return True + print('not desktop') return False def has_display_connection(self) -> bool: From b14458986fa64ba031a24ed973674d1dc3617622 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Fri, 12 Jul 2024 17:02:21 +0800 Subject: [PATCH 20/36] style: fit everything within 80 char limit --- providers/base/bin/reboot_check_test.py | 101 ++++++++++++++---------- 1 file changed, 61 insertions(+), 40 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 2acb1fcdf0..80f617b22f 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -19,7 +19,9 @@ class ShellResult: - """Wrapper class around the return value of run_command, guarantees non-null""" + """ + Wrapper class around the return value of run_command, guarantees non-null + """ def __init__(self, return_code: int, stdout: str, stderr: str): self.return_code = return_code @@ -43,7 +45,8 @@ class Device(enum.Enum): ], # these can fail the test case "optional": [Device.DRM], # these only produce warnings } # type: dict[T.Literal['required','optional'], list[Device]] - # to modify, add more values in the enum and reference them in required/optional respectively + # to modify, add more values in the enum + # and reference them in required/optional respectively def get_drm_info(self) -> str: return str(os.listdir("/sys/class/drm")) @@ -84,12 +87,13 @@ def compare_device_lists( ) -> bool: """Compares the list of devices in expected_dir against actual_dir - :param expected_dir: a directory of files containing the expected values the device list - :param actual_dir: a directory of files containing the actual device list + :param expected_dir: files containing the expected device list + :param actual_dir: files containing the actual device list :return: whether the device list matches """ print( - "Comparing device list files in (expected){} against (actual){}...".format( + "Comparing device lists in (expected){} against (actual){}..." + .format( expected_dir, actual_dir ) ) @@ -99,9 +103,7 @@ def compare_device_lists( actual = "{}/{}_log".format(actual_dir, device.value) if not filecmp.cmp(expected, actual): print( - "The output of {} differs from the list gathered at the beginning of the session!".format( - device.value - ), + "[ ERR ] The output of {} differs!".format(device.value), file=sys.stderr, ) return False @@ -155,20 +157,26 @@ def run_command(args: T.List[str]) -> ShellResult: :return: return code, stdout and stderr, all non-null :rtype: ShellResult """ - # PIPE is needed for subprocess.run to capture stdout and stderr (<=3.7 behavior) + # PIPE is needed for subprocess.run to capture stdout and stderr + # <=3.7 behavior try: - out = subprocess.run(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + out = subprocess.run( + args, stdout=subprocess.PIPE, stderr=subprocess.PIPE + ) except FileNotFoundError as e: - return ShellResult(1, '', "Command {} not found".format(args[0], e)) + return ShellResult( + 1, "", "Command {} not found, {}".format(args[0], e) + ) return ShellResult( return_code=out.returncode, - # if there's nothing on stdout, .stdout is None (<=3.7 behavior), so we need a default value + # if there's nothing on stdout, .stdout is None (<=3.7 behavior) + # so we need a default value stdout=(out.stdout or b"").decode(), stderr=(out.stderr or b"").decode(), ) - # This could throw on non-UTF8 decodable byte strings, but that should be rare - # since utf-8 is backwards compatible with ascii + # This could throw on non-UTF8 decodable byte strings + # but that should be rare since utf-8 is backwards compatible with ascii class FwtsTester: @@ -178,10 +186,11 @@ def is_fwts_supported(self) -> bool: def fwts_log_check_passed( self, output_directory: str, fwts_arguments=["klog", "oops"] ) -> bool: - """Check if fwts logs passes the checks specified in sleep_test_log_check.py. + """ + Check if fwts logs passes the checks specified in sleep_test_log_check This script live in the same directory - :param output_directory: where the output of fwts should be redirected to + :param output_directory: where the output of fwts should be written to :type output_directory: str :return: whether sleep_test_log_check.py returned 0 (success) :rtype: bool @@ -205,7 +214,8 @@ def fwts_log_check_passed( def get_failed_services() -> T.List[str]: - """Counts the number of failed services listed in systemctl + """ + Counts the number of failed services listed in systemctl :return: a list of failed services as they appear in systemctl """ @@ -226,20 +236,20 @@ def get_failed_services() -> T.List[str]: def create_parser(): parser = argparse.ArgumentParser( prog="Reboot tests", - description="This script is used to collect device information and to check for differences between reboots.", + description="Collects device info and compares them across reboots", ) parser.add_argument( "-d", "--dump-to", required=False, dest="output_directory", - help="Absolute path to the output directory. Device info-dumps will be written here.", + help="Device info-dumps will be written here", ) parser.add_argument( "-c", "--compare-to", dest="comparison_directory", - help="Absolute path to the comparison directory. This should contain the ground-truth.", + help="Directory of ground-truth for device info comparison", ) parser.add_argument( "-s", @@ -247,7 +257,7 @@ def create_parser(): default=False, dest="do_service_check", action="store_true", - help="Whether the script should check if all system services are running", + help="If specified, check if all system services are running", ) parser.add_argument( "-f", @@ -255,7 +265,7 @@ def create_parser(): default=False, dest="do_fwts_check", action="store_true", - help="Whether the script should look for fwts log errors", + help="If specified, look for fwts log errors", ) parser.add_argument( "-g", @@ -263,14 +273,15 @@ def create_parser(): default=False, dest="do_renderer_check", action="store_true", - help="Whether the script should check if hardware rendering is being used", + help="If specified, check if hardware rendering is being used", ) return parser def remove_color_code(string: str) -> str: - """Removes ANSI color escape sequences from string + """ + Removes ANSI color escape sequences from string :param string: the string that you would like to remove color code credit: Hanhsuan Lee @@ -281,22 +292,23 @@ def remove_color_code(string: str) -> str: class HardwareRendererTester: def is_desktop_image(self) -> bool: - print("Checking if this system is using a desktop image...", end='') + print("Checking if this system is using a desktop image...", end="") if not shutil.which("dpkg"): # core and server image doesn't have dpkg return False # if we found any of these packages, we are on desktop if run_command(["dpkg", "-l", "ubuntu-desktop"]).return_code == 0: - print('ok') return True - if run_command(["dpkg", "-l", "ubuntu-desktop-minimal"]).return_code == 0: - print('ok') + if ( + run_command(["dpkg", "-l", "ubuntu-desktop-minimal"]).return_code + == 0 + ): return True - print('not desktop') return False def has_display_connection(self) -> bool: - """Checks if a display is connected by searching the /sys/class/drm directory + """ + Checks if a display is connected by searching /sys/class/drm :return: True if there's at least 1 node that is connected """ @@ -312,21 +324,24 @@ def has_display_connection(self) -> bool: # kernel doesn't see any GPU nodes print( "There's nothing under {}".format(DRM_PATH), - "if an external GPU is connected, check if the connection is loose", + "if an external GPU is connected," + "check if the connection is loose", ) return False print("These nodes", possible_gpu_nodes, "exist") for gpu in possible_gpu_nodes: - # for each gpu, check for connection, return true if anything is connected + # for each gpu, check for connection + # return true if anything is connected try: with open("{}/{}/status".format(DRM_PATH, gpu)) as status_file: if status_file.read().strip().lower() == "connected": print("{} is connected to display!".format(gpu)) return True except FileNotFoundError: - # this just means we don't have a status file => no connection, continue to the next + # this just means we don't have a status file + # => no connection, continue to the next pass except Exception as e: print("Unexpected error: ", e, file=sys.stderr) @@ -334,11 +349,15 @@ def has_display_connection(self) -> bool: print( "No display is connected. This case will be skipped.", "Maybe the display cable is not connected?", + "If the device is not supposed to have a display," + "then skipping is expected", ) return False def is_hardware_renderer_available(self) -> bool: - """Checks if hardware rendering is being used. THIS ASSUMES A DRM CONNECTION EXISTS + """ + Checks if hardware rendering is being used. + THIS ASSUMES A DRM CONNECTION EXISTS - self.has_display_connection() should be called first if unsure :return: True if a hardware renderer is active, otherwise return False @@ -375,12 +394,14 @@ def is_hardware_renderer_available(self) -> bool: def parse_unity_support_output( self, unity_output_string: str ) -> T.Dict[str, str]: - """Parses the output of `unity_support_test` into a dictionary + """ + Parses the output of `unity_support_test` into a dictionary - :param output_string: the raw output from running `unity_support_test -p` + :param output_string: the raw output from running unity_support_test -p :type output_string: str - :return: string key-value pairs that mirror the output of unity_support_test. - Left hand side of the first colon are the keys; right hand side are the values. + :return: string key-value pairs that mirror the output of unity_support + Left hand side of the first colon are the keys; + right hand side are the values. :rtype: dict[str, str] """ @@ -416,7 +437,7 @@ def main() -> int: if args.comparison_directory is not None: if args.output_directory is None: print( - "Error: Please also specify an output directory with the -d flag.", + "[ ERR ] Please specify an output directory with the -d flag.", file=sys.stderr, ) else: @@ -450,7 +471,7 @@ def main() -> int: if args.do_renderer_check: tester = HardwareRendererTester() - if tester.has_display_connection() and tester.is_desktop_image(): + if tester.is_desktop_image() and tester.has_display_connection(): # skip renderer test if there's no display renderer_test_passed = tester.is_hardware_renderer_available() From c64cbb2b48bfb57d0b5c4af7c4db326fc98df64c Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Mon, 15 Jul 2024 16:19:44 +0800 Subject: [PATCH 21/36] test: update unit tests to reflect new method names --- .../base/tests/test_reboot_check_test.py | 28 ++++--------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index 4147a10dc3..68b8dd4df7 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -148,24 +148,6 @@ def test_is_hardware_renderer_available( tester = RCT.HardwareRendererTester() self.assertTrue(tester.is_hardware_renderer_available()) - @patch("reboot_check_test.run_command") - def test_get_display_id(self, mock_run: MagicMock): - with patch.dict(os.environ, {"DISPLAY": ":0"}): - self.assertEqual(self.tester.get_display_id(), ":0") - - with patch.dict(os.environ, {"DISPLAY": ""}): - mock_run.side_effect = self.create_side_effect("wayland") - self.assertEqual(self.tester.get_display_id(), ":0") - - mock_run.side_effect = self.create_side_effect("wayland", False) - self.assertEqual(self.tester.get_display_id(), ":0") - - mock_run.side_effect = self.create_side_effect("x11") - self.assertEqual(self.tester.get_display_id(), ":0") - - mock_run.side_effect = self.create_side_effect("x11", False) - self.assertIsNone(self.tester.get_display_id()) - class InfoDumpTests(unittest.TestCase): @classmethod @@ -274,7 +256,9 @@ def test_get_failed_services_with_failed_services( "snap.checkbox.agent.service loaded failed failed Service for snap applictaion checkbox.agent", "", ) - self.assertEqual(RCT.get_failed_services(), [mock_run.return_value.stdout]) + self.assertEqual( + RCT.get_failed_services(), [mock_run.return_value.stdout] + ) class MainFunctionTests(unittest.TestCase): @@ -285,7 +269,7 @@ def setUpClass(cls): def tearDown(self): shutil.rmtree(self.temp_output_dir, ignore_errors=True) - shutil.rmtree(self.temp_comparison_dir, ignore_errors=True) + shutil.rmtree(self.temp_comparison_dir, ignore_errors=True) @patch("reboot_check_test.run_command") def test_partial_main(self, mock_run: MagicMock): @@ -345,7 +329,7 @@ def test_main_function_full( "reboot_check_test.FwtsTester.is_fwts_supported" ) as mock_is_fwts_supported: mock_is_fwts_supported.return_value = True - mock_compare.return_value = True + mock_compare.return_value = True RCT.main() @@ -368,4 +352,4 @@ def test_main_function_full( # <= is an overloaded operator for sets that checks the isSubset relation self.assertLessEqual( expected_commands, actual, "should be a subset" - ) \ No newline at end of file + ) From 0057b073db95d809b0d2ffbba64ad5bbdfab5a5b Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Tue, 16 Jul 2024 13:11:08 +0800 Subject: [PATCH 22/36] fix: assume display variable --- providers/base/bin/reboot_check_test.py | 26 +++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 80f617b22f..a5b70fe2e8 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -89,6 +89,7 @@ def compare_device_lists( :param expected_dir: files containing the expected device list :param actual_dir: files containing the actual device list + :param devices: what devices do we want to compare, see DEFAULT_DEVICES :return: whether the device list matches """ print( @@ -164,9 +165,7 @@ def run_command(args: T.List[str]) -> ShellResult: args, stdout=subprocess.PIPE, stderr=subprocess.PIPE ) except FileNotFoundError as e: - return ShellResult( - 1, "", "Command {} not found, {}".format(args[0], e) - ) + return ShellResult(1, "", "Command {} not found {}".format(args[0], e)) return ShellResult( return_code=out.returncode, @@ -292,7 +291,7 @@ def remove_color_code(string: str) -> str: class HardwareRendererTester: def is_desktop_image(self) -> bool: - print("Checking if this system is using a desktop image...", end="") + print("Checking if this system is using a desktop image...") if not shutil.which("dpkg"): # core and server image doesn't have dpkg return False @@ -303,7 +302,10 @@ def is_desktop_image(self) -> bool: run_command(["dpkg", "-l", "ubuntu-desktop-minimal"]).return_code == 0 ): + print("Ubuntu desktop detected!") return True + + print("Not on ubuntu desktop.") return False def has_display_connection(self) -> bool: @@ -374,9 +376,19 @@ def is_hardware_renderer_available(self) -> bool: [ "{}/usr/lib/nux/unity_support_test".format(RUNTIME_ROOT), "-p", + "-display", + os.getenv("DISPLAY", ":0"), ] ) if unity_support_output.return_code != 0: + print( + "[ ERR ] unity support test returned {}".format( + unity_support_output.return_code + ), + file=sys.stderr, + ) + print("stdout", unity_support_output.stdout, file=sys.stderr) + print("stderr", unity_support_output.stderr, file=sys.stderr) return False is_hardware_rendered = ( @@ -389,6 +401,7 @@ def is_hardware_renderer_available(self) -> bool: print("[ OK ] This machine is using a hardware renderer!") return True + print("[ ERR ] Software rendering detected", file=sys.stderr) return False def parse_unity_support_output( @@ -443,9 +456,10 @@ def main() -> int: else: collector = DeviceInfoCollector() collector.dump(args.output_directory) - collector.compare_device_lists( + if collector.compare_device_lists( args.comparison_directory, args.output_directory - ) + ): + print("[ OK ] Devices match!") # dump (no checks) if only output_directory is specified if args.output_directory is not None and args.comparison_directory is None: From 9fdcb1beb1e15a3800c3e6f2e6f1b1ba7c5faf86 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Tue, 16 Jul 2024 16:08:07 +0800 Subject: [PATCH 23/36] test: update tests to reflect removal of get_display_id --- .../base/tests/test_reboot_check_test.py | 49 +++++++------------ 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index 68b8dd4df7..dcc28b2d83 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -83,30 +83,6 @@ class DisplayConnectionTests(unittest.TestCase): def setUp(self) -> None: self.tester = RCT.HardwareRendererTester() - def create_side_effect( - self, - display_server_name: 'T.Literal["wayland", "x11", "tty"]', - happy_path=True, - ): - def side_effect(args: T.List[str]): - stdout = "" - if args[0] == "loginctl": - stdout = "Type={}".format(display_server_name) - if args[0] == "pgrep": - if happy_path: - stdout = "75632 /usr/bin/Xwayland :0 -rootless -noreset -accessx -core -auth /run/user/1000/.mutter-Xwaylandauth.FFE5P2" - else: - stdout = "" # Xwayland is not guaranteed to exist - if args[0] == "w": - if happy_path: - stdout = "ubuntu :0 :0 13:43 13:55m 0.01s 0.00s /usr/libexec/gdm-wayland-session en" - else: - stdout = "ubuntu tty2 tty2 Mon13 35:51m 0.01s 0.00s /usr/libexec/gdm-wayland-session en" - - return RCT.ShellResult(0, stdout, "") - - return side_effect - def test_display_check_happy_path(self): with patch( "os.listdir", return_value=["fakeCard0", "fakeCard1"] @@ -129,7 +105,6 @@ def test_display_check_no_display_path(self): ): self.assertFalse(self.tester.has_display_connection()) - @patch("reboot_check_test.HardwareRendererTester.get_display_id") @patch( "reboot_check_test.HardwareRendererTester.parse_unity_support_output" ) @@ -138,16 +113,23 @@ def test_is_hardware_renderer_available( self, mock_run: MagicMock, mock_parse: MagicMock, - mock_get_display_id: MagicMock, ): mock_run.side_effect = do_nothing - mock_get_display_id.return_value = ":0" mock_parse.return_value = { "Not software rendered": "yes", } tester = RCT.HardwareRendererTester() self.assertTrue(tester.is_hardware_renderer_available()) + @patch("reboot_check_test.run_command") + def test_is_ubuntu_desktop(self, mock_run: MagicMock): + mock_run.return_value = RCT.ShellResult(1, "No such package", "") + tester = RCT.HardwareRendererTester() + self.assertFalse(tester.is_desktop_image()) + + mock_run.return_value = RCT.ShellResult(0, "", "") + self.assertTrue(tester.is_desktop_image()) + class InfoDumpTests(unittest.TestCase): @classmethod @@ -262,6 +244,13 @@ def test_get_failed_services_with_failed_services( class MainFunctionTests(unittest.TestCase): + + def test_run_cmd_exception(self): + cmd = sh_split("non_existent_command -a -b -c") + output = RCT.run_command(cmd) + self.assertEqual(output.return_code, 1) + self.assertIn("Command non_existent_command not found", output.stderr) + @classmethod def setUpClass(cls): cls.temp_output_dir = "{}/temp_output_dir".format(os.getcwd()) @@ -308,13 +297,9 @@ def test_partial_main(self, mock_run: MagicMock): ) # only lspci, lsusb, iw calls self.assertEqual(mock_compare.call_count, 1) - @patch("reboot_check_test.HardwareRendererTester.get_display_id") @patch("reboot_check_test.run_command") - def test_main_function_full( - self, mock_run: MagicMock, mock_get_display_id: MagicMock - ): + def test_main_function_full(self, mock_run: MagicMock): mock_run.side_effect = do_nothing - mock_get_display_id.return_value = ":0" # Full suite with patch( "sys.argv", From fe8930bbc514b94e767e71c5a7006a96e46e7cc6 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Tue, 16 Jul 2024 16:08:24 +0800 Subject: [PATCH 24/36] fix: missing conditionals on certain logs --- providers/base/bin/reboot_check_test.py | 219 ++++++++++++------------ 1 file changed, 109 insertions(+), 110 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index a5b70fe2e8..d164025091 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -29,6 +29,34 @@ def __init__(self, return_code: int, stdout: str, stderr: str): self.stderr = stderr +def run_command(args: T.List[str]) -> ShellResult: + """Wrapper around subprocess.run + + :param args: same args that goes to subprocess.run + :type args: T.List[str] + :return: return code, stdout and stderr, all non-null + :rtype: ShellResult + """ + # PIPE is needed for subprocess.run to capture stdout and stderr + # <=3.7 behavior + try: + out = subprocess.run( + args, stdout=subprocess.PIPE, stderr=subprocess.PIPE + ) + except FileNotFoundError as e: + return ShellResult(1, "", "Command {} not found {}".format(args[0], e)) + + return ShellResult( + return_code=out.returncode, + # if there's nothing on stdout, .stdout is None (<=3.7 behavior) + # so we need a default value + stdout=(out.stdout or b"").decode(), + stderr=(out.stderr or b"").decode(), + ) + # This could throw on non-UTF8 decodable byte strings + # but that should be rare since utf-8 is backwards compatible with ascii + + class DeviceInfoCollector: class Device(enum.Enum): @@ -93,8 +121,7 @@ def compare_device_lists( :return: whether the device list matches """ print( - "Comparing device lists in (expected){} against (actual){}..." - .format( + "Comparing device lists in (expected){} against (actual){}...".format( expected_dir, actual_dir ) ) @@ -150,34 +177,6 @@ def __init__(self) -> None: } -def run_command(args: T.List[str]) -> ShellResult: - """Wrapper around subprocess.run - - :param args: same args that goes to subprocess.run - :type args: T.List[str] - :return: return code, stdout and stderr, all non-null - :rtype: ShellResult - """ - # PIPE is needed for subprocess.run to capture stdout and stderr - # <=3.7 behavior - try: - out = subprocess.run( - args, stdout=subprocess.PIPE, stderr=subprocess.PIPE - ) - except FileNotFoundError as e: - return ShellResult(1, "", "Command {} not found {}".format(args[0], e)) - - return ShellResult( - return_code=out.returncode, - # if there's nothing on stdout, .stdout is None (<=3.7 behavior) - # so we need a default value - stdout=(out.stdout or b"").decode(), - stderr=(out.stderr or b"").decode(), - ) - # This could throw on non-UTF8 decodable byte strings - # but that should be rare since utf-8 is backwards compatible with ascii - - class FwtsTester: def is_fwts_supported(self) -> bool: return shutil.which("fwts") is not None @@ -212,82 +211,6 @@ def fwts_log_check_passed( return result.return_code == 0 -def get_failed_services() -> T.List[str]: - """ - Counts the number of failed services listed in systemctl - - :return: a list of failed services as they appear in systemctl - """ - command = [ - "systemctl", - "list-units", - "--system", - "--no-ask-password", - "--no-pager", - "--no-legend", - "--state=failed", - "--plain", # plaintext, otherwise it includes color codes - ] - - return run_command(command).stdout.splitlines() - - -def create_parser(): - parser = argparse.ArgumentParser( - prog="Reboot tests", - description="Collects device info and compares them across reboots", - ) - parser.add_argument( - "-d", - "--dump-to", - required=False, - dest="output_directory", - help="Device info-dumps will be written here", - ) - parser.add_argument( - "-c", - "--compare-to", - dest="comparison_directory", - help="Directory of ground-truth for device info comparison", - ) - parser.add_argument( - "-s", - "--service-check", - default=False, - dest="do_service_check", - action="store_true", - help="If specified, check if all system services are running", - ) - parser.add_argument( - "-f", - "--fwts-check", - default=False, - dest="do_fwts_check", - action="store_true", - help="If specified, look for fwts log errors", - ) - parser.add_argument( - "-g", - "--graphics", - default=False, - dest="do_renderer_check", - action="store_true", - help="If specified, check if hardware rendering is being used", - ) - - return parser - - -def remove_color_code(string: str) -> str: - """ - Removes ANSI color escape sequences from string - - :param string: the string that you would like to remove color code - credit: Hanhsuan Lee - """ - return re.sub(r"\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])", "", string) - - class HardwareRendererTester: def is_desktop_image(self) -> bool: @@ -387,8 +310,6 @@ def is_hardware_renderer_available(self) -> bool: ), file=sys.stderr, ) - print("stdout", unity_support_output.stdout, file=sys.stderr) - print("stderr", unity_support_output.stderr, file=sys.stderr) return False is_hardware_rendered = ( @@ -430,6 +351,82 @@ def parse_unity_support_output( return output +def get_failed_services() -> T.List[str]: + """ + Counts the number of failed services listed in systemctl + + :return: a list of failed services as they appear in systemctl + """ + command = [ + "systemctl", + "list-units", + "--system", + "--no-ask-password", + "--no-pager", + "--no-legend", + "--state=failed", + "--plain", # plaintext, otherwise it includes color codes + ] + + return run_command(command).stdout.splitlines() + + +def create_parser(): + parser = argparse.ArgumentParser( + prog="Reboot tests", + description="Collects device info and compares them across reboots", + ) + parser.add_argument( + "-d", + "--dump-to", + required=False, + dest="output_directory", + help="Device info-dumps will be written here", + ) + parser.add_argument( + "-c", + "--compare-to", + dest="comparison_directory", + help="Directory of ground-truth for device info comparison", + ) + parser.add_argument( + "-s", + "--service-check", + default=False, + dest="do_service_check", + action="store_true", + help="If specified, check if all system services are running", + ) + parser.add_argument( + "-f", + "--fwts-check", + default=False, + dest="do_fwts_check", + action="store_true", + help="If specified, look for fwts log errors", + ) + parser.add_argument( + "-g", + "--graphics", + default=False, + dest="do_renderer_check", + action="store_true", + help="If specified, check if hardware rendering is being used", + ) + + return parser + + +def remove_color_code(string: str) -> str: + """ + Removes ANSI color escape sequences from string + + :param string: the string that you would like to remove color code + credit: Hanhsuan Lee + """ + return re.sub(r"\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])", "", string) + + def main() -> int: """Main routine @@ -471,6 +468,8 @@ def main() -> int: args.output_directory ): fwts_passed = False + else: + print("[ OK ] fwts checks passed!") if args.do_service_check: failed_services = get_failed_services() @@ -480,8 +479,8 @@ def main() -> int: file=sys.stderr, ) service_check_passed = False - - print("[ OK ] All system services are running!") + else: + print("[ OK ] Didn't find any failed system services!") if args.do_renderer_check: tester = HardwareRendererTester() From e15ae0676e7397bd7c45becff15779db857730ab Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Wed, 17 Jul 2024 10:09:30 +0800 Subject: [PATCH 25/36] fix: let unity support infer environment variable --- providers/base/bin/reboot_check_test.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index d164025091..744bbacdb3 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -289,18 +289,18 @@ def is_hardware_renderer_available(self) -> bool: :rtype: bool """ - print( - "Checking display id: {}".format( - os.getenv("DISPLAY", ":0 (assume)") - ) - ) + DISPLAY = os.getenv("DISPLAY", "") + + if DISPLAY == '': + print("$DISPLAY is not set, we will let unity_support infer this") + else: + print("Checking $DISPLAY={}".format(DISPLAY)) + unity_support_output = run_command( [ "{}/usr/lib/nux/unity_support_test".format(RUNTIME_ROOT), - "-p", - "-display", - os.getenv("DISPLAY", ":0"), + "-p" ] ) if unity_support_output.return_code != 0: From 27fd47b34393cff5398a746292a9bc58baec9fc4 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Wed, 17 Jul 2024 10:20:21 +0800 Subject: [PATCH 26/36] refactor: revert changes in boot.pxu to put in a separate PR --- providers/base/units/stress/boot.pxu | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/providers/base/units/stress/boot.pxu b/providers/base/units/stress/boot.pxu index 96d80bae6a..28e1fb067b 100644 --- a/providers/base/units/stress/boot.pxu +++ b/providers/base/units/stress/boot.pxu @@ -46,7 +46,7 @@ _purpose: This creates baseline data sets which will be considered the master unit: job plugin: shell command: - reboot_check_test.py -d "$PLAINBOX_SESSION_SHARE/before_reboot" + reboot_check_test.sh -d "$PLAINBOX_SESSION_SHARE/before_reboot" environ: LD_LIBRARY_PATH user: root estimated_duration: 1s @@ -94,7 +94,7 @@ unit: job plugin: shell environ: LD_LIBRARY_PATH command: - reboot_check_test.py -g -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/cold_reboot_cycle1" -s -f + reboot_check_test.sh -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/cold_reboot_cycle1" -s -f user: root flags: preserve-locale estimated_duration: 1.0 @@ -111,7 +111,7 @@ template-unit: job plugin: shell environ: LD_LIBRARY_PATH command: - reboot_check_test.py -g -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/cold_reboot_cycle{reboot_id}" -s -f + reboot_check_test.sh -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/cold_reboot_cycle{reboot_id}" -s -f user: root flags: preserve-locale estimated_duration: 1.0 @@ -158,7 +158,7 @@ unit: job plugin: shell environ: LD_LIBRARY_PATH command: - reboot_check_test.py -g -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/warm_reboot_cycle1" -s -f -g + reboot_check_test.sh -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/warm_reboot_cycle1" -s -f user: root flags: preserve-locale estimated_duration: 1.0 @@ -175,8 +175,8 @@ template-unit: job plugin: shell environ: LD_LIBRARY_PATH command: - reboot_check_test.py -g -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/warm_reboot_cycle{reboot_id}" -s -f + reboot_check_test.sh -c "$PLAINBOX_SESSION_SHARE/before_reboot" -d "$PLAINBOX_SESSION_SHARE/warm_reboot_cycle{reboot_id}" -s -f user: root flags: preserve-locale estimated_duration: 1.0 -depends: warm-boot-loop-reboot{reboot_id} +depends: warm-boot-loop-reboot{reboot_id} \ No newline at end of file From 0a3d904ccd92dc0455b2eb75c3d97e35624a0749 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Wed, 17 Jul 2024 10:29:10 +0800 Subject: [PATCH 27/36] fix: flake8 linter errors --- providers/base/bin/reboot_check_test.py | 4 ++-- providers/base/tests/test_reboot_check_test.py | 15 +++++++++------ providers/base/units/stress/boot.pxu | 2 +- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 744bbacdb3..60e8037ab2 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -121,7 +121,8 @@ def compare_device_lists( :return: whether the device list matches """ print( - "Comparing device lists in (expected){} against (actual){}...".format( + "Comparing device lists in (expected){} against (actual){}..." + .format( expected_dir, actual_dir ) ) @@ -296,7 +297,6 @@ def is_hardware_renderer_available(self) -> bool: else: print("Checking $DISPLAY={}".format(DISPLAY)) - unity_support_output = run_command( [ "{}/usr/lib/nux/unity_support_test".format(RUNTIME_ROOT), diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index dcc28b2d83..33a6571140 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -19,7 +19,7 @@ def test_parse_ok_unity_support_string(self): OK_UNITY_STRING = """\ OpenGL vendor string: Intel OpenGL renderer string: Mesa Intel(R) UHD Graphics (ICL GT1) - OpenGL version string: 4.6 (Compatibility Profile) Mesa 23.2.1-1ubuntu3.1~22.04.2 + OpenGL version string: 4.6 (Compatibility Profile) Mesa 23.2.1 Not software rendered: \x1B[033myes\x1B[0m Not blacklisted: \x1B[033myes\x1B[0m @@ -38,7 +38,7 @@ def test_parse_ok_unity_support_string(self): expected = { "OpenGL vendor string": "Intel", "OpenGL renderer string": "Mesa Intel(R) UHD Graphics (ICL GT1)", - "OpenGL version string": "4.6 (Compatibility Profile) Mesa 23.2.1-1ubuntu3.1~22.04.2", + "OpenGL version string": "4.6 (Compatibility Profile) Mesa 23.2.1", "Not software rendered": "yes", "Not blacklisted": "yes", "GLX fbconfig": "yes", @@ -59,7 +59,7 @@ def test_parse_bad_unity_support_string(self): BAD_UNITY_STRING = """ OpenGL vendor string Intel OpenGL renderer string: Mesa Intel(R) UHD Graphics (ICL GT1) - OpenGL version string 4.6 (Compatibility Profile) Mesa 23.2.1-1ubuntu3.1~22.04.2 + OpenGL version string 4.6 (Compatibility Profile) Mesa 23.2.1-1ubuntu GL version is 1.4+% \x1B[033myes\x1B[0m """ actual = self.tester.parse_unity_support_output(BAD_UNITY_STRING) @@ -235,7 +235,8 @@ def test_get_failed_services_with_failed_services( ): mock_run.return_value = RCT.ShellResult( 0, - "snap.checkbox.agent.service loaded failed failed Service for snap applictaion checkbox.agent", + "snap.checkbox.agent.service loaded failed failed Service\ + for snap applictaion checkbox.agent", "", ) self.assertEqual( @@ -262,7 +263,8 @@ def tearDown(self): @patch("reboot_check_test.run_command") def test_partial_main(self, mock_run: MagicMock): - # this test only validates the main function logic(if it picks out the correct tests to run) + # this test only validates the main function logic + # (if it picks out the correct tests to run) mock_run.side_effect = do_nothing with patch( @@ -334,7 +336,8 @@ def test_main_function_full(self, mock_run: MagicMock): # then take the 1st element, which is the command name actual.add(call[0][0][0]) - # <= is an overloaded operator for sets that checks the isSubset relation + # <= is an overloaded operator for sets + # that checks the isSubset relation self.assertLessEqual( expected_commands, actual, "should be a subset" ) diff --git a/providers/base/units/stress/boot.pxu b/providers/base/units/stress/boot.pxu index 28e1fb067b..71dbb4dbb5 100644 --- a/providers/base/units/stress/boot.pxu +++ b/providers/base/units/stress/boot.pxu @@ -179,4 +179,4 @@ command: user: root flags: preserve-locale estimated_duration: 1.0 -depends: warm-boot-loop-reboot{reboot_id} \ No newline at end of file +depends: warm-boot-loop-reboot{reboot_id} From b4b49306566f42a352f33dee415e978bc56ed4d4 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Wed, 17 Jul 2024 16:01:30 +0800 Subject: [PATCH 28/36] fix: increase test coverage --- providers/base/bin/reboot_check_test.py | 13 ++++++------- .../base/tests/test_reboot_check_test.py | 19 ++++++++++++++++++- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 60e8037ab2..be3b688b76 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -121,8 +121,7 @@ def compare_device_lists( :return: whether the device list matches """ print( - "Comparing device lists in (expected){} against (actual){}..." - .format( + "Comparing devices in (expected) {} against (actual) {}...".format( expected_dir, actual_dir ) ) @@ -292,16 +291,13 @@ def is_hardware_renderer_available(self) -> bool: DISPLAY = os.getenv("DISPLAY", "") - if DISPLAY == '': + if DISPLAY == "": print("$DISPLAY is not set, we will let unity_support infer this") else: print("Checking $DISPLAY={}".format(DISPLAY)) unity_support_output = run_command( - [ - "{}/usr/lib/nux/unity_support_test".format(RUNTIME_ROOT), - "-p" - ] + ["{}/usr/lib/nux/unity_support_test".format(RUNTIME_ROOT), "-p"] ) if unity_support_output.return_code != 0: print( @@ -450,6 +446,9 @@ def main() -> int: "[ ERR ] Please specify an output directory with the -d flag.", file=sys.stderr, ) + raise ValueError( + "Cmoparison directory is specified, but output directory isn't" + ) else: collector = DeviceInfoCollector() collector.dump(args.output_directory) diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index 33a6571140..4f5afb2b1e 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -320,7 +320,6 @@ def test_main_function_full(self, mock_run: MagicMock): RCT.main() - # self.assertTrue(mock_compare.called) self.assertTrue(mock_is_fwts_supported.called) expected_commands = { @@ -341,3 +340,21 @@ def test_main_function_full(self, mock_run: MagicMock): self.assertLessEqual( expected_commands, actual, "should be a subset" ) + + with patch( + "reboot_check_test.get_failed_services" + ) as mock_get_failed_services: + mock_get_failed_services.return_value = [ + "failed service1", + "failed service2", + ] + self.assertEqual(RCT.main(), 1) + + def test_only_comparison_is_specified(self): + with patch( + "sys.argv", + sh_split( + 'reboot_check_test.py -c "{}"'.format(self.temp_output_dir) + ), + ), self.assertRaises(ValueError): + RCT.main() From 1d18e30ce051824a4223f0a34316f0e1d41aa0c0 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Wed, 17 Jul 2024 16:23:00 +0800 Subject: [PATCH 29/36] fix: combine dpkg check conditions --- providers/base/bin/reboot_check_test.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index be3b688b76..553ac0f309 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -219,10 +219,11 @@ def is_desktop_image(self) -> bool: # core and server image doesn't have dpkg return False # if we found any of these packages, we are on desktop - if run_command(["dpkg", "-l", "ubuntu-desktop"]).return_code == 0: - return True if ( - run_command(["dpkg", "-l", "ubuntu-desktop-minimal"]).return_code + run_command(["dpkg", "-l", "ubuntu-desktop"]).return_code == 0 + or run_command( + ["dpkg", "-l", "ubuntu-desktop-minimal"] + ).return_code == 0 ): print("Ubuntu desktop detected!") From be122440115c0530e76f29585a9086395194c873 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Tue, 23 Jul 2024 13:53:23 +0800 Subject: [PATCH 30/36] update: use the new desktop env detector --- providers/base/bin/reboot_check_test.py | 22 ++----------------- .../base/tests/test_reboot_check_test.py | 9 -------- 2 files changed, 2 insertions(+), 29 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 553ac0f309..26defaeaa6 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -9,6 +9,7 @@ import sys import enum import typing as T +from checkbox_support.scripts.image_checker import has_desktop_environment # Checkbox could run in a snap container, so we need to prepend this root path @@ -213,25 +214,6 @@ def fwts_log_check_passed( class HardwareRendererTester: - def is_desktop_image(self) -> bool: - print("Checking if this system is using a desktop image...") - if not shutil.which("dpkg"): - # core and server image doesn't have dpkg - return False - # if we found any of these packages, we are on desktop - if ( - run_command(["dpkg", "-l", "ubuntu-desktop"]).return_code == 0 - or run_command( - ["dpkg", "-l", "ubuntu-desktop-minimal"] - ).return_code - == 0 - ): - print("Ubuntu desktop detected!") - return True - - print("Not on ubuntu desktop.") - return False - def has_display_connection(self) -> bool: """ Checks if a display is connected by searching /sys/class/drm @@ -484,7 +466,7 @@ def main() -> int: if args.do_renderer_check: tester = HardwareRendererTester() - if tester.is_desktop_image() and tester.has_display_connection(): + if has_desktop_environment() and tester.has_display_connection(): # skip renderer test if there's no display renderer_test_passed = tester.is_hardware_renderer_available() diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index 4f5afb2b1e..7793c44166 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -121,15 +121,6 @@ def test_is_hardware_renderer_available( tester = RCT.HardwareRendererTester() self.assertTrue(tester.is_hardware_renderer_available()) - @patch("reboot_check_test.run_command") - def test_is_ubuntu_desktop(self, mock_run: MagicMock): - mock_run.return_value = RCT.ShellResult(1, "No such package", "") - tester = RCT.HardwareRendererTester() - self.assertFalse(tester.is_desktop_image()) - - mock_run.return_value = RCT.ShellResult(0, "", "") - self.assertTrue(tester.is_desktop_image()) - class InfoDumpTests(unittest.TestCase): @classmethod From 643254972ca1c7d16a4b78c985be00c6bc97e6e7 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Tue, 23 Jul 2024 14:37:01 +0800 Subject: [PATCH 31/36] test: increase coverage --- .../base/tests/test_reboot_check_test.py | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index 7793c44166..fa701a27cc 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -120,7 +120,29 @@ def test_is_hardware_renderer_available( } tester = RCT.HardwareRendererTester() self.assertTrue(tester.is_hardware_renderer_available()) + + @patch( + "reboot_check_test.HardwareRendererTester.parse_unity_support_output" + ) + @patch("reboot_check_test.run_command") + def test_is_hardware_renderer_available_fail( + self, + mock_run: MagicMock, + mock_parse: MagicMock, + ): + + mock_run.side_effect = lambda _: RCT.ShellResult(1, '', '') + tester = RCT.HardwareRendererTester() + self.assertFalse(tester.is_hardware_renderer_available()) + mock_run.reset_mock() + mock_run.side_effect = do_nothing + mock_parse.return_value = { + "Not software rendered": "no", + } + tester = RCT.HardwareRendererTester() + self.assertFalse(tester.is_hardware_renderer_available()) + class InfoDumpTests(unittest.TestCase): @classmethod From f75942b4f4cc9ee8d61ab871287547dcd50c814c Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Tue, 23 Jul 2024 14:41:08 +0800 Subject: [PATCH 32/36] style: formatting --- providers/base/tests/test_reboot_check_test.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index fa701a27cc..7e229b12ee 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -120,7 +120,7 @@ def test_is_hardware_renderer_available( } tester = RCT.HardwareRendererTester() self.assertTrue(tester.is_hardware_renderer_available()) - + @patch( "reboot_check_test.HardwareRendererTester.parse_unity_support_output" ) @@ -130,8 +130,8 @@ def test_is_hardware_renderer_available_fail( mock_run: MagicMock, mock_parse: MagicMock, ): - - mock_run.side_effect = lambda _: RCT.ShellResult(1, '', '') + + mock_run.side_effect = lambda _: RCT.ShellResult(1, "", "") tester = RCT.HardwareRendererTester() self.assertFalse(tester.is_hardware_renderer_available()) @@ -142,7 +142,7 @@ def test_is_hardware_renderer_available_fail( } tester = RCT.HardwareRendererTester() self.assertFalse(tester.is_hardware_renderer_available()) - + class InfoDumpTests(unittest.TestCase): @classmethod From 4bc4ef9fa55a61ec938701cf9ace2304f71a5159 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Thu, 26 Sep 2024 15:14:58 +0800 Subject: [PATCH 33/36] fix: remove subprocess.run wrapper --- providers/base/bin/reboot_check_test.py | 81 +++++++------------------ 1 file changed, 23 insertions(+), 58 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 26defaeaa6..33834e169b 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -2,7 +2,7 @@ import argparse import os -import subprocess +import subprocess as sp import re import shutil import filecmp @@ -19,45 +19,6 @@ SNAP = os.getenv("SNAP", default="") -class ShellResult: - """ - Wrapper class around the return value of run_command, guarantees non-null - """ - - def __init__(self, return_code: int, stdout: str, stderr: str): - self.return_code = return_code - self.stdout = stdout - self.stderr = stderr - - -def run_command(args: T.List[str]) -> ShellResult: - """Wrapper around subprocess.run - - :param args: same args that goes to subprocess.run - :type args: T.List[str] - :return: return code, stdout and stderr, all non-null - :rtype: ShellResult - """ - # PIPE is needed for subprocess.run to capture stdout and stderr - # <=3.7 behavior - try: - out = subprocess.run( - args, stdout=subprocess.PIPE, stderr=subprocess.PIPE - ) - except FileNotFoundError as e: - return ShellResult(1, "", "Command {} not found {}".format(args[0], e)) - - return ShellResult( - return_code=out.returncode, - # if there's nothing on stdout, .stdout is None (<=3.7 behavior) - # so we need a default value - stdout=(out.stdout or b"").decode(), - stderr=(out.stderr or b"").decode(), - ) - # This could throw on non-UTF8 decodable byte strings - # but that should be rare since utf-8 is backwards compatible with ascii - - class DeviceInfoCollector: class Device(enum.Enum): @@ -81,8 +42,10 @@ def get_drm_info(self) -> str: return str(os.listdir("/sys/class/drm")) def get_wireless_info(self) -> str: - iw_out = run_command(["iw", "dev"]) - lines = iw_out.stdout.splitlines() + iw_out = sp.run( + ["iw", "dev"], stdout=sp.PIPE, stderr=sp.PIPE, check=True + ) + lines = iw_out.stdout.decode().splitlines() lines_to_write = list( filter( lambda line: "addr" in line @@ -94,19 +57,21 @@ def get_wireless_info(self) -> str: return "\n".join(map(lambda line: line.strip(), lines_to_write)) def get_usb_info(self) -> str: - return run_command( + return sp.run( [ "checkbox-support-lsusb", "-f", '"{}"/var/lib/usbutils/usb.ids'.format(RUNTIME_ROOT), "-s", - ] - ).stdout + ], + check=True, + ).stdout.decode() def get_pci_info(self) -> str: - return run_command( + return sp.run( ["lspci", "-i", "{}/usr/share/misc/pci.ids".format(SNAP)], - ).stdout + check=True, + ).stdout.decode() def compare_device_lists( self, @@ -197,8 +162,8 @@ def fwts_log_check_passed( log_file_path = "{}/fwts_{}.log".format( output_directory, "_".join(fwts_arguments) ) - run_command(["fwts", "-r", log_file_path, *fwts_arguments]) - result = run_command( + sp.run(["fwts", "-r", log_file_path, *fwts_arguments]) + result = sp.run( [ "sleep_test_log_check.py", "-v", @@ -206,10 +171,10 @@ def fwts_log_check_passed( "-t", "all", log_file_path, - ] + ], ) - return result.return_code == 0 + return result.returncode == 0 class HardwareRendererTester: @@ -279,22 +244,22 @@ def is_hardware_renderer_available(self) -> bool: else: print("Checking $DISPLAY={}".format(DISPLAY)) - unity_support_output = run_command( + unity_support_output = sp.run( ["{}/usr/lib/nux/unity_support_test".format(RUNTIME_ROOT), "-p"] ) - if unity_support_output.return_code != 0: + if unity_support_output.returncode != 0: print( "[ ERR ] unity support test returned {}".format( - unity_support_output.return_code + unity_support_output.returncode ), file=sys.stderr, ) return False is_hardware_rendered = ( - self.parse_unity_support_output(unity_support_output.stdout).get( - "Not software rendered" - ) + self.parse_unity_support_output( + unity_support_output.stdout.decode() + ).get("Not software rendered") == "yes" ) if is_hardware_rendered: @@ -347,7 +312,7 @@ def get_failed_services() -> T.List[str]: "--plain", # plaintext, otherwise it includes color codes ] - return run_command(command).stdout.splitlines() + return sp.run(command).stdout.decode().splitlines() def create_parser(): From 14f3c1e9abb3d016b323b4a8b903c8b4b1ef33a4 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Thu, 26 Sep 2024 15:37:03 +0800 Subject: [PATCH 34/36] fix: use class attributes instead of enum to avoid writing .value --- providers/base/bin/reboot_check_test.py | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index 33834e169b..e33f5c2e69 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -7,7 +7,6 @@ import shutil import filecmp import sys -import enum import typing as T from checkbox_support.scripts.image_checker import has_desktop_environment @@ -21,7 +20,7 @@ class DeviceInfoCollector: - class Device(enum.Enum): + class Device: PCI = "pci" WIRELESS = "wireless" USB = "usb" @@ -34,7 +33,7 @@ class Device(enum.Enum): Device.USB, ], # these can fail the test case "optional": [Device.DRM], # these only produce warnings - } # type: dict[T.Literal['required','optional'], list[Device]] + } # type: dict[str, list[str]] # to modify, add more values in the enum # and reference them in required/optional respectively @@ -77,7 +76,7 @@ def compare_device_lists( self, expected_dir: str, actual_dir: str, - devices=DEFAULT_DEVICES, + devices: T.Dict[str, T.List[str]] = DEFAULT_DEVICES ) -> bool: """Compares the list of devices in expected_dir against actual_dir @@ -93,18 +92,18 @@ def compare_device_lists( ) for device in devices["required"]: # file paths of the expected and actual device lists - expected = "{}/{}_log".format(expected_dir, device.value) - actual = "{}/{}_log".format(actual_dir, device.value) + expected = "{}/{}_log".format(expected_dir, device) + actual = "{}/{}_log".format(actual_dir, device) if not filecmp.cmp(expected, actual): print( - "[ ERR ] The output of {} differs!".format(device.value), + "[ ERR ] The output of {} differs!".format(device), file=sys.stderr, ) return False for device in devices["optional"]: - expected = "{}/{}_log".format(expected_dir, device.value) - actual = "{}/{}_log".format(actual_dir, device.value) + expected = "{}/{}_log".format(expected_dir, device) + actual = "{}/{}_log".format(actual_dir, device) if not filecmp.cmp(expected, actual): print( "[ WARN ] Items under {} have changed.".format(actual), @@ -116,19 +115,19 @@ def compare_device_lists( def dump( self, output_directory: str, - devices=DEFAULT_DEVICES, + devices: T.Dict[str, T.List[str]] = DEFAULT_DEVICES, ) -> None: os.makedirs(output_directory, exist_ok=True) # add extra behavior if necessary for device in devices["required"]: with open( - "{}/{}_log".format(output_directory, device.value), "w" + "{}/{}_log".format(output_directory, device), "w" ) as file: file.write(self.dump_function[device]()) for device in devices["optional"]: with open( - "{}/{}_log".format(output_directory, device.value), "w" + "{}/{}_log".format(output_directory, device), "w" ) as file: file.write(self.dump_function[device]()) From 5d149fc0200f8d9beadddf977b7d7aca6bb00dbe Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Thu, 26 Sep 2024 16:28:38 +0800 Subject: [PATCH 35/36] fix: reflect changes in unit tests --- .../base/tests/test_reboot_check_test.py | 62 +++++++++---------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index 7e229b12ee..0576d40fac 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -5,10 +5,11 @@ import unittest import os import typing as T +import subprocess as sp -def do_nothing(_: T.List[str]): - return RCT.ShellResult(0, "", "") +def do_nothing(args: T.List[str], **kwargs): + return sp.CompletedProcess(args, 0, "".encode(), "".encode()) class UnitySupportParserTests(unittest.TestCase): @@ -108,7 +109,7 @@ def test_display_check_no_display_path(self): @patch( "reboot_check_test.HardwareRendererTester.parse_unity_support_output" ) - @patch("reboot_check_test.run_command") + @patch("subprocess.run") def test_is_hardware_renderer_available( self, mock_run: MagicMock, @@ -124,14 +125,16 @@ def test_is_hardware_renderer_available( @patch( "reboot_check_test.HardwareRendererTester.parse_unity_support_output" ) - @patch("reboot_check_test.run_command") + @patch("subprocess.run") def test_is_hardware_renderer_available_fail( self, mock_run: MagicMock, mock_parse: MagicMock, ): - mock_run.side_effect = lambda _: RCT.ShellResult(1, "", "") + mock_run.side_effect = lambda _: sp.CompletedProcess( + [], 1, "".encode(), "".encode() + ) tester = RCT.HardwareRendererTester() self.assertFalse(tester.is_hardware_renderer_available()) @@ -154,7 +157,7 @@ def tearDown(self): shutil.rmtree(self.temp_output_dir, ignore_errors=True) shutil.rmtree(self.temp_comparison_dir, ignore_errors=True) - def mock_run_command(self, args: T.List[str]) -> RCT.ShellResult: + def mock_run(self, args: T.List[str], **_) -> sp.CompletedProcess: stdout = "" if args[0] == "iw": stdout = """\ @@ -177,17 +180,19 @@ def mock_run_command(self, args: T.List[str]) -> RCT.ShellResult: else: raise Exception("Unexpected use of this mock") - return RCT.ShellResult(0, stdout, "") + return sp.CompletedProcess( + args, 0, stdout.encode(), "".encode() + ) - @patch("reboot_check_test.run_command") + @patch("subprocess.run") def test_info_dump_only_happy_path(self, mock_run: MagicMock): - # wrap over run_command's return value - mock_run.side_effect = self.mock_run_command + # wrap over run's return value + mock_run.side_effect = self.mock_run RCT.DeviceInfoCollector().dump(self.temp_output_dir) - @patch("reboot_check_test.run_command") + @patch("subprocess.run") def test_info_dump_and_comparison_happy_path(self, mock_run: MagicMock): - mock_run.side_effect = self.mock_run_command + mock_run.side_effect = self.mock_run collector = RCT.DeviceInfoCollector() @@ -204,7 +209,7 @@ def test_info_dump_and_comparison_happy_path(self, mock_run: MagicMock): with open( "{}/{}_log".format( self.temp_comparison_dir, - RCT.DeviceInfoCollector.Device.WIRELESS.value, + RCT.DeviceInfoCollector.Device.WIRELESS, ), "w", ) as f: @@ -222,7 +227,7 @@ def test_info_dump_and_comparison_happy_path(self, mock_run: MagicMock): with open( "{}/{}_log".format( self.temp_comparison_dir, - RCT.DeviceInfoCollector.Device.DRM.value, + RCT.DeviceInfoCollector.Device.DRM, ), "w", ) as f: @@ -237,34 +242,31 @@ def test_info_dump_and_comparison_happy_path(self, mock_run: MagicMock): class FailedServiceCheckerTests(unittest.TestCase): - @patch("reboot_check_test.run_command") + @patch("subprocess.run") def test_get_failed_services_happy_path(self, mock_run: MagicMock): - mock_run.return_value = RCT.ShellResult(0, "", "") + mock_run.return_value = sp.CompletedProcess( + [], 0, "".encode(), "".encode() + ) self.assertEqual(RCT.get_failed_services(), []) - @patch("reboot_check_test.run_command") + @patch("subprocess.run") def test_get_failed_services_with_failed_services( self, mock_run: MagicMock ): - mock_run.return_value = RCT.ShellResult( + mock_run.return_value = sp.CompletedProcess( + [], 0, "snap.checkbox.agent.service loaded failed failed Service\ - for snap applictaion checkbox.agent", + for snap applictaion checkbox.agent".encode(), "", ) self.assertEqual( - RCT.get_failed_services(), [mock_run.return_value.stdout] + RCT.get_failed_services(), [mock_run.return_value.stdout.decode()] ) class MainFunctionTests(unittest.TestCase): - def test_run_cmd_exception(self): - cmd = sh_split("non_existent_command -a -b -c") - output = RCT.run_command(cmd) - self.assertEqual(output.return_code, 1) - self.assertIn("Command non_existent_command not found", output.stderr) - @classmethod def setUpClass(cls): cls.temp_output_dir = "{}/temp_output_dir".format(os.getcwd()) @@ -274,7 +276,7 @@ def tearDown(self): shutil.rmtree(self.temp_output_dir, ignore_errors=True) shutil.rmtree(self.temp_comparison_dir, ignore_errors=True) - @patch("reboot_check_test.run_command") + @patch("subprocess.run") def test_partial_main(self, mock_run: MagicMock): # this test only validates the main function logic # (if it picks out the correct tests to run) @@ -282,9 +284,7 @@ def test_partial_main(self, mock_run: MagicMock): with patch( "sys.argv", - sh_split( - "reboot_check_test.py -d {}".format(self.temp_output_dir) - ), + sh_split("reboot_check_test.py -d {}".format(self.temp_output_dir)), ): RCT.main() self.assertEqual( @@ -312,7 +312,7 @@ def test_partial_main(self, mock_run: MagicMock): ) # only lspci, lsusb, iw calls self.assertEqual(mock_compare.call_count, 1) - @patch("reboot_check_test.run_command") + @patch("subprocess.run") def test_main_function_full(self, mock_run: MagicMock): mock_run.side_effect = do_nothing # Full suite From d636da3aaf63f63e7b255fd95d21ccddb3a8e478 Mon Sep 17 00:00:00 2001 From: Zhongning Li <60045212+tomli380576@users.noreply.github.com> Date: Thu, 26 Sep 2024 16:32:16 +0800 Subject: [PATCH 36/36] style: formatting --- providers/base/bin/reboot_check_test.py | 2 +- .../base/tests/test_reboot_check_test.py | 20 +++++++++---------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/providers/base/bin/reboot_check_test.py b/providers/base/bin/reboot_check_test.py index e33f5c2e69..17527b3447 100755 --- a/providers/base/bin/reboot_check_test.py +++ b/providers/base/bin/reboot_check_test.py @@ -76,7 +76,7 @@ def compare_device_lists( self, expected_dir: str, actual_dir: str, - devices: T.Dict[str, T.List[str]] = DEFAULT_DEVICES + devices: T.Dict[str, T.List[str]] = DEFAULT_DEVICES, ) -> bool: """Compares the list of devices in expected_dir against actual_dir diff --git a/providers/base/tests/test_reboot_check_test.py b/providers/base/tests/test_reboot_check_test.py index 0576d40fac..f179da53e4 100644 --- a/providers/base/tests/test_reboot_check_test.py +++ b/providers/base/tests/test_reboot_check_test.py @@ -180,9 +180,7 @@ def mock_run(self, args: T.List[str], **_) -> sp.CompletedProcess: else: raise Exception("Unexpected use of this mock") - return sp.CompletedProcess( - args, 0, stdout.encode(), "".encode() - ) + return sp.CompletedProcess(args, 0, stdout.encode(), "".encode()) @patch("subprocess.run") def test_info_dump_only_happy_path(self, mock_run: MagicMock): @@ -269,12 +267,12 @@ class MainFunctionTests(unittest.TestCase): @classmethod def setUpClass(cls): - cls.temp_output_dir = "{}/temp_output_dir".format(os.getcwd()) - cls.temp_comparison_dir = "{}/temp_comparison_dir".format(os.getcwd()) + cls.tmp_output_dir = "{}/temp_output_dir".format(os.getcwd()) + cls.tmp_comparison_dir = "{}/temp_comparison_dir".format(os.getcwd()) def tearDown(self): - shutil.rmtree(self.temp_output_dir, ignore_errors=True) - shutil.rmtree(self.temp_comparison_dir, ignore_errors=True) + shutil.rmtree(self.tmp_output_dir, ignore_errors=True) + shutil.rmtree(self.tmp_comparison_dir, ignore_errors=True) @patch("subprocess.run") def test_partial_main(self, mock_run: MagicMock): @@ -284,7 +282,7 @@ def test_partial_main(self, mock_run: MagicMock): with patch( "sys.argv", - sh_split("reboot_check_test.py -d {}".format(self.temp_output_dir)), + sh_split("reboot_check_test.py -d {}".format(self.tmp_output_dir)), ): RCT.main() self.assertEqual( @@ -298,7 +296,7 @@ def test_partial_main(self, mock_run: MagicMock): "sys.argv", sh_split( 'reboot_check_test.py -d "{}" -c "{}"'.format( - self.temp_output_dir, self.temp_comparison_dir + self.tmp_output_dir, self.tmp_comparison_dir ) ), ), patch( @@ -320,7 +318,7 @@ def test_main_function_full(self, mock_run: MagicMock): "sys.argv", sh_split( 'reboot_check_test.py -d "{}" -c "{}" -f -s -g'.format( - self.temp_output_dir, self.temp_comparison_dir + self.tmp_output_dir, self.tmp_comparison_dir ) ), ), patch( @@ -367,7 +365,7 @@ def test_only_comparison_is_specified(self): with patch( "sys.argv", sh_split( - 'reboot_check_test.py -c "{}"'.format(self.temp_output_dir) + 'reboot_check_test.py -c "{}"'.format(self.tmp_output_dir) ), ), self.assertRaises(ValueError): RCT.main()