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

test: try to reduce flakes on Windows #28035

Merged
merged 4 commits into from
Jun 6, 2019

Conversation

refack
Copy link
Contributor

@refack refack commented Jun 2, 2019

  • 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 refack requested a review from Trott June 2, 2019 21:02
@refack refack self-assigned this Jun 2, 2019
@nodejs-github-bot

This comment has been minimized.

@refack
Copy link
Contributor Author

refack commented Jun 2, 2019

/CC @nodejs/testing

@mscdex
Copy link
Contributor

mscdex commented Jun 2, 2019

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

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

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

@refack
Copy link
Contributor Author

refack commented Jun 2, 2019

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

Fixed.

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

refack commented Jun 2, 2019

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:

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

This comment has been minimized.

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

This comment has been minimized.

@mscdex mscdex changed the title test,lib: try rmdir first on windows test: try rmdir first on windows Jun 3, 2019
@refack 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 nodejs deleted a comment from nodejs-github-bot Jun 3, 2019
@nodejs-github-bot

This comment has been minimized.

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

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Trott
Copy link
Member

Trott commented Jun 3, 2019

@nodejs/platform-windows

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// On Windows first try to delegate rmdir to a shell.
if (process.platform === 'win32' && st.isDirectory()) {
try {
execSync(`rmdir /q /s ${pathname}`, { timout: 1000 });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If everything passes, should this not return?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sam-github
Copy link
Contributor

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

refack commented Jun 5, 2019

Is this intended to fix failures like below?

Exactly.

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

refack commented Jun 5, 2019

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

@nodejs-github-bot

This comment has been minimized.

@refack refack force-pushed the testutils-rmdir-first-on-windows branch from bf5b8e8 to 4059eba Compare June 5, 2019 22:06
@nodejs nodejs deleted a comment from nodejs-github-bot Jun 5, 2019
@refack refack force-pushed the testutils-rmdir-first-on-windows branch from 4059eba to e531a68 Compare June 5, 2019 22:14
@nodejs-github-bot

This comment has been minimized.

@refack refack changed the title test: try rmdir first on windows test: try to reduce flakes on Windows Jun 5, 2019
@refack refack force-pushed the testutils-rmdir-first-on-windows branch from e531a68 to 803a946 Compare June 5, 2019 23:06
@nodejs-github-bot

This comment has been minimized.

test/common/README.md Outdated Show resolved Hide resolved
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor doc update to the function signature

@refack refack force-pushed the testutils-rmdir-first-on-windows branch from 7fce3e9 to 60133a1 Compare June 6, 2019 23:21
@nodejs-github-bot
Copy link
Collaborator

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 refack force-pushed the testutils-rmdir-first-on-windows branch from 60133a1 to 65a5f7b Compare June 6, 2019 23:39
@refack refack merged commit 65a5f7b into nodejs:master Jun 6, 2019
@refack
Copy link
Contributor Author

refack commented Jun 6, 2019

landed in:
59f666c
9032ab4
2f8cf5e
65a5f7b

@refack refack deleted the testutils-rmdir-first-on-windows branch June 6, 2019 23:51
@refack refack removed their assignment Jun 7, 2019
@refack refack mentioned this pull request Jun 10, 2019
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
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>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
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>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
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>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
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>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants