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.

Sign up for GitHub

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

Jump to bottom

fs: make mutating options in readdir() not affect results #56057

Closed
wants to merge 2 commits into from
Closed

fs: make mutating options in readdir() not affect results #56057

wants to merge 2 commits into from
+23 −0

Conversation

LiviaMedeiros
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros commented Nov 28, 2024 •
edited
Loading

The callback version is implemented in sync way (see #56006), so its test should pass without adjustments.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Nov 28, 2024
@codecov Codecov
Copy link

codecov bot commented Nov 28, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.00%. Comparing base (9c046ea) to head (aa7c1ef).
Report is 52 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #56057   +/-   ##
=======================================
  Coverage   88.00%   88.00%           
=======================================
  Files         656      656           
  Lines      189131   189138    +7     
  Branches    36009    36001    -8     
=======================================
+ Hits       166439   166455   +16     
+ Misses      15853    15849    -4     
+ Partials     6839     6834    -5
Files with missing lines Coverage Δ
lib/fs.js 93.27% <100.00%> (+<0.01%) ⬆️
lib/internal/fs/promises.js 97.56% <100.00%> (+<0.01%) ⬆️

... and 28 files with indirect coverage changes

@LiviaMedeiros LiviaMedeiros added the blocked PRs that are blocked by other issues or PRs. label Nov 29, 2024
@LiviaMedeiros
Copy link
Contributor Author

To be safe, Callback API test blocked until #56041 lands (the test can be moved to that PR).

@LiviaMedeiros LiviaMedeiros force-pushed the fs-readdir-async-options-mutation branch from 5dcdd50 to d09a59e Compare December 7, 2024 09:53
@LiviaMedeiros LiviaMedeiros force-pushed the fs-readdir-async-options-mutation branch from d09a59e to 779031f Compare December 7, 2024 09:56
@LiviaMedeiros LiviaMedeiros added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. request-ci Add this label to start a Jenkins CI on a PR. and removed blocked PRs that are blocked by other issues or PRs. labels Dec 7, 2024
@LiviaMedeiros LiviaMedeiros force-pushed the fs-readdir-async-options-mutation branch from ff33adc to aa7c1ef Compare December 7, 2024 10:03
@LiviaMedeiros
Copy link
Contributor Author

Rebased on main on top of 53356c3, now it patches both Callback and Promises APIs.

In callback version, copy is made only for recursive mode.

@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Dec 7, 2024
@github-actions GitHub Actions

This comment was marked as resolved.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@LiviaMedeiros LiviaMedeiros removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Dec 7, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 12, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 12, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 743eacc...9109965

nodejs-github-bot pushed a commit that referenced this pull request Dec 12, 2024
@LiviaMedeiros @nodejs-github-bot
fs: make mutating options in Promises readdir() not affect results
b5e25aa 
PR-URL: #56057
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Dec 12, 2024
@LiviaMedeiros @nodejs-github-bot
fs: make mutating options in Callback readdir() not affect results
9109965 
PR-URL: #56057
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Dec 13, 2024
@LiviaMedeiros @targos
fs: make mutating options in Promises readdir() not affect results
2df8257 
PR-URL: #56057
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Dec 13, 2024
@LiviaMedeiros @targos
fs: make mutating options in Callback readdir() not affect results
c21a9e0 
PR-URL: #56057
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@github-actions github-actions bot mentioned this pull request Dec 18, 2024
Merged
aduh95 pushed a commit that referenced this pull request Dec 18, 2024
@LiviaMedeiros @aduh95
fs: make mutating options in Promises readdir() not affect results
8d485f1 
PR-URL: #56057
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Dec 18, 2024
@LiviaMedeiros @aduh95
fs: make mutating options in Callback readdir() not affect results
4cc0493 
PR-URL: #56057
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mertcanaltin mertcanaltin mertcanaltin approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@juanarbol juanarbol juanarbol approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

Assignees
No one assigned
Labels
commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@LiviaMedeiros @nodejs-github-bot @jasnell @lpinca @BridgeAR @juanarbol @RafaelGSS @mertcanaltin