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

Support Node v6 #273

Merged
merged 8 commits into from
Apr 28, 2016
Merged

Support Node v6 #273

merged 8 commits into from
Apr 28, 2016

Conversation

benmosher
Copy link
Member

Track progress against this PR.

Known issues:

@strawbrary
Copy link
Contributor

v6 is on Travis now so you should be good to retry.

I haven't encountered any issues on v6 besides the null path one.

@jfmengels
Copy link
Collaborator

jfmengels commented Apr 27, 2016

I re-ran the tests, and it's good to go :)

@benmosher
Copy link
Member Author

Interesting, if no tests are failing, that means there is no test that covers #272? Which seems weird, I would think at least one test would try to resolve a builtin.

So feels like we need at least one test for that breaks a la #272 before merging? (after fixing it, too)

@strawbrary
Copy link
Contributor

Hmm that is strange. On my local box I get 36 failures when running on v6.0.0. Could someone else try it and see if they get the same?

@jfmengels
Copy link
Collaborator

It passes all tests on my machine (removed node_modules, npm ied and npm test) 😕

@benmosher
Copy link
Member Author

@strawbrary are you on Windows? or OSX?
@jfmengels: same question?

I just added v6 to the Appveyor config to see how Windows takes it.

@jfmengels
Copy link
Collaborator

Linux Ubuntu here.

@strawbrary
Copy link
Contributor

OSX El Capitan

@strawbrary
Copy link
Contributor

strawbrary commented Apr 27, 2016

It seems like fileExistsWithCaseSync is never called with a null filepath under Linux, while it is under OSX and Windows since they are case insensitive. So it looks like only AppVeyor will be able to catch this issue.

@benmosher
Copy link
Member Author

Yep, that makes sense. I'm on OSX but haven't tried to run locally yet.

@Salakar
Copy link

Salakar commented Apr 28, 2016

Tested on OSX + node v6, all good.

(also resolves Webstorm's Cannot parse checkstyle XML error - for those finding this error randomly lurking up on them 😆 )

LGTM

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

Successfully merging this pull request may close these issues.

4 participants