diff --git a/doc/changelog.rst b/doc/changelog.rst index d69bd5755..d96b361ea 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -18,6 +18,7 @@ To be released at some future point in time Description +- ExecArgs handling correction - ReadTheDocs config file added and enabled on PRs - Enforce changelog updates - Remove deprecated SmartSim modules @@ -28,6 +29,8 @@ Description Detailed Notes +- Add checks and tests to ensure SmartSim users cannot initialize run settings + with a list of lists as the exe_args argument. (SmartSim-PR517_) - Add readthedocs configuration file and enable readthedocs builds on pull requests. Additionally added robots.txt file generation when readthedocs environment detected. (SmartSim-PR512_) @@ -46,6 +49,7 @@ Detailed Notes (SmartSim-PR504_) - Update the generic `t.Any` typehints in Experiment API. (SmartSim-PR501_) +.. _SmartSim-PR517: https://github.com/CrayLabs/SmartSim/pull/517 .. _SmartSim-PR512: https://github.com/CrayLabs/SmartSim/pull/512 .. _SmartSim-PR518: https://github.com/CrayLabs/SmartSim/pull/518 .. _SmartSim-PR514: https://github.com/CrayLabs/SmartSim/pull/514 diff --git a/smartsim/settings/base.py b/smartsim/settings/base.py index 284d435c0..d7760decc 100644 --- a/smartsim/settings/base.py +++ b/smartsim/settings/base.py @@ -446,15 +446,8 @@ def add_exe_args(self, args: t.Union[str, t.List[str]]) -> None: :param args: executable arguments :type args: str | list[str] - :raises TypeError: if exe args are not strings """ - if isinstance(args, str): - args = args.split() - - for arg in args: - if not isinstance(arg, str): - raise TypeError("Executable arguments should be a list of str") - + args = self._build_exe_args(args) self._exe_args.extend(args) def set( @@ -533,26 +526,26 @@ def set( @staticmethod def _build_exe_args(exe_args: t.Optional[t.Union[str, t.List[str]]]) -> t.List[str]: - """Convert exe_args input to a desired collection format""" - if exe_args: - if isinstance(exe_args, str): - return exe_args.split() - if isinstance(exe_args, list): - exe_args = copy.deepcopy(exe_args) - plain_type = all(isinstance(arg, (str)) for arg in exe_args) - if not plain_type: - nested_type = all( - all(isinstance(arg, (str)) for arg in exe_args_list) - for exe_args_list in exe_args - ) - if not nested_type: - raise TypeError( - "Executable arguments were not list of str or str" - ) - return exe_args - return exe_args - raise TypeError("Executable arguments were not list of str or str") - return [] + """Check and convert exe_args input to a desired collection format""" + if not exe_args: + return [] + + if isinstance(exe_args, list): + exe_args = copy.deepcopy(exe_args) + + if not ( + isinstance(exe_args, str) + or ( + isinstance(exe_args, list) + and all(isinstance(arg, str) for arg in exe_args) + ) + ): + raise TypeError("Executable arguments were not a list of str or a str.") + + if isinstance(exe_args, str): + return exe_args.split() + + return exe_args def format_run_args(self) -> t.List[str]: """Return formatted run arguments diff --git a/tests/test_run_settings.py b/tests/test_run_settings.py index b9439f41a..25566248d 100644 --- a/tests/test_run_settings.py +++ b/tests/test_run_settings.py @@ -185,55 +185,34 @@ def test_add_exe_args_list_of_mixed(): settings.add_exe_args(["1", "2", 3]) -def test_add_exe_args_space_delimited_string(): +def test_add_exe_args_list_of_lists(): """Ensure that any non-string exe arg fails validation for all""" settings = RunSettings("python") - expected = ["1", "2", "3"] - settings.add_exe_args("1 2 3") - - assert settings.exe_args == expected - - -def test_add_exe_args_list_of_mixed_lists(): - """Ensure that any non-string exe arg fails validation for all""" - settings = RunSettings("python") - with pytest.raises(TypeError) as type_error: - settings.add_exe_args([["1", "2", 3], ["4", "5", 6]]) - - assert "Executable arguments should be a list of str" in type_error.value.args - - -def test_add_exe_args_list_of_mixed_lists_init(): - """Ensure that any non-string exe arg fails validation for all""" - exe_args = [["1", "2", 3], ["4", "5", 6]] - - with pytest.raises(TypeError) as type_error: - settings = RunSettings("python", exe_args=exe_args) - - assert "Executable arguments were not list of str or str" in type_error.value.args + with pytest.raises(TypeError): + settings.add_exe_args(["1", "2", "3"], ["1", "2", "3"]) -def test_add_exe_args_list_of_str_lists_init(): - """Ensure that list[list[str]] pass validation""" +def test_init_exe_args_list_of_lists(): + """Ensure that a list of lists exe arg fails validation""" exe_args = [["1", "2", "3"], ["4", "5", "6"]] + with pytest.raises(TypeError): + _ = RunSettings("python", exe_args=exe_args) - settings = RunSettings("python", exe_args=exe_args) - assert settings.exe_args == exe_args +def test_init_exe_args_list_of_lists_mixed(): + """Ensure that a list of lists exe arg fails validation""" + exe_args = [["1", "2", 3], ["4", "5", 6]] + with pytest.raises(TypeError): + _ = RunSettings("python", exe_args=exe_args) -def test_add_exe_args_list_of_str_lists(): - """Ensure that list[list[str]] fail validation when added via method""" - exe_args = [["1", "2", "3"], ["4", "5", "6"]] - +def test_add_exe_args_space_delimited_string(): + """Ensure that any non-string exe arg fails validation for all""" settings = RunSettings("python") + expected = ["1", "2", "3"] + settings.add_exe_args("1 2 3") - with pytest.raises(TypeError) as type_error: - settings.add_exe_args(exe_args) - - # NOTE that this behavior differs from sending constructor args like - # tested in test_add_exe_args_list_of_str_lists_init where it's allowed - assert "Executable arguments should be a list of str" in type_error.value.args + assert settings.exe_args == expected def test_format_run_args():