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

try to fix windows tests #1419

Merged
merged 12 commits into from
Feb 22, 2021
Merged

try to fix windows tests #1419

merged 12 commits into from
Feb 22, 2021

Conversation

JakubKoralewski
Copy link
Contributor

@JakubKoralewski JakubKoralewski commented Feb 20, 2021

Fixes #1417

  1. *.debug files were somehow not converted by git to LF file endings so I added those to .gitattributes
    I had to remove all files and git reset for the changes to show
  2. spack had very linux specific path handling which I attempted to fix but not all tests are passing:
    https://gist.github.com/JakubKoralewski/f6368a554073f14240184b40fefde888

I'd appreciate some help, as I don't know what to do with the above test failures (fixture::pass::issue_1139\example_1 doesnt seem to be path related, but fixture::pass::pr_1105\example_7 seems to be :confused:)

@CLAassistant
Copy link

CLAassistant commented Feb 20, 2021

CLA assistant check
All committers have signed the CLA.

@kdy1
Copy link
Member

kdy1 commented Feb 20, 2021

Can you please change this into a draft PR so I can know when the task is done?

@JakubKoralewski JakubKoralewski marked this pull request as draft February 20, 2021 14:48
@JakubKoralewski
Copy link
Contributor Author

Can you please change this into a draft PR so I can know when the task is done?

Sure.

Could you take a look at the fixture::pass::issue_1139\example_1 test output? I can try to debug fixture::pass::pr_1105\example_7 as it seems to be something wrong with windows paths, but i have no idea why the former would fail, seems to be unrelated to this PR. Could you confirm?

@kdy1
Copy link
Member

kdy1 commented Feb 20, 2021

@JakubKoralewski I think it's a bug of swc and it will be fixed by #1382

@JakubKoralewski JakubKoralewski marked this pull request as ready for review February 22, 2021 08:24
@JakubKoralewski
Copy link
Contributor Author

On further inspection it looks like in the fixture::pass::pr_1105\example_7 test the created windows path:

<    url: "$DIR/tests/pass/pr-1105/example-7/input/f.js",
>    url: "\\\\?\\D:\\projects\\swc\\swc\\spack\\tests\\pass\\pr-1105\\example-7\\input\\f.js",

is actually correct, it's just that the way the tests compare the outputs does not take into account Windows paths. I'm not sure if the verbatim \\?\ syntax will be interpreted correctly though.

@kdy1 What do you think? My idea is that it's an issue with tests not with the current implementation, so I believe it is ready for your review. Considering there is no CI for Windows I don't think modifying the tests logic for a single incorrect test is worth it, I'd add it as a bug and move on.

@kdy1
Copy link
Member

kdy1 commented Feb 22, 2021

My idea is that it's an issue with tests not with the current implementation, so I believe it is ready for your review

I agree. I'll take a look after taking some rest.

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM.
I'm a bit worried about the nightly feature, though.

@@ -1,4 1,5 @@
#![cfg_attr(test, feature(test))]
#![feature(or_patterns)]
Copy link
Member

Choose a reason for hiding this comment

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

Is this really required?

Copy link
Member

Choose a reason for hiding this comment

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

swc uses nightly for a while because of some rustfmt options, but it does not rely on nightly features from non-test codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not required, I just noticed that nightly was used anyway so I wanted to use some terser syntax, i'm not familiar with the consequences of using nightly features. looking at rust-lang/rust/issues/54883 looks to be out in next stable

should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

It's okay if it's going to be stabilized. Thanks!

@JakubKoralewski JakubKoralewski changed the title [WIP] try to fix windows tests try to fix windows tests Feb 22, 2021
@kdy1 kdy1 merged commit 59bd00d into swc-project:master Feb 22, 2021
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

tests not passing on windows
3 participants