From 8a4a7df60f000e0097028702cd1fcfd22ea77bf4 Mon Sep 17 00:00:00 2001 From: Matt Stone Date: Wed, 18 Sep 2024 13:54:58 -0400 Subject: [PATCH 01/10] fix: Executable path validation tests should not be dependent on user env --- tests/util/test_executable_runner.py | 40 ++++++++++++++++++---------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/tests/util/test_executable_runner.py b/tests/util/test_executable_runner.py index 0d75dca..25f401b 100644 --- a/tests/util/test_executable_runner.py +++ b/tests/util/test_executable_runner.py @@ -1,6 +1,7 @@ import os from pathlib import Path from tempfile import NamedTemporaryFile +from unittest import mock import pytest @@ -30,21 +31,32 @@ def test_validate_executable_path_not_executable() -> None: def test_validate_executable_path() -> None: - exec = "yes" - exec_full_str = f"/usr/bin/{exec}" - exec_full_path = Path(exec_full_str) - assert exec_full_path.is_absolute() - - # find it on the PATH - assert exec_full_path == ExecutableRunner.validate_executable_path(executable=exec) - # find it given an absolute path as a string - assert exec_full_path == ExecutableRunner.validate_executable_path(executable=exec_full_str) - # find it given an absolute path as a Path - assert exec_full_path == ExecutableRunner.validate_executable_path(executable=exec_full_path) - - # do not find it on the PATH if given as a Path + """ + `validate_executable_path` should find the `yes` executable in the following scenarios: + 1. when the string "yes" is passed + 2. when the absolute path to the `yes` executable is passed, either as a string or a Path + """ + executables: list[Path | str] = ["yes", "/usr/bin/yes", Path("/usr/bin/yes")] + expected_path = Path("/usr/bin/yes") + + with mock.patch.dict(os.environ): + # Clear the PATH, in case the user has a local version of `yes` elsewhere on their PATH + os.environ.pop("PATH") + + for executable in executables: + validated_path = ExecutableRunner.validate_executable_path(executable=executable) + assert validated_path == expected_path + + +def test_validate_executable_path_rejects_paths() -> None: + """ + `validate_executable_path` should not treat non-existent Path objects as valid executables. + + If the user passes the name of an executable on the PATH as a `Path` instead of a string`, it + should be treated as a non-existent Path and a `ValueError` should be raised. + """ with pytest.raises(ValueError, match="Executable does not exist"): - ExecutableRunner.validate_executable_path(executable=Path(exec)) + ExecutableRunner.validate_executable_path(executable=Path("yes")) def test_validate_executable_path_new_file() -> None: From dbbee0049cfd8117fc5c4d269c60df8ce9ef5bdf Mon Sep 17 00:00:00 2001 From: Matt Stone Date: Wed, 18 Sep 2024 13:57:55 -0400 Subject: [PATCH 02/10] refactor: pytest.mark.parametrize --- tests/util/test_executable_runner.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/util/test_executable_runner.py b/tests/util/test_executable_runner.py index 25f401b..ea9cf77 100644 --- a/tests/util/test_executable_runner.py +++ b/tests/util/test_executable_runner.py @@ -30,22 +30,21 @@ def test_validate_executable_path_not_executable() -> None: ExecutableRunner.validate_executable_path(executable=tmpfile.name) -def test_validate_executable_path() -> None: +@pytest.mark.parametrize("executable", ["yes", "/usr/bin/yes", Path("/usr/bin/yes")]) +def test_validate_executable_path(executable: str | Path) -> None: """ `validate_executable_path` should find the `yes` executable in the following scenarios: 1. when the string "yes" is passed 2. when the absolute path to the `yes` executable is passed, either as a string or a Path """ - executables: list[Path | str] = ["yes", "/usr/bin/yes", Path("/usr/bin/yes")] expected_path = Path("/usr/bin/yes") with mock.patch.dict(os.environ): # Clear the PATH, in case the user has a local version of `yes` elsewhere on their PATH os.environ.pop("PATH") - for executable in executables: - validated_path = ExecutableRunner.validate_executable_path(executable=executable) - assert validated_path == expected_path + validated_path = ExecutableRunner.validate_executable_path(executable=executable) + assert validated_path == expected_path def test_validate_executable_path_rejects_paths() -> None: From 45b1b3e558e36a87ce1308d29222f64856f9d706 Mon Sep 17 00:00:00 2001 From: Matt Stone Date: Wed, 18 Sep 2024 13:58:42 -0400 Subject: [PATCH 03/10] chore: type hints --- tests/util/test_executable_runner.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/util/test_executable_runner.py b/tests/util/test_executable_runner.py index ea9cf77..18c19cb 100644 --- a/tests/util/test_executable_runner.py +++ b/tests/util/test_executable_runner.py @@ -37,13 +37,13 @@ def test_validate_executable_path(executable: str | Path) -> None: 1. when the string "yes" is passed 2. when the absolute path to the `yes` executable is passed, either as a string or a Path """ - expected_path = Path("/usr/bin/yes") + expected_path: Path = Path("/usr/bin/yes") with mock.patch.dict(os.environ): # Clear the PATH, in case the user has a local version of `yes` elsewhere on their PATH os.environ.pop("PATH") - validated_path = ExecutableRunner.validate_executable_path(executable=executable) + validated_path: Path = ExecutableRunner.validate_executable_path(executable=executable) assert validated_path == expected_path From 56d2d2625a39fa1b5ed0057a002525ebe049090e Mon Sep 17 00:00:00 2001 From: Matt Stone Date: Wed, 18 Sep 2024 14:04:16 -0400 Subject: [PATCH 04/10] fix: test --- tests/util/test_executable_runner.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/tests/util/test_executable_runner.py b/tests/util/test_executable_runner.py index 18c19cb..6e859f4 100644 --- a/tests/util/test_executable_runner.py +++ b/tests/util/test_executable_runner.py @@ -30,12 +30,11 @@ def test_validate_executable_path_not_executable() -> None: ExecutableRunner.validate_executable_path(executable=tmpfile.name) -@pytest.mark.parametrize("executable", ["yes", "/usr/bin/yes", Path("/usr/bin/yes")]) -def test_validate_executable_path(executable: str | Path) -> None: +@pytest.mark.parametrize("executable", ["/usr/bin/yes", Path("/usr/bin/yes")]) +def test_validate_executable_path_returns_existing_paths(executable: str | Path) -> None: """ - `validate_executable_path` should find the `yes` executable in the following scenarios: - 1. when the string "yes" is passed - 2. when the absolute path to the `yes` executable is passed, either as a string or a Path + `validate_executable_path` should return the `yes` executable when the absolute path to the + executable is passed, either as a string or a Path. """ expected_path: Path = Path("/usr/bin/yes") @@ -47,6 +46,18 @@ def test_validate_executable_path(executable: str | Path) -> None: assert validated_path == expected_path +def test_validate_executable_finds_on_path() -> None: + """ + `validate_executable_path` should find executables on the PATH when the name of the executable + is passed as a string. + """ + expected_path: Path = Path("/usr/bin/yes") + + with mock.patch("shutil.which", return_value=expected_path.as_posix()): + validated_path: Path = ExecutableRunner.validate_executable_path(executable="yes") + assert validated_path == expected_path + + def test_validate_executable_path_rejects_paths() -> None: """ `validate_executable_path` should not treat non-existent Path objects as valid executables. From f3af01f9760aa5e5d96ebe121d6e790a2e284124 Mon Sep 17 00:00:00 2001 From: Matt Stone Date: Thu, 19 Sep 2024 09:40:54 -0400 Subject: [PATCH 05/10] wip: clear path --- tests/util/test_executable_runner.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/tests/util/test_executable_runner.py b/tests/util/test_executable_runner.py index 6e859f4..13c8bf4 100644 --- a/tests/util/test_executable_runner.py +++ b/tests/util/test_executable_runner.py @@ -19,12 +19,20 @@ def test_close_twice() -> None: assert exec.close() is False -def test_validate_executable_path_does_not_eexist() -> None: +def test_validate_executable_path_does_not_exist() -> None: + """ + `validate_executable_path` should raise a ValueError when the provided executable does not + exist. + """ with pytest.raises(ValueError, match="Executable does not exist"): ExecutableRunner.validate_executable_path(executable="/path/to/nowhere") def test_validate_executable_path_not_executable() -> None: + """ + `validate_executable_path` should raise a ValueError when the provided executable does not have + execute permissions. + """ with NamedTemporaryFile(suffix=".exe", mode="w", delete=True) as tmpfile: with pytest.raises(ValueError, match="is not executable"): ExecutableRunner.validate_executable_path(executable=tmpfile.name) @@ -38,9 +46,9 @@ def test_validate_executable_path_returns_existing_paths(executable: str | Path) """ expected_path: Path = Path("/usr/bin/yes") - with mock.patch.dict(os.environ): - # Clear the PATH, in case the user has a local version of `yes` elsewhere on their PATH - os.environ.pop("PATH") + # Clear the PATH, in case the user has a local version of `yes` elsewhere on their PATH + with mock.patch.dict(os.environ, clear=True): + os.environ["PATH"] = "/usr/bin" validated_path: Path = ExecutableRunner.validate_executable_path(executable=executable) assert validated_path == expected_path @@ -62,8 +70,8 @@ def test_validate_executable_path_rejects_paths() -> None: """ `validate_executable_path` should not treat non-existent Path objects as valid executables. - If the user passes the name of an executable on the PATH as a `Path` instead of a string`, it - should be treated as a non-existent Path and a `ValueError` should be raised. + Specifically, if the user passes the name of an executable on the PATH as a `Path` instead of a + string, it should be treated as a non-existent Path and a `ValueError` should be raised. """ with pytest.raises(ValueError, match="Executable does not exist"): ExecutableRunner.validate_executable_path(executable=Path("yes")) From bbfdf6d68a5ff53f2d0cd7afaf6d5e619a4de319 Mon Sep 17 00:00:00 2001 From: Matt Stone Date: Thu, 19 Sep 2024 09:42:10 -0400 Subject: [PATCH 06/10] refactor: consolidate tests --- tests/util/test_executable_runner.py | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/tests/util/test_executable_runner.py b/tests/util/test_executable_runner.py index 13c8bf4..093f120 100644 --- a/tests/util/test_executable_runner.py +++ b/tests/util/test_executable_runner.py @@ -38,11 +38,12 @@ def test_validate_executable_path_not_executable() -> None: ExecutableRunner.validate_executable_path(executable=tmpfile.name) -@pytest.mark.parametrize("executable", ["/usr/bin/yes", Path("/usr/bin/yes")]) -def test_validate_executable_path_returns_existing_paths(executable: str | Path) -> None: +@pytest.mark.parametrize("executable", ["yes", "/usr/bin/yes", Path("/usr/bin/yes")]) +def test_validate_executable_path(executable: str | Path) -> None: """ - `validate_executable_path` should return the `yes` executable when the absolute path to the - executable is passed, either as a string or a Path. + `validate_executable_path` should return the `yes` executable in the following scenarios: + 1. When the name of the executable is passed as a string. + 2. When the absolute path to the executable is passed, either as a string or a Path. """ expected_path: Path = Path("/usr/bin/yes") @@ -54,18 +55,6 @@ def test_validate_executable_path_returns_existing_paths(executable: str | Path) assert validated_path == expected_path -def test_validate_executable_finds_on_path() -> None: - """ - `validate_executable_path` should find executables on the PATH when the name of the executable - is passed as a string. - """ - expected_path: Path = Path("/usr/bin/yes") - - with mock.patch("shutil.which", return_value=expected_path.as_posix()): - validated_path: Path = ExecutableRunner.validate_executable_path(executable="yes") - assert validated_path == expected_path - - def test_validate_executable_path_rejects_paths() -> None: """ `validate_executable_path` should not treat non-existent Path objects as valid executables. From 75568bb07d91b7c4c1a756bacdef657100e98c9d Mon Sep 17 00:00:00 2001 From: Matt Stone Date: Thu, 19 Sep 2024 09:49:29 -0400 Subject: [PATCH 07/10] refactor: use tmp path instead of /usr/bin --- tests/util/test_executable_runner.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/util/test_executable_runner.py b/tests/util/test_executable_runner.py index 093f120..b42a205 100644 --- a/tests/util/test_executable_runner.py +++ b/tests/util/test_executable_runner.py @@ -38,21 +38,24 @@ def test_validate_executable_path_not_executable() -> None: ExecutableRunner.validate_executable_path(executable=tmpfile.name) -@pytest.mark.parametrize("executable", ["yes", "/usr/bin/yes", Path("/usr/bin/yes")]) -def test_validate_executable_path(executable: str | Path) -> None: +def test_validate_executable_path(tmp_path: Path) -> None: """ `validate_executable_path` should return the `yes` executable in the following scenarios: 1. When the name of the executable is passed as a string. 2. When the absolute path to the executable is passed, either as a string or a Path. """ - expected_path: Path = Path("/usr/bin/yes") + expected_path = tmp_path / "yes" + expected_path.touch() # create the file + expected_path.chmod(755) # make it executable # Clear the PATH, in case the user has a local version of `yes` elsewhere on their PATH with mock.patch.dict(os.environ, clear=True): - os.environ["PATH"] = "/usr/bin" + os.environ["PATH"] = str(tmp_path) - validated_path: Path = ExecutableRunner.validate_executable_path(executable=executable) - assert validated_path == expected_path + executables: list[str | Path] = ["yes", expected_path, str(expected_path)] + for executable in executables: + validated_path: Path = ExecutableRunner.validate_executable_path(executable=executable) + assert validated_path == expected_path def test_validate_executable_path_rejects_paths() -> None: From 425738c101c3b8cb3f7f7f66f105adaa7af176ca Mon Sep 17 00:00:00 2001 From: Matt Stone Date: Thu, 19 Sep 2024 09:51:05 -0400 Subject: [PATCH 08/10] test: update doc --- tests/util/test_executable_runner.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/util/test_executable_runner.py b/tests/util/test_executable_runner.py index b42a205..e6d8bf6 100644 --- a/tests/util/test_executable_runner.py +++ b/tests/util/test_executable_runner.py @@ -41,8 +41,8 @@ def test_validate_executable_path_not_executable() -> None: def test_validate_executable_path(tmp_path: Path) -> None: """ `validate_executable_path` should return the `yes` executable in the following scenarios: - 1. When the name of the executable is passed as a string. - 2. When the absolute path to the executable is passed, either as a string or a Path. + 1. The name of the executable is passed as a string, and the executable is on the user's PATH. + 2. The absolute path to the executable is passed, either as a string or a Path. """ expected_path = tmp_path / "yes" expected_path.touch() # create the file From 19893fd4c8acbfa1fb9cf043913c1fdb7eeeca2a Mon Sep 17 00:00:00 2001 From: Matt Stone Date: Thu, 19 Sep 2024 09:52:02 -0400 Subject: [PATCH 09/10] doc: update comment --- tests/util/test_executable_runner.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/util/test_executable_runner.py b/tests/util/test_executable_runner.py index e6d8bf6..d6488b6 100644 --- a/tests/util/test_executable_runner.py +++ b/tests/util/test_executable_runner.py @@ -45,10 +45,10 @@ def test_validate_executable_path(tmp_path: Path) -> None: 2. The absolute path to the executable is passed, either as a string or a Path. """ expected_path = tmp_path / "yes" - expected_path.touch() # create the file - expected_path.chmod(755) # make it executable + expected_path.touch() + expected_path.chmod(755) - # Clear the PATH, in case the user has a local version of `yes` elsewhere on their PATH + # Clear the PATH, to override any local versions of `yes` on the user's PATH with mock.patch.dict(os.environ, clear=True): os.environ["PATH"] = str(tmp_path) From 50627066aedcebd708257057e6204bbd2e41b1fb Mon Sep 17 00:00:00 2001 From: Matt Stone Date: Thu, 19 Sep 2024 09:57:10 -0400 Subject: [PATCH 10/10] test: cleanup --- tests/util/test_executable_runner.py | 34 ++++++++-------------------- 1 file changed, 10 insertions(+), 24 deletions(-) diff --git a/tests/util/test_executable_runner.py b/tests/util/test_executable_runner.py index d6488b6..017220e 100644 --- a/tests/util/test_executable_runner.py +++ b/tests/util/test_executable_runner.py @@ -1,6 +1,5 @@ import os from pathlib import Path -from tempfile import NamedTemporaryFile from unittest import mock import pytest @@ -19,23 +18,27 @@ def test_close_twice() -> None: assert exec.close() is False -def test_validate_executable_path_does_not_exist() -> None: +def test_validate_executable_path_does_not_exist(tmp_path: Path) -> None: """ `validate_executable_path` should raise a ValueError when the provided executable does not exist. """ + bad_path: Path = tmp_path / "nowhere" + with pytest.raises(ValueError, match="Executable does not exist"): - ExecutableRunner.validate_executable_path(executable="/path/to/nowhere") + ExecutableRunner.validate_executable_path(executable=bad_path) -def test_validate_executable_path_not_executable() -> None: +def test_validate_executable_path_not_executable(tmp_path: Path) -> None: """ `validate_executable_path` should raise a ValueError when the provided executable does not have execute permissions. """ - with NamedTemporaryFile(suffix=".exe", mode="w", delete=True) as tmpfile: - with pytest.raises(ValueError, match="is not executable"): - ExecutableRunner.validate_executable_path(executable=tmpfile.name) + bad_path: Path = tmp_path / "not_executable" + bad_path.touch() + + with pytest.raises(ValueError, match="is not executable"): + ExecutableRunner.validate_executable_path(executable=bad_path) def test_validate_executable_path(tmp_path: Path) -> None: @@ -67,20 +70,3 @@ def test_validate_executable_path_rejects_paths() -> None: """ with pytest.raises(ValueError, match="Executable does not exist"): ExecutableRunner.validate_executable_path(executable=Path("yes")) - - -def test_validate_executable_path_new_file() -> None: - with NamedTemporaryFile(suffix=".exe", mode="w", delete=True) as tmpfile: - exec_str: str = tmpfile.name - exec_path: Path = Path(exec_str) - # not an executable - with pytest.raises(ValueError, match="is not executable"): - ExecutableRunner.validate_executable_path(executable=exec_str) - # make it executable and test again - os.chmod(exec_str, 755) - exec_full_path: Path = exec_path.absolute() - assert exec_full_path == ExecutableRunner.validate_executable_path(executable=exec_str) - assert exec_full_path == ExecutableRunner.validate_executable_path(executable=exec_path) - assert exec_full_path == ExecutableRunner.validate_executable_path( - executable=exec_full_path - )