test: use fs rimraf to refresh tmpdir by cjihrig · Pull Request #30569 · nodejs/node · GitHub

This PR is a revisit to #29235 (which I couldn't reopen since I had force pushed the branch). The goal of that PR was to use core's new rimraf implementation when refreshing the tmpdir in tests. However, that PR hit a snag that I didn't have time to look into:

=== release test-fs-readdir-ucs2 ===
Path: parallel/test-fs-readdir-ucs2
--- stderr ---
Can't clean tmpdir: /home/travis/build/nodejs/node/test/.tmp.602
Files blocking: [ '=�\u0004�' ]
/home/travis/build/nodejs/node/test/common/tmpdir.js:88
    throw e;
    ^
Error: Unable to rimraf /home/travis/build/nodejs/node/test/.tmp.602
    at rimrafSync (/home/travis/build/nodejs/node/test/common/tmpdir.js:42:11)
    at process.onexit (/home/travis/build/nodejs/node/test/common/tmpdir.js:73:5)
    at process.emit (events.js:214:15)
Command: out/Release/node /home/travis/build/nodejs/node/test/parallel/test-fs-readdir-ucs2.js

I addressed that failure in the second commit here (I copied what the rimraf in our test suite does regarding encoding). It seems to be going well in the CI: https://ci.nodejs.org/job/node-test-commit/32928/.

Another alternative to 357e233 might be to introduce an encoding option to rmdir().

cc'ing the people from #29235: @Trott @targos @gengjiawen

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

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

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

Just curious, what goes wrong on Windows when using buffer rather than utf8?

Just curious, what goes wrong on Windows when using buffer rather than utf8?

I haven't tried that, but I can. It looks like the tests version of rimraf has been using Buffers on Linux and utf8 everywhere else since 060e5f0. @jasnell do you remember why you didn't use Buffers everywhere?

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

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

https://ci.nodejs.org/job/node-test-pull-request/26972/ (which uses Buffers across all platforms) came back green. However, that was a resume build of https://ci.nodejs.org/job/node-test-pull-request/26968/, which had one failure on Windows:

13:20:29 not ok 126 parallel/test-child-process-fork-exec-path
13:20:29   ---
13:20:29   duration_ms: 0.498
13:20:29   severity: fail
13:20:29   exitcode: 1
13:20:29   stack: |-
13:20:29     Can't clean tmpdir: c:\workspace\node-test-binary-windows-2\test\.tmp.126
13:20:29     Files blocking: [ 'node-copy.exe' ]
13:20:29     
13:20:29     c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:88
13:20:29         throw e;
13:20:29         ^
13:20:29     
13:20:29     Error: EPERM: operation not permitted, unlink '\\?\c:\workspace\node-test-binary-windows-2\test\.tmp.126\node-copy.exe'
13:20:29         at unlinkSync (fs.js:1056:3)
13:20:29         at fixWinEPERMSync (internal/fs/rimraf.js:259:5)
13:20:29         at rimrafSync (internal/fs/rimraf.js:194:14)
13:20:29         at internal/fs/rimraf.js:222:9
13:20:29         at Array.forEach (<anonymous>)
13:20:29         at _rmdirSync (internal/fs/rimraf.js:219:45)
13:20:29         at fixWinEPERMSync (internal/fs/rimraf.js:257:5)
13:20:29         at rimrafSync (internal/fs/rimraf.js:194:14)
13:20:29         at Object.rmdirSync (fs.js:768:12)
13:20:29         at rimrafSync (c:\workspace\node-test-binary-windows-2\test\common\tmpdir.js:39:6) {
13:20:29       errno: -4048,
13:20:29       syscall: 'unlink',
13:20:29       code: 'EPERM',
13:20:29       path: '\\\\?\\c:\\workspace\\node-test-binary-windows-2\\test\\.tmp.126\\node-copy.exe'
13:20:29     }
13:20:29   ...

This was the same failure observed in #30074. I may try to move forward with some of the proposed changes in #30580 first to see if better synchronous retry logic helps here.

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

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

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

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

Windows CI rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/31958/

LGTM although I imagine this is blocked until #30785 or something similar lands, as otherwise Windows CI will be very unreliable?

This commit gives the synchronous version of rimraf the same linear retry logic as the asynchronous version. Prior to this commit, sync rimraf kept retrying the operation as soon as possible until maxRetries was reached. PR-URL: #30785 Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

rimraf should only retry if certain errors are encountered. Additionally, there is no point sleeping if an error occurs on the last try. PR-URL: #30785 Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

This commit gives the synchronous version of rimraf the same linear retry logic as the asynchronous version. Prior to this commit, sync rimraf kept retrying the operation as soon as possible until maxRetries was reached. PR-URL: #30785 Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

rimraf should only retry if certain errors are encountered. Additionally, there is no point sleeping if an error occurs on the last try. PR-URL: #30785 Fixes: #30580 Refs: #30569 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

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

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

EDIT: CI was yellow. Windows CI was green.

PR-URL: nodejs#30569 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This commit adds synchronous retry logic to the unlinkSync() calls in rimraf. PR-URL: nodejs#30569 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Now that the functionality is built into core, use it to refresh the test suite's tmpdir. PR-URL: nodejs#30569 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Landed in 7629fb2...4a5fb74.

PR-URL: #30569 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This commit adds synchronous retry logic to the unlinkSync() calls in rimraf. PR-URL: #30569 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Now that the functionality is built into core, use it to refresh the test suite's tmpdir. PR-URL: #30569 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>