-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
revert on Travis: pin Node.js 10.x version to 10.0.0 #1350
Conversation
Looks like AppVeyor has not enabled Node.js 10.2.0 yet:
Let's keep this pull request around until AppVeyor adds Node.js 10.2.0 support, at which point we can land it. |
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.
LGTM once Appveyor resolves their issue
Cross-posting from appveyor/ci#2276:
|
AppVeyor has updated to include support 10.3.0 |
eef6a68
to
eae3b33
Compare
Hmm, looks like there is some other bug on Windows, the test run freezes inside our test suite. See https://ci.appveyor.com/project/bajtos/loopback-next/build/1.0.5656-master/job/of9c4nq2dwhhp96g
|
I was able to reproduce the problem locally on both Node.js 10.3.0 and 10.4.0. Will investigate further. |
After a day of debugging, my conclusion is that this is a regression in Node.js 10.2.0. The tracking bug: nodejs/node#21210 |
1789057
to
c283122
Compare
I am no longer able to reproduce the AppVeyor problem locally. Let's move this patch forward by landing only the Travis CI part. I have also added a commit to make the tests more robust, see c283122 |
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.
Do we know how to track when appveyor is good to be unpinned from node 10.0.0? If there isn't, we should periodically check to see if it can be reverted to latest. Otherwise, this PR LGTM
I don't know about any good solution, other than open a PR to unpin 10.0.0 (as I did here) and then periodically trigger new AppVeyor run to see if the problem has been resolved. I'll open a follow-up PR. |
c283122
to
3bd83eb
Compare
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.
Are the timeout changes still necessary? Was it part of testing AppVeyor changes or is it intentional? I'm not opposed to them -- just curious since I've rarely run into a build failing due to a timeout (have seen it but rare).
The one marked for 15 minutes is a bit concerning ...
Can land without further review from me.
The changes are not strictly necessary, but intentional. In the current master, the timeout was set for infinity via |
This partially reverts commit a36e257 and configures Travis CI to use the latest Node.js 10.x version, since the problem we were experiencing in 10.1.0 has been fixed in 10.2.0. AppVeyor stays on 10.0.0 because the version 10.2.0 introduced a subtle timing issue that causes CLI tests to hang, see nodejs/node#21210
3bd83eb
to
ce81840
Compare
The follow-up pull request for AppVeyor: #1431 |
This partially reverts commit a36e257,
as it is no longer needed on Travis CI - the new Node.js version 10.2.0 has
fixed the problem we were experiencing on Node.js 10.1.0.
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated