test: try to reduce flakes on Windows by refack · Pull Request #28035 · nodejs/node

refack · 2019-06-02T21:02:48Z

test: always suffix tmpdir
This makes temp dir names consistent whether we run in stand-alone mode,
via test.py in single process, or in multi-process.
test: shell out to rmdir first on Windows
cmd's rmdir is hardened to deal with Windows edge cases, like
lingering processes, indexing, and AV checks. So we give it a try first.
test: regression test tmpdir

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

refack · 2019-06-02T21:04:07Z

/CC @nodejs/testing

mscdex · 2019-06-02T21:12:49Z

Shouldn't the subsystem prefix here just be test: ?

richardlau · 2019-06-02T21:23:13Z

Please could you add a brief sentence or two in the commit message as to why?

refack · 2019-06-02T21:23:32Z

Shouldn't the subsystem prefix here just be test: ?

Fixed.

refack · 2019-06-02T21:33:28Z

Please could you add a brief sentence or two in the commit message as to why?

Wrote some words. Didn't have anything too enlightening 🤷‍♂.
WRT to refresh() nothing is extremely robust, it's just a extra heuristic to overcome some recent failures seen in CI:

test/async-hooks/test-pipeconnectwrap.js

Trott

LGTM if CI is green and once info about opts is added to the tmpdir.refresh() entry in test/common/README.md.

Trott · 2019-06-03T01:49:56Z

@nodejs/platform-windows

test/common/tmpdir.js

BridgeAR

LGTM but it seems like on Windows it will now always try both ways, even though the folder might already be deleted by rmdir.

BridgeAR · 2019-06-04T11:37:28Z

test/common/tmpdir.js

If everything passes, should this not return?

Not totally sure why, but it's not trivial https://ci.nodejs.org/job/node-test-binary-windows-2/1275/

sam-github · 2019-06-05T16:41:07Z

Is this intended to fix failures like below?


c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:27
      throw e;
      ^

Error: ENOTEMPTY: directory not empty, rmdir 'c:\workspace\node-test-binary-windows-2\test\.tmp.6'
    at Object.rmdirSync (fs.js:693:3)
    at rmdirSync (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:48:10)
    at rimrafSync (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:18:7)
    at Object.refresh (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:66:3)
    at Object.<anonymous> (c:\workspace\node-test-binary-windows-2\test\parallel\test-child-process-spawnsync-args.js:18:8)
    at Module._compile (internal/modules/cjs/loader.js:777:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:788:10)
    at Module.load (internal/modules/cjs/loader.js:640:32)
    at Function.Module._load (internal/modules/cjs/loader.js:555:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:840:10) {
  errno: -4051,
  syscall: 'rmdir',
  code: 'ENOTEMPTY',
  path: 'c:\\workspace\\node-test-binary-windows-2\\test\\.tmp.6'
}

https://ci.nodejs.org/job/node-test-binary-windows-2/1244/COMPILED_BY=vs2017,RUNNER=win2008r2-vs2017,RUN_SUBSET=0/testReport/junit/(root)/test/parallel_test_child_process_spawnsync_args/

refack · 2019-06-05T16:49:55Z

Is this intended to fix failures like below?

Exactly.

refack · 2019-06-05T17:22:39Z

@Trott added doc

@BridgeAR ~~I wanted this to be best-effort with minimal change to current semantics.~~
had to rebase and fix some more stuff, so I added a return.

I'm going to land this to reduce flakes in CI, and I'll follow up with teaks.

test/common/README.md

Trott

LGTM with minor doc update to the function signature

nodejs-github-bot · 2019-06-06T23:23:27Z

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

cmd's `rmdir` is hardened to deal with Windows edge cases, like lingering processes, indexing, and AV checks. So we give it a try first. * Added `opts = { spawn = true }` to opt-out of spawning * test-pipeconnectwrap.js - spawning messes up async_hooks state PR-URL: nodejs#28035 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

This makes temp dir names consistent whether we run in stand-alone mode, via `test.py` in single process, or in multi-process. PR-URL: nodejs#28035 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

PR-URL: nodejs#28035 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Also try to make more traceable. PR-URL: nodejs#28035 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

refack · 2019-06-06T23:51:35Z

landed in:
59f666c
9032ab4
2f8cf5e
65a5f7b

cmd's `rmdir` is hardened to deal with Windows edge cases, like lingering processes, indexing, and AV checks. So we give it a try first. * Added `opts = { spawn = true }` to opt-out of spawning * test-pipeconnectwrap.js - spawning messes up async_hooks state PR-URL: #28035 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

This makes temp dir names consistent whether we run in stand-alone mode, via `test.py` in single process, or in multi-process. PR-URL: #28035 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

PR-URL: #28035 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Also try to make more traceable. PR-URL: #28035 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

refack requested a review from Trott June 2, 2019 21:02

refack self-assigned this Jun 2, 2019

This comment has been minimized.

Sign in to view

refack force-pushed the testutils-rmdir-first-on-windows branch from 297daf1 to 9c57dea Compare June 2, 2019 21:22

refack force-pushed the testutils-rmdir-first-on-windows branch from 9c57dea to b557192 Compare June 2, 2019 21:28

nodejs deleted a comment from nodejs-github-bot Jun 2, 2019

This comment has been minimized.

Sign in to view

refack force-pushed the testutils-rmdir-first-on-windows branch from c869ded to ccf9100 Compare June 2, 2019 23:02

This comment has been minimized.

Sign in to view

mscdex changed the title ~~test,lib: try rmdir first on windows~~ test: try rmdir first on windows Jun 3, 2019

Trott reviewed Jun 3, 2019

View reviewed changes

test/async-hooks/test-pipeconnectwrap.js Outdated Show resolved Hide resolved

refack force-pushed the testutils-rmdir-first-on-windows branch 2 times, most recently from 55fd037 to a17d5ba Compare June 3, 2019 01:18

nodejs deleted a comment from nodejs-github-bot Jun 3, 2019

This comment has been minimized.

Sign in to view

refack added the test Issues and PRs related to the tests. label Jun 3, 2019

Trott approved these changes Jun 3, 2019

View reviewed changes

lpinca reviewed Jun 3, 2019

View reviewed changes

test/common/tmpdir.js Outdated Show resolved Hide resolved

refack commented Jun 3, 2019

View reviewed changes

test/common/tmpdir.js Outdated Show resolved Hide resolved

BridgeAR approved these changes Jun 4, 2019

View reviewed changes

benjamingr approved these changes Jun 4, 2019

View reviewed changes

sam-github approved these changes Jun 5, 2019

View reviewed changes

refack force-pushed the testutils-rmdir-first-on-windows branch from 91c51b2 to bf5b8e8 Compare June 5, 2019 17:21

This comment has been minimized.

Sign in to view

refack force-pushed the testutils-rmdir-first-on-windows branch from bf5b8e8 to 4059eba Compare June 5, 2019 22:06

nodejs deleted a comment from nodejs-github-bot Jun 5, 2019

refack force-pushed the testutils-rmdir-first-on-windows branch from 4059eba to e531a68 Compare June 5, 2019 22:14

This comment has been minimized.

Sign in to view

refack changed the title ~~test: try rmdir first on windows~~ test: try to reduce flakes on Windows Jun 5, 2019

refack force-pushed the testutils-rmdir-first-on-windows branch from e531a68 to 803a946 Compare June 5, 2019 23:06

This comment has been minimized.

Sign in to view

Trott reviewed Jun 6, 2019

View reviewed changes

test/common/README.md Outdated Show resolved Hide resolved

Trott approved these changes Jun 6, 2019

View reviewed changes

refack force-pushed the testutils-rmdir-first-on-windows branch from 7fce3e9 to 60133a1 Compare June 6, 2019 23:21

refack mentioned this pull request Jun 6, 2019

Investigate flaky parallel/test-worker-debug on Windows #28106

Closed

refack added 4 commits June 6, 2019 19:38

test: regression test tmpdir

2f8cf5e

PR-URL: nodejs#28035 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

refack force-pushed the testutils-rmdir-first-on-windows branch from 60133a1 to 65a5f7b Compare June 6, 2019 23:39

refack merged commit 65a5f7b into nodejs:master Jun 6, 2019

refack deleted the testutils-rmdir-first-on-windows branch June 6, 2019 23:51

refack removed their assignment Jun 7, 2019

refack mentioned this pull request Jun 10, 2019

Windows CI failures #28152

Closed

BridgeAR mentioned this pull request Jun 17, 2019

v12.5.0 proposal #28268

Merged

Uh oh!

Conversation

refack commented Jun 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

This comment has been minimized.

refack commented Jun 2, 2019

Uh oh!

mscdex commented Jun 2, 2019

Uh oh!

richardlau commented Jun 2, 2019

Uh oh!

refack commented Jun 2, 2019

Uh oh!

refack commented Jun 2, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

This comment has been minimized.

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Jun 3, 2019

Uh oh!

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

refack Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

sam-github commented Jun 5, 2019

Uh oh!

refack commented Jun 5, 2019

Uh oh!

refack commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jun 6, 2019

Uh oh!

refack commented Jun 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

refack commented Jun 2, 2019 •

edited

Loading

refack commented Jun 5, 2019 •

edited

Loading