Skip to content

Tests fail in Windows #4727

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

Closed
GeoffreyBooth opened this issue Sep 29, 2017 · 12 comments
Closed

Tests fail in Windows #4727

GeoffreyBooth opened this issue Sep 29, 2017 · 12 comments
Labels

Comments

@GeoffreyBooth
Copy link
Collaborator

See nodejs/citgm#487 and https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1003/nodes=win-vs2015/testReport/(root)/citgm/coffeescript_v2_0_1/

3 of the 4 failed tests seem to stem from #3946 @cosmicexplorer.

@cosmicexplorer
Copy link
Contributor

I'll look at these asap.

@GeoffreyBooth
Copy link
Collaborator Author

Thanks much! It might be we need to add a few more return if isWindows() lines at the start of tests that don’t make sense to run in Windows (assuming -- stuff is irrelevant to Windows).

@cosmicexplorer
Copy link
Contributor

cosmicexplorer commented Sep 29, 2017

It looks possible that line endings are getting mistreated and could be getting transformed into something weird (the octal escape?), spinning up my windows box now.

@cosmicexplorer
Copy link
Contributor

Ok, I'm pretty sure the way windows paths are templated into assertErrorFormat() is incorrect because of backslashes and escaping rules. Specifically, in test "#2849: compilation error in a require()d file" in test/error_messages.coffee, on my windows box, the backslashes simply go away (I think e.g. C:\Users\... turns into a \U, which evaluates to just a U). That may explain why this error is occurring in the linked test, but that would require the backslash sequence to be interpreted differently than I just described, so not sure if that's the whole story. I'll see how far just doubling backslashes in the require('...') templates goes.

@cosmicexplorer
Copy link
Contributor

Doubling backslashes in the require '...' call fixes that error, but there are a few more test failures to get through.

@GeoffreyBooth
Copy link
Collaborator Author

You’re welcome to push a branch to https://github.com/GeoffreyBooth/coffeescript, if you’d like to collaborate there. Thanks for taking this on!

@cosmicexplorer
Copy link
Contributor

Ok, I had a branch but polluted it before I set core.autocrlf so might take a bit to recover (but not long, I'll push asap). I can definitely solve failure 1, but the rest have been annoying.

@cosmicexplorer
Copy link
Contributor

Hey, I have a branch up here which fixes 1 of the 4 failures. There are some notes on the second, haven't looked at others.

@GeoffreyBooth
Copy link
Collaborator Author

It looks like you’ve dived quite deep on the argument parsing . . . before you go any farther, are those tests even applicable in Windows?

@GeoffreyBooth
Copy link
Collaborator Author

@cosmicexplorer I pushed some commits on your branch, I got the tests passing in my (virtual) Windows environment. How do they work for you?

@GeoffreyBooth
Copy link
Collaborator Author

GeoffreyBooth commented Sep 30, 2017

FYI the secret to getting spawnSync working in Windows was the shell: true option: nodejs/node#3675 (comment)

@cosmicexplorer
Copy link
Contributor

@GeoffreyBooth thanks for figuring out spawnSync -- I don't think the child_process docs are clear on the need for {shell: true} at all.

I think the only tests that don't apply to Windows are just the ones checking #! parsing. Something like the windows subsystem for linux would be able to use #! lines, but I'm pretty sure process.platform() returns 'linux' in that case, so those tests will run whenever they are applicable, I think.

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

No branches or pull requests

2 participants