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

Revert commits that cause failing tests on Windows CI #4679

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 13, 2016

Two commits in this one:

Revert "module: optimize js and json file i/o"

    This reverts commit 7c603280024de60329d5da283fb8433420bc6716.

    It is causing CI failures on Windows.

And

   Revert "fs: change statSync to accessSync in realpathSync"

    This reverts commit 809bf5e38cdb1fe304da9d413f07e9d7e66ce8f9.

    It was causing tests to fail on Windows CI.

Ref: #4575

R=@cjihrig

@Trott
Copy link
Member Author

Trott commented Jan 13, 2016

@cjihrig
Copy link
Contributor

cjihrig commented Jan 13, 2016

LGTM, pending CI of course!

@jbergstroem
Copy link
Member

@Trott
Copy link
Member Author

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:

https://ci.nodejs.org/job/node-test-pull-request/1223/

@cjihrig
Copy link
Contributor

cjihrig commented Jan 13, 2016

All green 💚

@jasnell
Copy link
Member

jasnell commented Jan 13, 2016

@bnoordhuis

@jbergstroem
Copy link
Member

Lets give Ben a while before we revert this.

@mscdex mscdex added windows Issues and PRs related to the Windows platform. fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. labels Jan 14, 2016
@jasnell
Copy link
Member

jasnell commented Jan 14, 2016

+1... He's been fairly ill this week. We should definitely give him a
chance to review
On Jan 13, 2016 4:34 PM, "Johan Bergström" notifications@github.com wrote:

Lets give Ben a while before we revert this.


Reply to this email directly or view it on GitHub
#4679 (comment).

@rvagg
Copy link
Member

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
Copy link
Member

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 <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This reverts commit 7c60328.

It is causing CI failures on Windows.

Ref: nodejs#4575
PR-URL: nodejs#4679
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Trott
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

cjihrig commented Jan 15, 2016

@nodejs/build any idea what's going on with the Windows machines?

@Trott
Copy link
Member Author

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 #node-build IRC channel. I'm not sure, but it's possible that the fact that Windows tests have been borked for a day or two may have masked the problem.

@cjihrig
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

Trott commented Jan 16, 2016

Trott added a commit that referenced this pull request Jan 16, 2016
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 <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Trott added a commit that referenced this pull request Jan 16, 2016
This reverts commit 7c60328.

It is causing CI failures on Windows.

Ref: #4575
PR-URL: #4679
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Trott
Copy link
Member Author

Trott commented Jan 16, 2016

Landed in 3441a41 and 19f7008

@Trott Trott closed this Jan 16, 2016
@rvagg
Copy link
Member

rvagg commented Jan 18, 2016

@Trott FYI, we've consistently used the format: Revert "original commit msg" as reversion messages, so even the subsystem prefix goes into the quotes. The tooling we have recognises this format too.

@Trott
Copy link
Member Author

Trott commented Jan 18, 2016

Sorry about using the wrong format. Although in my defense...:

(EDIT: Probably really just the first one as I'm not even sure the others are strict reverts the way these are.)

@Trott
Copy link
Member Author

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
Copy link
Member

rvagg commented Jan 18, 2016

@Trott thanks for sorting this all btw, great job.

@rvagg
Copy link
Member

rvagg commented Jan 18, 2016

v5.x is now up to date with master including the two commits from #4575 that didn't get reverted by this PR. All green: https://ci.nodejs.org/job/node-test-commit/1802/ 👍

@MylesBorins
Copy link
Contributor

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

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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 <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
This reverts commit 7c60328.

It is causing CI failures on Windows.

Ref: nodejs#4575
PR-URL: nodejs#4679
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
gibfahn added a commit to gibfahn/node that referenced this pull request May 16, 2017
PR-URL: nodejs#13015
Fixes: nodejs#12979
Refs: nodejs#4679 (comment)
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
PR-URL: nodejs#13015
Fixes: nodejs#12979
Refs: nodejs#4679 (comment)
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 22, 2017
PR-URL: #13015
Fixes: #12979
Refs: #4679 (comment)
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #13015
Fixes: #12979
Refs: #4679 (comment)
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bzoz bzoz mentioned this pull request Feb 9, 2018
2 tasks
@Trott Trott deleted the revert-14 branch January 13, 2022 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants