Revert commits that cause failing tests on Windows CI by Trott · Pull Request #4679 · nodejs/node

Trott · 2016-01-13T22:11:52Z

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 · 2016-01-13T22:13:42Z

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

cjihrig · 2016-01-13T22:21:18Z

LGTM, pending CI of course!

jbergstroem · 2016-01-13T22:30:02Z

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 · 2016-01-13T22:43:28Z

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 · 2016-01-13T23:04:01Z

All green 💚

jasnell · 2016-01-13T23:57:35Z

@bnoordhuis

jbergstroem · 2016-01-14T00:34:27Z

Lets give Ben a while before we revert this.

jasnell · 2016-01-14T00:59:42Z

+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 · 2016-01-14T11:22:33Z

@nodejs/release I've done some cherry-picking of v5.x but have excluded the series of 3 commits around these changes:

[7c60328] - module: optimize js and json file i/o (Ben Noordhuis) module: cache stat() results more aggressively #4575
[83f8d98] - module: cache stat() results more aggressively (Ben Noordhuis) module: cache stat() results more aggressively #4575
[809bf5e] - fs: change statSync to accessSync in realpathSync (Ben Noordhuis) module: cache stat() results more aggressively #4575

They should probably be left out until we figure this out.

bnoordhuis · 2016-01-14T22:07:58Z

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 · 2016-01-15T06:51:06Z

Rebased against current master. One more CI run out of (hopefully excessive) caution: https://ci.nodejs.org/job/node-test-pull-request/1247/

cjihrig · 2016-01-15T13:20:37Z

Looks like Jenkins itself had some problems. Trying again: https://ci.nodejs.org/job/node-test-pull-request/1253/

cjihrig · 2016-01-15T15:01:12Z

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

Trott · 2016-01-15T15:02:32Z

@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 · 2016-01-15T16:05:20Z

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 · 2016-01-15T16:48:50Z

😢 agree... tho that does give @bnoordhuis a bit more time to figure out what's happening here with those commits

jbergstroem · 2016-01-16T00:46:32Z

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 · 2016-01-16T04:35:53Z

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

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 · 2016-01-16T04:37:25Z

Landed in 3441a41 and 19f7008

rvagg · 2016-01-18T04:26:40Z

@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 · 2016-01-18T04:45:42Z

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 · 2016-01-18T04:48:39Z

(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 · 2016-01-18T04:54:58Z

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

rvagg · 2016-01-18T05:17:20Z

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 · 2016-03-10T21:02:15Z

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 <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>

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>

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>

Trott mentioned this pull request Jan 13, 2016

module: cache stat() results more aggressively #4575

Closed

jbergstroem mentioned this pull request Jan 13, 2016

tools: update eslint to v1.x #2286

Closed

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

This was referenced Jan 14, 2016

cluster: fix race condition setting suicide prop #4349

Closed

test: enable parallel testing in test-ci #4476

Closed

Node.js Testing WG Meeting 2016-Jan-15 nodejs/testing#1

Closed

joaocgreis mentioned this pull request Jan 14, 2016

build: Add option to select environment for msbuild (vc2015 or vc2013) #4645

Closed

Trott added 2 commits January 14, 2016 22:46

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

9c4c63b

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 force-pushed the revert-14 branch from 4b5b850 to 9c4c63b Compare January 15, 2016 06:49

Trott closed this Jan 16, 2016

rvagg added the dont-land-on-v5.x label Jan 18, 2016

MylesBorins mentioned this pull request Mar 10, 2016

Audit commits not found on v4.x #5647

Closed

MylesBorins added the dont-land-on-v4.x label Mar 10, 2016

This was referenced May 11, 2017

Agree on commit reverting strategy #12979

Closed

fs: Revert throw on invalid callbacks #12976

Merged

gibfahn mentioned this pull request May 13, 2017

doc: document method for reverting commits #13015

Merged

2 tasks

bnoordhuis mentioned this pull request Nov 20, 2017

module: speed up package.json parsing #15767

Closed

bnoordhuis mentioned this pull request Feb 1, 2018

fs.existsSync returns wrong value on windows if there are no permissions on the given file #17921

Closed

bzoz mentioned this pull request Feb 9, 2018

fs: use fs.access in fs.exists #18618

Closed

2 tasks

Trott deleted the revert-14 branch January 13, 2022 22:31

Uh oh!

Conversation

Trott commented Jan 13, 2016

Uh oh!

Trott commented Jan 13, 2016

Uh oh!

cjihrig commented Jan 13, 2016

Uh oh!

jbergstroem commented Jan 13, 2016

Uh oh!

Trott commented Jan 13, 2016

Uh oh!

cjihrig commented Jan 13, 2016

Uh oh!

jasnell commented Jan 13, 2016

Uh oh!

jbergstroem commented Jan 14, 2016

Uh oh!

jasnell commented Jan 14, 2016

Uh oh!

rvagg commented Jan 14, 2016

Uh oh!

bnoordhuis commented Jan 14, 2016

Uh oh!

Trott commented Jan 15, 2016

Uh oh!

cjihrig commented Jan 15, 2016

Uh oh!

cjihrig commented Jan 15, 2016

Uh oh!

Trott commented Jan 15, 2016

Uh oh!

cjihrig commented Jan 15, 2016

Uh oh!

jasnell commented Jan 15, 2016

Uh oh!

jbergstroem commented Jan 16, 2016

Uh oh!

Trott commented Jan 16, 2016

Uh oh!

Trott commented Jan 16, 2016

Uh oh!

rvagg commented Jan 18, 2016

Uh oh!

Trott commented Jan 18, 2016

Uh oh!

Trott commented Jan 18, 2016

Uh oh!

rvagg commented Jan 18, 2016

Uh oh!

rvagg commented Jan 18, 2016

Uh oh!

MylesBorins commented Mar 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants