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

Conversation

rsashank
Copy link
Member

@rsashank rsashank commented Jun 5, 2024

What does this PR do, and why?

Addressed issues with file path separators \\ and GUI return codes specific to WSL environments.

Fixes the failing of these tests on WSL environments, these have been failing for a while now but I just got used to ignoring them, decided to look into them today and fix. 😓

image

Outstanding aspect(s)

  • [ ]

External discussion & connections

  • Discussed in #zulip-terminal in WSL specific test fails
  • Fully fixes #
  • Partially fixes issue #
  • Builds upon previous unmerged work in PR #
  • Is a follow-up to work in PR #
  • Requires merge of PR #
  • Merge will enable work on #

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

@zulipbot zulipbot added the size: S [Automatic label added by zulipbot] label Jun 5, 2024
Comment on lines 543 to 545
if PLATFORM == "WSL":
media_path = media_path.replace("/", "\\")

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I can import PLATFORM into tests, but this seemed like the only way to address the difference in file paths in WSL.

Copy link
Member Author

Choose a reason for hiding this comment

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

To further clarify, based on a discussion on CZO, the issue stems from the normalized_path function used in the download_media method. The media_path assertion doesn't account for the WSL-specific normalized path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For a conditional in a test, the cleaner way to handle this would normally be to parametrize the test (over PLATFORM), though note you'd want a third path for the expected_return_value or similar output parameter, since it doesn't correspond to url or media_path in every case (though the latter may not be representative - see below re confirming).

The alternative to this conditional/parametrizing solution, which may also be clearer in the long term, is related to how (as we confirmed in the channel) the path returned is the path used for opening the temporary file, not the path used for saving it. So it would be clearer to refactor the code before fixing the tests, such that the normalized_file_path occurs outside of this function, and this function would be platform-independent to test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the normalized_file_path to be called in process_media 👍
Clear explanation, thanks!

Comment on lines 608 to 641
@pytest.mark.parametrize(
"returncode, error",
[
(0, []),
(1 if PLATFORM == "WSL" else 0, []),
(
1,
0 if PLATFORM == "WSL" else 1,
[
" The tool ",
("footer_contrast", "xdg-open"),
" did not run successfully" ". Exited with ",
("footer_contrast", "1"),
("footer_contrast", "0" if PLATFORM == "WSL" else "1"),
],
),
],
Copy link
Member Author

@rsashank rsashank Jun 5, 2024

Choose a reason for hiding this comment

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

This test didn't consider the different return codes in WSL, as given in platform_code.py line 92.

def successful_GUI_return_code() -> int:  # noqa: N802 (allow upper case)
    """
    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
    if PLATFORM == "WSL":
        return 1

    return 0

Copy link
Member Author

Choose a reason for hiding this comment

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

explorer.exe seems to always return a non-zero exit code, regardless of whether the task is successful.

More information could be found on this issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your initial change may not be relevant now, so skip this paragraph to focus on possible solutions. However, note that (a) we should avoid repeating ourselves in this way, and (b) the way you have it written makes the test itself perform differently when run on WSL and elsewhere, which would be fragile - ie. suddenly break if not tested regularly (we don't test on WSL in CI). As long as the behavior and code can actually be run and tested from a different platform, we should prefer to mock PLATFORM and ensure the outcome matches what we expect.

Based on the behavior you confirmed in the channel (and as per your comment above), it seems this is a bug, so it'd be worth not just fixing the test here.

One simple approach would be to revise the code to only output a message on running the tool, since that's the common factor we can actually determine across platforms.

However, to allow retaining some of the existing behavior, we can refactor the function you quoted, such that it returns some value, depending upon a passed-in return code. This would move the comparison, and hence these explicit error codes, into a platform-dependent location. In turn, the test you're aiming to fix could easily mock the new function, and would be independent of platform.

Depending upon the approach taken, we would lose the ability to test/show the exact error code in the output, but I'm not convinced this adds much to the experience, unless debugging.

To retain the existing behavior, different return values from that new function could signify eg. 'definitely opened', 'failed to open', 'trying to open', to cover the case across platforms.

Depending how far you wish to go with this refactor, a lot of the logic in open_media could move into the new platform-dependent function, including possibly the tools-per-platform themselves. However, that could be a series of refactors after the first simplification.

Copy link
Member Author

Choose a reason for hiding this comment

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

we can refactor the function you quoted, such that it returns some value, depending upon a passed-in return code. This would move the comparison, and hence these explicit error codes, into a platform-dependent location.

Implemented this approach, thanks for the suggestion! Also will look into refactoring some open_media logic to a platform-specific location in another PR.

@rsashank rsashank changed the title bugfix: tests: Fix tests failing on WSL systems. Fix tests failing on WSL systems. Jun 6, 2024
@Niloth-p
Copy link
Collaborator

Niloth-p commented Jun 7, 2024

Nice work! Thanks for addressing those.
The motivation and code LGTM.
As discussed, this is not something that I can run and test though, as I don't use WSL.

@neiljp
Copy link
Collaborator

neiljp commented Jun 9, 2024

@rsashank I started a review, but will leave that pending until I hear back to confirm the implementation details I noted in #zulip-terminal.

@rsashank rsashank added area: tests PR needs review PR requires feedback to proceed platform: WSL labels Jun 10, 2024
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@rsashank Thanks for finding these problems and working on solutions - we don't have WSL in CI (GitHub PR tests), so some of these issues are not always obvious, and not everything can be tested in CI in any case.

Following the discussion in the channel, I've adjusted some comments, though the same general gist of my suggestions remains the same, ie. that refactoring slightly first would be preferable, and may allow further subsequent refactoring which would make everything a lot easier to read.

Due to limited resources for testing on each platform, in general its better if we can isolate as much into platform-code as we can, during times while people are available to test it manually. That applies to this code in helper, as well as anywhere else we explicitly rely upon PLATFORM. That allows the platform-dependent code to be manually-tested well, and the interface between that and the application to be as clean as possible, as well as tests like these to be less fragile :)

Comment on lines 608 to 641
@pytest.mark.parametrize(
"returncode, error",
[
(0, []),
(1 if PLATFORM == "WSL" else 0, []),
(
1,
0 if PLATFORM == "WSL" else 1,
[
" The tool ",
("footer_contrast", "xdg-open"),
" did not run successfully" ". Exited with ",
("footer_contrast", "1"),
("footer_contrast", "0" if PLATFORM == "WSL" else "1"),
],
),
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your initial change may not be relevant now, so skip this paragraph to focus on possible solutions. However, note that (a) we should avoid repeating ourselves in this way, and (b) the way you have it written makes the test itself perform differently when run on WSL and elsewhere, which would be fragile - ie. suddenly break if not tested regularly (we don't test on WSL in CI). As long as the behavior and code can actually be run and tested from a different platform, we should prefer to mock PLATFORM and ensure the outcome matches what we expect.

Based on the behavior you confirmed in the channel (and as per your comment above), it seems this is a bug, so it'd be worth not just fixing the test here.

One simple approach would be to revise the code to only output a message on running the tool, since that's the common factor we can actually determine across platforms.

However, to allow retaining some of the existing behavior, we can refactor the function you quoted, such that it returns some value, depending upon a passed-in return code. This would move the comparison, and hence these explicit error codes, into a platform-dependent location. In turn, the test you're aiming to fix could easily mock the new function, and would be independent of platform.

Depending upon the approach taken, we would lose the ability to test/show the exact error code in the output, but I'm not convinced this adds much to the experience, unless debugging.

To retain the existing behavior, different return values from that new function could signify eg. 'definitely opened', 'failed to open', 'trying to open', to cover the case across platforms.

Depending how far you wish to go with this refactor, a lot of the logic in open_media could move into the new platform-dependent function, including possibly the tools-per-platform themselves. However, that could be a series of refactors after the first simplification.

Comment on lines 543 to 545
if PLATFORM == "WSL":
media_path = media_path.replace("/", "\\")

Copy link
Collaborator

Choose a reason for hiding this comment

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

For a conditional in a test, the cleaner way to handle this would normally be to parametrize the test (over PLATFORM), though note you'd want a third path for the expected_return_value or similar output parameter, since it doesn't correspond to url or media_path in every case (though the latter may not be representative - see below re confirming).

The alternative to this conditional/parametrizing solution, which may also be clearer in the long term, is related to how (as we confirmed in the channel) the path returned is the path used for opening the temporary file, not the path used for saving it. So it would be clearer to refactor the code before fixing the tests, such that the normalized_file_path occurs outside of this function, and this function would be platform-independent to test.

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback bug Something isn't working area: refactoring and removed PR needs review PR requires feedback to proceed labels Jun 13, 2024
@zulipbot
Copy link
Member

Hello @zulip/server-refactoring members, this pull request was labeled with the "area: refactoring" label, so you may want to check it out!

@rsashank rsashank force-pushed the wsl_helpertestfix branch from 5a821d0 to 424d8e8 Compare June 27, 2024 23:47
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: S [Automatic label added by zulipbot] labels Jun 28, 2024
@rsashank rsashank added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 28, 2024
@neiljp neiljp force-pushed the wsl_helpertestfix branch from 424d8e8 to 39cc322 Compare July 5, 2024 05:05
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@rsashank I hoped to merge this fully but ended up leaving a few comments aimed primarily at the second commit - I've picked off the first commit and merged it already, after making the change I indicated in the comment re which item to patch.

Hopefully that first commit reduces the failing tests you're finding regularly, and the other commit will be quick to follow :)

Comment on lines 575 to 576
mocker.patch(MODULE + ".PLATFORM", platform)
mocker.patch("zulipterminal.platform_code.PLATFORM", platform)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that these two lines are patching

  • the PLATFORM imported into helper
  • the PLATFORM in the original module

So, at the very least this is duplication. When patching, we want to adjust the name that's used, and if you only use the version you added (and remove the previous one), then it won't run the test - it doesn't patch properly.

=> The existing line is sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

While testing locally in my WSL environment, the platform detection incorrectly identifies the platform as WSL when either one of the patches is missing. This causes the tests to enter the if PLATFORM == 'WSL' condition and change the path.

So, I had to mock the PLATFORM value in both the platform_code and helper.

I'm not entirely sure why this is happening, but patching both seems to ensure the consistency for all platforms that are being tested.
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for this feedback! This seems likely since we still use PLATFORM in both locations for this :/

This appears to be the only outstanding change in this commit after a rebase, since I merged the first commit previously, so the commit text is currently inaccurate at that point. It'd also be useful to group and comment the two lines, if they appear to be necessary to pass the tests, as well as update the commit.

This is a 'test bugfix' commit, specific to WSL, essentially?

Copy link
Member Author

Choose a reason for hiding this comment

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

Grouped them together and added a comment, also changed commit message.

Yes, it's a test bugfix specific to WSL.

Comment on lines 98 to 102
# NOTE: WSL 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" if exit_status == 1 else "failure"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This claims to be able to return and handle failure in the code, but the comment says it cannot. Then we don't test this code. If we're always going to return success, we should do that and then only test for it with WSL - or be able to return both success and failure, and test for both.

Is it only explorer.exe under WSL that always succeeds?

If that's the case, this is quite fragile since this seems related to knowing that we're using it for explorer.exe only, but in another part of the code entirely. Having this change will be good for now, but we should ensure that we note this in each location that's affected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for pointing this out. I've removed the mention of "failure" handling.

The linked issue specifically refers to explorer.exe, and since validate_GUI_exit_status only deals with explorer.exe, I believe it should be fine to document here. What do you say?

zulipterminal/platform_code.py Outdated Show resolved Hide resolved
tests/platform_code/test_platform_code.py Outdated Show resolved Hide resolved
media_path: str = "/tmp/zt-somerandomtext-image.png",
) -> None:
mocked_run = mocker.patch(MODULE + ".subprocess.run")
mocker.patch("zulipterminal.platform_code.PLATFORM", platform)
Copy link
Collaborator

Choose a reason for hiding this comment

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

See my comment on the other commit - you should be able to tell if this is working properly, by whether running the test shows the code sees the patched value properly. Something as simple as print(PLATFORM) in the code will do the job - give it a try!

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't seem to work on my local WSL environment - trying to figure out why :)

I need to have both patched.

("footer_contrast", "1"),
],
),
# NOTE: WSL always returns a non-zero exit code (1), so we do not test for it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is more re explorer.exe, right, rather than WSL?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, explorer.exe (WSL). Works?

tests/helper/test_helper.py Outdated Show resolved Hide resolved
("WSL", 1, "success"),
("Linux", 1, "failure"),
("MacOS", 1, "failure"),
# NOTE: WSL always returns a non-zero exit code (1), so we do not test for it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As per another comment, we should test for this, even if it results in the same response - we're testing the function, not the system behavior, in this test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the failure handling entirely since it's considered an abnormality that might be addressed in the future. explorer.exe seems to be intended to return 1 for failure and 0 for success, so returning 0 for a failure didn't make sense.

This could be updated if WSL fixes this, what do you think?

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Jul 5, 2024
@rsashank rsashank force-pushed the wsl_helpertestfix branch 3 times, most recently from 6853166 to 2f08b0b Compare September 19, 2024 20:46
@rsashank rsashank added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Sep 19, 2024
@rsashank
Copy link
Member Author

Updated this PR, thanks for the feedback @neiljp!

Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@rsashank I'd like to get this merged to firm up the WSL behavior - while someone is testing it :)

Regards to the platform isolation, placing the remaining PLATFORM-dependent functionality in platform_code.py would be preferable and ease testing overall, and could be another refactor to explore in addition to the followup I mentioned in a comment.

Comment on lines 575 to 576
mocker.patch(MODULE + ".PLATFORM", platform)
mocker.patch("zulipterminal.platform_code.PLATFORM", platform)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for this feedback! This seems likely since we still use PLATFORM in both locations for this :/

This appears to be the only outstanding change in this commit after a rebase, since I merged the first commit previously, so the commit text is currently inaccurate at that point. It'd also be useful to group and comment the two lines, if they appear to be necessary to pass the tests, as well as update the commit.

This is a 'test bugfix' commit, specific to WSL, essentially?

Comment on lines +628 to +635
" The tool ",
("footer_contrast", "open"),
" did not run successfully" ". Exited with ",
("footer_contrast", "1"),
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?

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Sep 23, 2024
@neiljp neiljp added this to the Next Release milestone Sep 23, 2024
@rsashank rsashank force-pushed the wsl_helpertestfix branch 2 times, most recently from a237c02 to 9220bf9 Compare September 30, 2024 17:08
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Sep 30, 2024
Addressed issues with file path separators `\\` specific to WSL
environments.
The process_media_tool function determines and returns the media tool
used based on the OS.
@rsashank rsashank force-pushed the wsl_helpertestfix branch 2 times, most recently from efa23bb to 532734c Compare September 30, 2024 17:11
@rsashank
Copy link
Member Author

@neiljp I've updated this PR—thank you!

I added another commit to move the platform-based conditions in the helper function to platform_code.py.

Can you elaborate on this a bit?

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.

@rsashank rsashank added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Sep 30, 2024
WSL always returns a non-zero exit code. The `successful_GUI_return_code()`
function has been refactored to directly compare exit statuses and return
"success" or "failure" accordingly.

Fix open_media test to validate both the tool and the error code across
various platforms.

Refactor process_media to shift platform dependent code to platform_code.py.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: refactoring area: tests bug Something isn't working platform: WSL PR needs review PR requires feedback to proceed size: XL [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants