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

CI Windows tests fix #111

Merged
merged 15 commits into from
Sep 25, 2023

Conversation

SnorlaxAssist
Copy link
Contributor

This PR solves a sub issue of #107.

I’ve made some changes to the process/spawn.luau test file, and the rust process/options.rs to improve support for Windows. Default shell process for windows has been changed to powershell instead of /bin/sh. These changes ensure that the tests can run smoothly across different operating systems by using commands that have equivalent or similar functionality in Unix and Windows based OS.

@SnorlaxAssist SnorlaxAssist marked this pull request as ready for review September 20, 2023 22:44
@SnorlaxAssist SnorlaxAssist changed the title CI Windows tests::process_spawn fix CI Windows tests::process_spawn & tests::net_serve_requests fix Sep 20, 2023
@SnorlaxAssist
Copy link
Contributor Author

Made test changes that fixes tests::net_serve_requests errors on Windows.

@CompeyDev
Copy link
Contributor

CompeyDev commented Sep 22, 2023

This looks great! Out of curiosity, what was causing net tests to fail (i.e., the server not stopping)?

EDIT: Looked at the changes, and that answered my question.

EDIT 2: I believe a lot of the windows testing logic can be simplified, as most of it is due to lack of GNU command line equivalents on Windows.

Here is proposed solution, try this on your local machine, and lmk if that fixes the serde compression issues.

@SnorlaxAssist
Copy link
Contributor Author

SnorlaxAssist commented Sep 24, 2023

Serde passes all test on my machine, I am using windows 11. Although in a virtual machine serde doesn't pass the test, I'm not exactly sure why but I'm more leaning to what @filiptibell said about files being checked out using CRLF instead of LF.

Edit: I can confirm CRLF was the issue, a fix was made in the test file. Although the issue could also be fixed within fs.readFile, I'm not too sure how my PC didn't pick up on this, likely due to some installs I have or maybe even WSL.

@SnorlaxAssist SnorlaxAssist changed the title CI Windows tests::process_spawn & tests::net_serve_requests fix CI Windows tests fix Sep 24, 2023
@CompeyDev
Copy link
Contributor

Edit: I can confirm CRLF was the issue, a fix was made in the test file. Although the issue could also be fixed within fs.readFile, I'm not too sure how my PC didn't pick up on this, likely due to some installs I have or maybe even WSL.

What exactly is the issue in fs.readFile? I'd PR this if it was feasible.

@CompeyDev
Copy link
Contributor

CompeyDev commented Sep 25, 2023

Btw, 1cf7e9b is not really making use of the MinGW installation, you'd also need to change the target being compiled for.

https://github.com/filiptibell/lune/blob/1cf7e9b7fd01cae1184374f6a0c2c44ca982f01d/.github/workflows/ci.yaml#L39

This should be x86_64-pc-windows-gnu, and should build successfully if MinGW is properly setup in the install step.

Copy link
Collaborator

@filiptibell filiptibell left a comment

Choose a reason for hiding this comment

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

Hi and thank you for the PR! I left a few minor comments.

RE: Using mingw and sticking with unix-y commands on Windows, I've been thinking on it a bit, and I think the main thing is someone downloading the repo for the first time on Windows should have all the tests pass without any additional configuration on their end. We should try to use system defaults like Powershell when possible in tests. We could have gone with cmd.exe too, like Node does, but that feels quite outdated.

tests/process/spawn.luau Outdated Show resolved Hide resolved
tests/process/spawn.luau Outdated Show resolved Hide resolved
tests/serde/compression/files.luau Outdated Show resolved Hide resolved
@filiptibell filiptibell mentioned this pull request Sep 25, 2023
5 tasks
@SnorlaxAssist
Copy link
Contributor Author

Hopefully this is the last commit, there is one slight issue maybe that could be a problem in the future, where MacOS process spawn test fails with Spawning a sleep process should take a reasonable amount of time it seems to run slightly longer just by a little than the SLEEP_DURATION * 1.5.

A possible fix can be to increase the 1.5x to 2x or 2.5x.

Copy link
Collaborator

@filiptibell filiptibell left a comment

Choose a reason for hiding this comment

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

LGTM! The timing issue on macOS is fine to leave out of this PR since it is addressing Windows issues after all. Thanks for getting this all fixed. Sorry about the commit noise - GitHub didn't want to do a merge commit so I had to rebase instead. It will all get squashed anyway.

SnorlaxAssist and others added 15 commits September 25, 2023 14:01
Using windows powershell as the shell executor.
Made changes to the commands and params to support windows equivalents.
Execution Tolerance time only for windows
Fixed args being different for linux and macOs for sleep test
Added and changed a few comments.
Connection refusal fix for Windows
Command Execution Tolarance is now only dedicated for windows.
Adjusted yield time with Command Execution Tolarance
Included MinGW installation from lune-org#106 (comment)
- This reverts commit 1cf7e9b.
- Added test serde file to convert CRLF to LF on windows.
explicit "powershell" instead of `true`
Large static windows time tolerance
- Reverts the CRLF to LF conversion change from a commit in 1f9f683.
- Increased the timeout time for windows from 15 -> 25
@filiptibell filiptibell force-pushed the windows-process-ci-test-fix branch from 6e039b1 to cb369f8 Compare September 25, 2023 19:01
@filiptibell filiptibell merged commit 83ac971 into lune-org:main Sep 25, 2023
3 of 4 checks passed
@SnorlaxAssist SnorlaxAssist deleted the windows-process-ci-test-fix branch September 25, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants