-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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: execute shell directly for refresh() #55051
base: main
Are you sure you want to change the base?
Conversation
`require("fs").rmSync("${escapedPath}", { maxRetries: 3, recursive: true, force: true });`, | ||
], | ||
); | ||
if (isUnix) { |
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.
WDYT about adding a version for windows, rmdir [path] /s /q
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55051 +/- ##
=======================================
Coverage 88.23% 88.23%
=======================================
Files 652 652
Lines 183912 183912
Branches 35865 35866 +1
=======================================
+ Hits 162272 162277 +5
+ Misses 14915 14914 -1
+ Partials 6725 6721 -4 |
cc @nodejs/cpp-reviewers @nodejs/tsc would you mind reviewing? |
What if we just called the shell directly to avoid the cost of booting a Node.js instance?
cc @nodejs/build any concerns?