-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: use fs rimraf to refresh tmpdir #30569
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, what goes wrong on Windows when using buffer
rather than utf8
?
https://ci.nodejs.org/job/node-test-pull-request/26972/ (which uses
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. |
Windows CI rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/31958/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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>
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>
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>
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>
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>
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>
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:
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), orvcbuild test
(Windows) passes