From 950ffeb8ad8872a8edfb38a315137125dc75156e Mon Sep 17 00:00:00 2001 From: Chris Harris Date: Tue, 15 Sep 2026 16:47:59 +0100 Subject: [PATCH] workloads: fix a typing issue As part of a code review for PR 359 it was noticed that in some instances the command classes were being initalised with a dict containing str, int, bool values when the constructor was explicitly expecting strings. Fix that type mis-match so that the options passed in to the *command classes are definitely in a dict[str, Union[str, list[str]] format IBM Bob 2.0.3 was used to help with this code. Signed-off-by: Chris Harris --- command/fio_command.py | 10 +++--- tests/test_workloads.py | 72 ++++++++++++++++++++++++++++++++++++++++- workloads/workloads.py | 37 +++++++++++++++++++-- 3 files changed, 110 insertions(+), 9 deletions(-) diff --git a/command/fio_command.py b/command/fio_command.py index 7552e83e..f6abef16 100644 --- a/command/fio_command.py +++ b/command/fio_command.py @@ -77,10 +77,10 @@ def _parse_options(self, options: dict[str, str]) -> CliOptions: if options.get("rate_iops", None) is not None: fio_cli_options["rate_iops"] = options.get("rate_iops", None) - if bool(options.get("time_based", False)) is True: + if options.get("time_based", "false") == "true": fio_cli_options["time_based"] = "" - if bool(options.get("no_sudo", False)) is False: + if options.get("no_sudo", "false") != "true": fio_cli_options["sudo"] = "" if options.get("norandommap", None) is not None: @@ -96,13 +96,13 @@ def _parse_options(self, options: dict[str, str]) -> CliOptions: fio_cli_options["rwmixread"] = read_percent fio_cli_options["rwmixwrite"] = write_percent - if bool(options.get("log_iops", True)): + if options.get("log_iops", "true") != "false": fio_cli_options["log_iops"] = "" - if bool(options.get("log_bw", True)): + if options.get("log_bw", "true") != "false": fio_cli_options["log_bw"] = "" - if bool(options.get("log_lat", True)): + if options.get("log_lat", "true") != "false": fio_cli_options["log_lat"] = "" processes_per_volume: int = int(options.get("procs_per_volume", 1)) diff --git a/tests/test_workloads.py b/tests/test_workloads.py index 484eaa07..2cc71133 100644 --- a/tests/test_workloads.py +++ b/tests/test_workloads.py @@ -300,7 +300,7 @@ def test_list_conversion_in_global_options(self) -> None: self.assertEqual(workloads._global_options["iodepth"], ["4", "8"]) def test_string_conversion_in_global_options(self) -> None: - """Test that non-list values are converted to strings""" + """Test that non-list scalar values in global config are converted to strings""" config: dict[str, Any] = { "time": 300, # int "ramp": 30, # int @@ -314,6 +314,76 @@ def test_string_conversion_in_global_options(self) -> None: self.assertIsInstance(workloads._global_options["time"], str) self.assertEqual(workloads._global_options["time"], "300") + def test_int_elements_in_global_list_options_are_stringified(self) -> None: + """Test that integer elements inside a list in global config are converted to strings. + + YAML can produce list[int] for values like 'total_iodepth: [16, 32]'. + all_configs() unpacks list elements directly into the options dict, so + each element must be a str before it reaches FioCommand. + """ + config: dict[str, Any] = { + "total_iodepth": [16, 32], # list of ints from YAML + "workloads": { + "test": {"mode": "randwrite"}, + }, + } + workloads: Workloads = self._create_workloads(config) + + iodepth_list = workloads._global_options["total_iodepth"] + self.assertIsInstance(iodepth_list, list) + for element in iodepth_list: + self.assertIsInstance(element, str, f"Expected str, got {type(element)} for value {element!r}") + + def test_scalar_non_str_in_workload_options_are_stringified(self) -> None: + """Test that scalar non-str values in workload-specific options are converted to strings. + + Values such as 'time: 600' or 'numjobs: 1' arrive as int from YAML + and must be str before reaching FioCommand. + """ + config: dict[str, Any] = { + "workloads": { + "test": { + "mode": "randwrite", + "time": 600, # int + "numjobs": 1, # int + "monitor": False, # bool + }, + }, + } + workloads: Workloads = self._create_workloads(config) + + workload = workloads._workloads[0] + for key in ("time", "numjobs", "monitor"): + value = workload._all_options[key] + self.assertIsInstance(value, str, f"Expected str for '{key}', got {type(value)} ({value!r})") + + self.assertEqual(workload._all_options["time"], "600") + self.assertEqual(workload._all_options["numjobs"], "1") + self.assertEqual(workload._all_options["monitor"], "false") + + def test_int_elements_in_workload_list_options_are_stringified(self) -> None: + """Test that integer elements inside a list in workload-specific options are converted to strings. + + A workload option like 'total_iodepth: [16, 32]' from YAML produces + list[int]; all_configs() unpacks those integers straight into the + options dict that FioCommand receives, so they must be strings first. + """ + config: dict[str, Any] = { + "workloads": { + "test": { + "mode": "randwrite", + "total_iodepth": [16, 32], # list of ints from YAML + }, + }, + } + workloads: Workloads = self._create_workloads(config) + + workload = workloads._workloads[0] + iodepth_list = workload._all_options["total_iodepth"] + self.assertIsInstance(iodepth_list, list) + for element in iodepth_list: + self.assertIsInstance(element, str, f"Expected str, got {type(element)} for value {element!r}") + if __name__ == "__main__": unittest.main() diff --git a/workloads/workloads.py b/workloads/workloads.py index 8ead933a..65f35947 100644 --- a/workloads/workloads.py +++ b/workloads/workloads.py @@ -154,12 +154,43 @@ def _create_configurations(self, workload_json: WorkloadYamlType) -> None: Get the options needed to construct the benchmark command to run the test """ for workload_name, workload_options in workload_json.items(): - workload = Workload(workload_name, workload_options, self._base_run_directory) + normalised_options = self._normalise_options(workload_options) + workload = Workload(workload_name, normalised_options, self._base_run_directory) workload.add_global_options(self._global_options) # workload.set_benchmark_type(self._benchmark_type) self._workloads.append(workload) + @staticmethod + def _to_str(value: object) -> str: + """Convert a scalar option value to its string representation. + + Booleans are lowercased (``True`` → ``"true"``, ``False`` → ``"false"``) + so that downstream CLI tools receive the conventional form. + The bool check must precede any int check because ``bool`` is a subclass + of ``int`` in Python. + """ + if isinstance(value, bool): + return str(value).lower() + return str(value) + + @staticmethod + def _normalise_options(options: WorkloadType) -> WorkloadType: + """ + Convert every option value (and every element within list values) + to a string so that dict[str, str | list[str]] is consistently + typed before being handed to Workload / FioCommand (which expect + dict[str, str]). + Booleans are lowercased (``True`` → ``"true"``, ``False`` → ``"false"``). + """ + normalised: WorkloadType = {} + for key, value in options.items(): + if isinstance(value, list): + normalised[key] = [Workloads._to_str(item) for item in value] + else: + normalised[key] = Workloads._to_str(value) + return normalised + def _get_global_options_from_configuration(self, configuration: BenchmarkConfigurationType) -> WorkloadType: """ Get any configuration options from the test plan .yaml that are not workload @@ -174,8 +205,8 @@ def _get_global_options_from_configuration(self, configuration: BenchmarkConfigu # workloads we also want to ignore as these will be dealt with at a later date pass elif isinstance(value, list): - global_options[option_name] = value + global_options[option_name] = [Workloads._to_str(item) for item in value] else: - global_options[option_name] = f"{value}" + global_options[option_name] = Workloads._to_str(value) return global_options