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: pass correct path to DirentFromStats during glob #55071

Merged
merged 5 commits into from
Oct 22, 2024
Merged

fs: pass correct path to DirentFromStats during glob #55071

merged 5 commits into from
Oct 22, 2024

Conversation

RedYetiDev
Copy link
Member

Fixes #55060

Passes the dirname as parentPath, rather than the full file path.

@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 Sep 22, 2024
@RedYetiDev RedYetiDev mentioned this pull request Sep 22, 2024
Closed
@codecov Codecov
Copy link

codecov bot commented Sep 23, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.39%. Comparing base (89a2f56) to head (4528fd6).
Report is 158 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #55071      +/-   ##
==========================================
- Coverage   88.39%   88.39%   -0.01%     
==========================================
  Files         652      652              
  Lines      186565   186565              
  Branches    36046    36037       -9     
==========================================
- Hits       164916   164915       -1     
- Misses      14908    14910       +2     
+ Partials     6741     6740       -1
Files with missing lines Coverage Δ
lib/internal/fs/glob.js 91.76% <100.00%> (ø)

... and 16 files with indirect coverage changes

@RedYetiDev
Copy link
Member Author

CC @nodejs/fs

@VoltrexKeyva VoltrexKeyva added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 24, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev RedYetiDev added the wip Issues and PRs that are still a work in progress. label Sep 24, 2024
@RedYetiDev
Copy link
Member Author

I need to account for \\ (vs /) on windows.

@RedYetiDev RedYetiDev removed the wip Issues and PRs that are still a work in progress. label Sep 25, 2024
@RedYetiDev RedYetiDev added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev RedYetiDev added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 29, 2024
@jakecastelli jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev
Copy link
Member Author

Looks like there are relevant failures I may need to take a look at:

        TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received null
            at isAbsolute (node:path:462:5)
            at normalizePath (file:///C:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-glob.mjs:342:34)
            at Array.map (<anonymous>)
            at TestContext.<anonymous> (file:///C:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-glob.mjs:368:75)
            at Test.runInAsyncScope (node:async_hooks:211:14)
            at Test.run (node:internal/test_runner/test:930:25)
            at Suite.processPendingSubtests (node:internal/test_runner/test:629:18)
            at Test.postRun (node:internal/test_runner/test:1026:19)
            at Test.run (node:internal/test_runner/test:969:12)
            at async Suite.processPendingSubtests (node:internal/test_runner/test:629:7) {
          code: 'ERR_INVALID_ARG_TYPE'
        }

@RedYetiDev
Copy link
Member Author

Rebased to fix coverage issues, can someone start a CI?

@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev
Copy link
Member Author

Can this land?

@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev
Copy link
Member Author

RedYetiDev commented Oct 10, 2024 •
edited
Loading

Hey, could someone resume the CI? If this lands today, it can be included in v23.0.0, which would be nice

This probably won't make it in time for the release,

@nodejs-github-bot
Copy link
Collaborator

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

@RedYetiDev
Copy link
Member Author

Hey, can someone restart the failed builds so this fix can land? Thanks!

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 22, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 22, 2024
@nodejs-github-bot nodejs-github-bot merged commit 7ae73b9 into nodejs:main Oct 22, 2024
55 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 7ae73b9

aduh95 pushed a commit that referenced this pull request Oct 23, 2024
@RedYetiDev @aduh95
fs: pass correct path to DirentFromStats during glob
45f960c 
PR-URL: #55071
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
@aduh95 aduh95 mentioned this pull request Oct 24, 2024
Merged
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@RedYetiDev @louwers
fs: pass correct path to DirentFromStats during glob
a908d60 
PR-URL: nodejs#55071
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@RedYetiDev @tpoisseau
fs: pass correct path to DirentFromStats during glob
e58eff2 
PR-URL: nodejs#55071
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@ZYSzys ZYSzys ZYSzys approved these changes

@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood approved these changes

@MoLow MoLow MoLow approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

The parentPath returned by fsPromises.glob is incorrect in some cases.
9 participants
@RedYetiDev @nodejs-github-bot @jasnell @MoLow @aduh95 @Ethan-Arrowood @ZYSzys @jakecastelli @VoltrexKeyva