Revert commits that cause failing tests on Windows CI#4679
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Trott commented Jan 13, 2016
cjihrig commented Jan 13, 2016
LGTM, pending CI of course! |
jbergstroem commented Jan 13, 2016
Great. One of the windows slaves freaked out: https://ci.nodejs.org/job/node-test-binary-windows/587/RUN_SUBSET=0,VS_VERSION=vs2015,label=win10/console |
Trott commented Jan 13, 2016
Yeah, at least it was a legit CI infra problem and not the test still failing. Anyway, I've REALLY missed the green CI results, so I'm running the whole thing again: |
cjihrig commented Jan 13, 2016
All green 💚 |
jasnell commented Jan 13, 2016
jbergstroem commented Jan 14, 2016
Lets give Ben a while before we revert this. |
mscdex added windows jasnell commented Jan 14, 2016
+1... He's been fairly ill this week. We should definitely give him a
|
rvagg commented Jan 14, 2016
@nodejs/release I've done some cherry-picking of v5.x but have excluded the series of 3 commits around these changes:
They should probably be left out until we figure this out. |
bnoordhuis commented Jan 14, 2016
LGTM. I'll look into the why of the regressions next week. |
This reverts commit 809bf5e ("change statSync to accessSync in realpathSync"). It was causing tests to fail on Windows CI. Ref: nodejs#4575 PR-URL: nodejs#4679 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
This reverts commit 7c60328. It is causing CI failures on Windows. Ref: nodejs#4575 PR-URL: nodejs#4679 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Trott commented Jan 15, 2016
Rebased against current master. One more CI run out of (hopefully excessive) caution: https://ci.nodejs.org/job/node-test-pull-request/1247/ |
cjihrig commented Jan 15, 2016
Looks like Jenkins itself had some problems. Trying again: https://ci.nodejs.org/job/node-test-pull-request/1253/ |
cjihrig commented Jan 15, 2016
@nodejs/build any idea what's going on with the Windows machines? |
Trott commented Jan 15, 2016
@cjihrig Looks like there's an issue with Jenkins ping time settings and Windows hosts going offline that @joaocgreis and @jbergstroem are (or were) discussing over in the |
cjihrig commented Jan 15, 2016
OK. Well, while I believe this will get us back to a green build, there's no rush if the build is going to fail anyway. |
jasnell commented Jan 15, 2016
😢 agree... tho that does give @bnoordhuis a bit more time to figure out what's happening here with those commits |
jbergstroem commented Jan 16, 2016
TL;DR: we're having reliability issues with windows slaves and were changing a few variables to see if it helped (which only made it worse). Reverting to best known state as of now. |
Trott commented Jan 16, 2016
Green! It's green! https://ci.nodejs.org/job/node-test-pull-request/1264/ |
This reverts commit 809bf5e ("change statSync to accessSync in realpathSync"). It was causing tests to fail on Windows CI. Ref: #4575 PR-URL: #4679 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
This reverts commit 7c60328. It is causing CI failures on Windows. Ref: #4575 PR-URL: #4679 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Trott commented Jan 16, 2016
rvagg commented Jan 18, 2016
@Trott FYI, we've consistently used the format: |
Trott commented Jan 18, 2016
(Of course, now I've compounded the problem by adding two more commits that don't conform. I should have asked specifically for review of the commit log messages. Sorry about that!) |
rvagg commented Jan 18, 2016
@Trott thanks for sorting this all btw, great job. |
rvagg commented Jan 18, 2016
|
MylesBorins commented Mar 10, 2016
I'm going to go ahead and assume we don't want this for lts. @rvagg please feel free to correct me if I'm wrong |
This reverts commit 809bf5e ("change statSync to accessSync in realpathSync"). It was causing tests to fail on Windows CI. Ref: nodejs#4575 PR-URL: nodejs#4679 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
This reverts commit 7c60328. It is causing CI failures on Windows. Ref: nodejs#4575 PR-URL: nodejs#4679 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: nodejs#13015Fixes: nodejs#12979 Refs: nodejs#4679 (comment) Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#13015Fixes: nodejs#12979 Refs: nodejs#4679 (comment) Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #13015Fixes: #12979 Refs: #4679 (comment) Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #13015Fixes: #12979 Refs: #4679 (comment) Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
8 participants
Two commits in this one:
And
Ref: #4575
R=@cjihrig