diff --git a/tests/helper/test_helper.py b/tests/helper/test_helper.py index 216b03e16a..a384821f63 100644 --- a/tests/helper/test_helper.py +++ b/tests/helper/test_helper.py @@ -548,7 +548,7 @@ def test_download_media( ("Linux", True, True, "xdg-open", "/path/to/media"), ("MacOS", True, True, "open", "/path/to/media"), ("WSL", True, True, "explorer.exe", "\\path\\to\\media"), - ("UnknownOS", True, False, "unknown-tool", "/path/to/media"), + ("UnknownOS", True, False, "invalid", "/path/to/media"), ], ids=[ "Linux_os_user", @@ -572,9 +572,13 @@ def test_process_media( MODULE + ".download_media", return_value=media_path ) mocked_open_media = mocker.patch(MODULE + ".open_media") - mocker.patch(MODULE + ".PLATFORM", platform) - mocker.patch("zulipterminal.platform_code.PLATFORM", platform) - mocker.patch("zulipterminal.core.Controller.show_media_confirmation_popup") + + # Mocking the PLATFORM variable in both platform_code and the main module + # to ensure consistency during tests that rely on platform detection. + mocker.patch("zulipterminal.platform_code" + ".PLATFORM", platform) + mocker.patch( + "zulipterminal.platform_code" + ".process_media_tool", return_value=tool + ) process_media(controller, link) @@ -603,28 +607,49 @@ def test_process_media_empty_url( @pytest.mark.parametrize( - "returncode, error", + "platform, returncode, tool, error", [ - (0, []), - ( + case("Linux", 0, "xdg-open", [], id="Linux:success"), + case("MacOS", 0, "open", [], id="MacOS:success"), + case("WSL", 1, "explorer.exe", [], id="WSL:success"), + case( + "Linux", 1, + "xdg-open", [ " The tool ", ("footer_contrast", "xdg-open"), " did not run successfully" ". Exited with ", ("footer_contrast", "1"), ], + id="Linux:error", ), + case( + "MacOS", + 1, + "open", + [ + " The tool ", + ("footer_contrast", "open"), + " did not run successfully" ". Exited with ", + ("footer_contrast", "1"), + ], + id="Mac:error", + ), + # NOTE: explorer.exe (WSL) always returns a non-zero exit code (1) + # so we do not test for it. ], ) def test_open_media( mocker: MockerFixture, + platform: str, returncode: int, + tool: str, error: List[Any], - tool: str = "xdg-open", media_path: str = "/tmp/zt-somerandomtext-image.png", ) -> None: mocked_run = mocker.patch(MODULE + ".subprocess.run") + mocker.patch("zulipterminal.platform_code" + ".PLATFORM", platform) mocked_run.return_value.returncode = returncode controller = mocker.Mock() diff --git a/tests/platform_code/test_platform_code.py b/tests/platform_code/test_platform_code.py index 21669da025..d0c94018d0 100644 --- a/tests/platform_code/test_platform_code.py +++ b/tests/platform_code/test_platform_code.py @@ -6,7 +6,7 @@ SupportedPlatforms, normalized_file_path, notify, - successful_GUI_return_code, + validate_GUI_exit_status, ) @@ -76,20 +76,25 @@ def test_notify_quotes( @pytest.mark.parametrize( - "platform, expected_return_code", + "platform, return_code, expected_return_value", [ - ("Linux", 0), - ("MacOS", 0), - ("WSL", 1), + ("Linux", 0, "success"), + ("MacOS", 0, "success"), + ("WSL", 1, "success"), + ("Linux", 1, "failure"), + ("MacOS", 1, "failure"), + # NOTE: explorer.exe (WSL) always returns a non-zero exit code (1), + # so we do not test for it. ], ) -def test_successful_GUI_return_code( +def test_validate_GUI_exit_status( mocker: MockerFixture, platform: SupportedPlatforms, - expected_return_code: int, + return_code: int, + expected_return_value: str, ) -> None: mocker.patch(MODULE + ".PLATFORM", platform) - assert successful_GUI_return_code() == expected_return_code + assert validate_GUI_exit_status(return_code) == expected_return_value @pytest.mark.parametrize( diff --git a/zulipterminal/helper.py b/zulipterminal/helper.py index a71d055b74..32cd931db6 100644 --- a/zulipterminal/helper.py +++ b/zulipterminal/helper.py @@ -40,9 +40,9 @@ REGEX_QUOTED_FENCE_LENGTH, ) from zulipterminal.platform_code import ( - PLATFORM, + process_media_tool, normalized_file_path, - successful_GUI_return_code, + validate_GUI_exit_status, ) @@ -785,18 +785,10 @@ def process_media(controller: Any, link: str) -> None: ) media_path = download_media(controller, link, show_download_status) media_path = normalized_file_path(media_path) - tool = "" - - # TODO: Add support for other platforms as well. - if PLATFORM == "WSL": - tool = "explorer.exe" - # Modifying path to backward slashes instead of forward slashes - media_path = media_path.replace("/", "\\") - elif PLATFORM == "Linux": - tool = "xdg-open" - elif PLATFORM == "MacOS": - tool = "open" - else: + + tool = process_media_tool() + + if tool == "invalid": controller.report_error("Media not supported for this platform") return @@ -841,7 +833,7 @@ def open_media(controller: Any, tool: str, media_path: str) -> None: command, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL ) exit_status = process.returncode - if exit_status != successful_GUI_return_code(): + if validate_GUI_exit_status(exit_status) == "failure": error = [ " The tool ", ("footer_contrast", tool), diff --git a/zulipterminal/platform_code.py b/zulipterminal/platform_code.py index f05853c144..98dd674039 100644 --- a/zulipterminal/platform_code.py +++ b/zulipterminal/platform_code.py @@ -89,18 +89,19 @@ def notify(title: str, text: str) -> str: return "" -def successful_GUI_return_code() -> int: # noqa: N802 (allow upper case) +def validate_GUI_exit_status( # noqa: N802 (allow upper case) + exit_status: int, +) -> Literal["success", "failure"]: """ Returns success return code for GUI commands, which are OS specific. """ - # WSL uses GUI return code as 1. Refer below link to know more: - # https://stackoverflow.com/questions/52423031/ - # why-does-opening-an-explorer-window-and-selecting-a-file-through-pythons-subpro/ - # 52423798#52423798 + # NOTE: WSL (explorer.exe) always returns a non-zero exit code (1) for GUI commands. + # This is a known issue. Therefore, we consider it a success if the exit code is 1. + # For more information, see: https://github.com/microsoft/WSL/issues/6565 if PLATFORM == "WSL": - return 1 + return "success" - return 0 + return "success" if exit_status == 0 else "failure" def normalized_file_path(path: str) -> str: