-
Notifications
You must be signed in to change notification settings - Fork 290
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
start to test node 7 and 8 #1689
Conversation
2fee170
to
43891b6
Compare
43891b6
to
968010a
Compare
I have updated this pr with the "fixed" tests and its now failing don't merge this until we have a passing build please. |
968010a
to
d0a6b18
Compare
The errors in 4 & 5 are caused by the length of the mocha timeout (fixed in #1699). I've just tried (and failed) to install adapt-cli with node v8, so this is likely a problem with the adapt-cli. There's something wrong with a URL for nodegit (returns a 403). May be a good idea to park this one for now. |
.Ill dig into it a bit when I get a chance as I was able to install adapt on node 8 with no issues but it doesn't have to go in the release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rejected due to the build errors
It looks like the current version of node-git used in the adapt cli does not support node 7 and 8 and if we bump it we will lose v5 support. I will move this pr to the v8 milestone for now and we can revisit it then. |
@@ -4,6 +4,8 @@ node_js: | |||
- "4" | |||
- "5" | |||
- "6" | |||
- "7" | |||
- "8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please could this be changed to the below to align with the CLI?
node_js:
- "lts/*"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should explicitly define what versions we test otherwise when a lts version jumps all our tests may stop working. I do think we should just change the tests to run on 6 and 8 though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still be on board with having the dynamic LTS value.
I could see it as an advantage because by the time a version enters active LTS, it is pulled down by default for new Node downloads. The tests may help in flagging possible issues as they arise.
Happy to get a general consensus. Any thoughts @taylortom / @dancgray / @lc-thomasberger ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that just setting the value as lts/*
will cause travis to use the latest active LTS, which at this point in time is v8 -- which I don't have a problem with (once we fix our v8 issues), because it'll force us to adopt current releases quicker...
There's not much documentation on this, and can't find anything that matches our specific needs.
Closing this as its deprecated by #1909 |
For #1580
Although we don't plan to move to v8 yet the tests pass and it would be good to keep them that way.