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

Fixes e2e tests using checks with UNIX paths. #3147

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

esdrubal
Copy link
Contributor

@esdrubal esdrubal commented Oct 26, 2022

On Windows the output error and warning paths use \ as path separator.
e2e tests have checks with hardcoded UNIX paths that use / as path separator character.

Solution was to replace \ by / on compile_and_capture_output.

@tritao tritao added the testing General testing label Oct 26, 2022
@tritao tritao requested a review from a team October 26, 2022 00:34
@mitchmindtree
Copy link
Contributor

Thanks for the PR @esdrubal!

I'm not sure we should start adding patches like this until we actually decide to properly support Windows within Sway, Forc, etc too. #1526 shares a rough path ahead for this for anyone interested in having a go at native Windows support.

@tritao
Copy link
Contributor

tritao commented Oct 26, 2022

Thanks for the PR @esdrubal!

I'm not sure we should start adding patches like this until we actually decide to properly support Windows within Sway, Forc, etc too. #1526 shares a rough path ahead for this for anyone interested in having a go at native Windows support.

I think this PR is basically the first step towards your Check all path handling task in the issue.

I have no idea about other Windows issues, but this fix (or a variation) will always be needed. On Windows, we want to display Windows-style paths when displaying diagnostics, yet the tests are written with Unix-style paths. We don't really want to change all tests FileCheck directives, so the only two options I can see are either:

  1. convert the paths in check directives to Windows-style when parsing them
  2. convert the compiler output paths to Unix-style before matching the checks

This PR implements 2) which I think is a good enough solution for the issue.

There are currently other Windows issues which still need addressing (compiling in debug is comptetely broken with a stack overflow issue for instance) but IMHO we shouldn't not merge this one because of that, since it brings us one step closer to being able to setup a Windows CI.

On Windows the output error and warning paths use '\' as path separator.
e2e tests have checks with hardcoded UNIX paths that use '/' as path
separator character.

Solution was to replace '\' by '/' on `compile_and_capture_output`.
Copy link
Contributor

@otrho otrho left a comment

Choose a reason for hiding this comment

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

I agree. A tiny change like this is helpful in the short term and can't really hurt.

Unless we explicitly don't want Sway on Windows... 😉

@tritao tritao enabled auto-merge (squash) October 27, 2022 09:48
@tritao tritao merged commit 166f72b into FuelLabs:master Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing General testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants