Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tests failing on WSL systems. #1511

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 33 additions & 7 deletions tests/helper/test_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -572,8 +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.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)

Expand Down Expand Up @@ -602,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"),
Comment on lines +632 to +635
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading this again, a reasonable PR after this would be to not output the error code, but provide more detailed output somewhere for followup later, a little like we do with the crash or threading errors. That, or try and diagnose further why the tool did not run properly.

We could also possibly provide a 'ran successfully' message in the footer for opening, but I've not looked at the code flow for that, and whether it would eg. overwrite other messages too rapidly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood 👍

Wouldn't diagnosing why the tool did not run properly be platform dependent though?

],
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()

Expand Down
21 changes: 13 additions & 8 deletions tests/platform_code/test_platform_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
SupportedPlatforms,
normalized_file_path,
notify,
successful_GUI_return_code,
validate_GUI_exit_status,
)


Expand Down Expand Up @@ -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(
Expand Down
22 changes: 7 additions & 15 deletions zulipterminal/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@
REGEX_QUOTED_FENCE_LENGTH,
)
from zulipterminal.platform_code import (
PLATFORM,
normalized_file_path,
successful_GUI_return_code,
process_media_tool,
validate_GUI_exit_status,
)


Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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),
Expand Down
31 changes: 24 additions & 7 deletions zulipterminal/platform_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -112,3 +113,19 @@ def normalized_file_path(path: str) -> str:
return path.replace("/", "\\")

return path


def process_media_tool() -> str:
"""
Returns the media tool command as per platform.
"""
if PLATFORM == "WSL":
tool = "explorer.exe"
elif PLATFORM == "Linux":
tool = "xdg-open"
elif PLATFORM == "MacOS":
tool = "open"
else:
tool = "invalid"

return tool
Loading