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

no-unused-modules appveyor tests failing #1317

Closed
ljharb opened this issue Apr 11, 2019 · 7 comments · Fixed by #1333
Closed

no-unused-modules appveyor tests failing #1317

ljharb opened this issue Apr 11, 2019 · 7 comments · Fixed by #1333

Comments

@ljharb
Copy link
Member

ljharb commented Apr 11, 2019

cc @rfermann

There's some appveyor tests failing due to #1142: https://ci.appveyor.com/project/benmosher/eslint-plugin-import/builds/23488849

What's particularly confusing is that a) they pass on travis and locally; b) one of the test cases is listed, identically, as both valid and invalid, so it shouldn't pass at all; there shouldn't be any filesystem impact, which means it shouldn't be failing on windows.

We need to get this fixed ASAP.

This was referenced Apr 13, 2019
@rfermann
Copy link
Contributor

I will look into this the next 2-3 days.

@rfermann
Copy link
Contributor

rfermann commented Apr 16, 2019

I was able to identify the problem for the failing tests. In line 465 I'm using astNode.source.value to resolve the full file path of an import statement.

On macOS, the values in this field are looking like this: /Users/someUser/eslint-plugin-import/tests/files/no-unused-modules/file-added-4.js.js. With this value it is possible to resolve the full file path.
On Windows 10, the values are somehow quirky:
Screenshot

instead of C:\eslint-plugin-import\tests\files\no-unused-modules\file-added-4.js.js, which fails resolving.

Printing astNode.source to the console, the value looks different:
Screenshot_1.
My assumption is, that the backslashes within the path are treated as escape characters. Is there any way to pass the strings without letting the escape characters taking effect?

@ljharb
Copy link
Member Author

ljharb commented Apr 16, 2019

I wonder if that reflects an issue in the code setting the value then - not sure whether that’s ours or eslint itself.

@rfermann
Copy link
Contributor

I could imagine that this is just the normal behavior on Windows systems, as all these systems are using the backslash instead the slash in the file path. So the value itself is a valid file path on Windows systems. Only JS doesn't like the backslashes in it when passing around the value.

A solution for this problem could be to use the raw field instead of the value field, as this field is correctly escaped.
I already tested this change on both Windows 10 and macOS and it is mostly working. Surprisingly another test is failing now which passed with the old code 😅

@ljharb
Copy link
Member Author

ljharb commented Apr 16, 2019

@rfermann thanks for the fix; it'd be great tho to try to track down why there's a difference and fix that - possibly in eslint itself.

@rfermann
Copy link
Contributor

I digged into that a bit. It seems, that this behavior is coming from the parser, not eslint itself. Babel-eslint is giving back the path in the same format on macOS and Windows (with slashes instead of backslashes), while espree is giving back different formats.

I haven't yet found the code causing this behavior, but I will stay on it.

@ljharb
Copy link
Member Author

ljharb commented Apr 19, 2019

Thanks, that’s great - filing an espree issue once you can repro it would be awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants