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

Comments

Projects
None yet
2 participants
@ljharb
Copy link
Collaborator

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

This comment has been minimized.

Copy link
Contributor

rfermann commented Apr 13, 2019

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

@rfermann

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator 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

This comment has been minimized.

Copy link
Contributor

rfermann commented Apr 16, 2019

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

This comment has been minimized.

Copy link
Collaborator 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.

@ljharb ljharb closed this in #1333 Apr 16, 2019

@rfermann

This comment has been minimized.

Copy link
Contributor

rfermann commented Apr 19, 2019

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

This comment has been minimized.

Copy link
Collaborator 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
You can’t perform that action at this time.