-
Notifications
You must be signed in to change notification settings - Fork 30k
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
fs: move rmSync
implementation to c++
#53617
fs: move rmSync
implementation to c++
#53617
Conversation
c541155
to
3d2975d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
3d2975d
to
d820ac1
Compare
d820ac1
to
8d02638
Compare
8d02638
to
2538925
Compare
Benchmark results (since CI is locked down):
|
Failed to start CI⚠ Something was pushed to the Pull Request branch since the last approving review. ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/9784060111 |
5c1f6ed
to
8c8e277
Compare
cc @nodejs/cpp-reviewers can you review? |
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 with green tests
Landed in 7168295 |
Marking it as dont-land because of #53962 (comment) |
@richardlau @aduh95 any idea why the tests are flaky? |
@anonrig the flakiness of the test (and its root cause) is beside the point, the fact that it break something is our CI means it likely going to break our users and therefore should not land on release lines. In an ideal world, when the CI reports the same test failing 6 times in the same PR but is passing in others, that PR would not land until the flakiness is addressed. |
Yes, I agree. I'm trying to understand how this change caused this regression. |
Is this safe to land on v22.x with cf2bce6 ? |
I need to write some benchmarks before making this pull-request ready.
Benchmark on all
fs
commands: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1575/