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.

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

lib: resolve the issue of not adhering to the specified buffersize #55896

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cu8code
Copy link

@cu8code cu8code commented Nov 17, 2024

ref: #55764

We create a queueHandler, and in every iteration we execute the handlers in the queueHandler until we get a non-null result.

@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 17, 2024
@cu8code
Copy link
Author

cu8code commented Nov 17, 2024

@Ethan-Arrowood, please take a look at the PR when you have a chance. It should be ready for merge unless some any issues is found!

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

Attention: Patch coverage is 80.48780% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.53%. Comparing base (556f1ae) to head (dba2d27).
Report is 119 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/fs/dir.js 80.48% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55896      +/-   ##
==========================================
+ Coverage   88.00%   88.53%   +0.53%     
==========================================
  Files         656      657       +1     
  Lines      188999   190413    +1414     
  Branches    35989    36554     +565     
==========================================
+ Hits       166331   168589    +2258     
+ Misses      15848    15007     -841     
+ Partials     6820     6817       -3     
Files with missing lines Coverage Δ
lib/internal/fs/dir.js 95.05% <80.48%> (-2.17%) ⬇️

... and 117 files with indirect coverage changes

@Ethan-Arrowood Ethan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2024
@Ethan-Arrowood
Copy link
Contributor

Looking good. Have you tested it locally by running the relevant fs readdir and opendir recursive test files?

I'm not really sure if this needs a unique test as you are more just improving an existing solution.

I requested a CI run now and we'll see how this does.

Thanks for the contribution!

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

@cu8code
Copy link
Author

cu8code commented Nov 29, 2024

Looking good. Have you tested it locally by running the relevant fs readdir and opendir recursive test files?

Yes, I've tested it. It should be good unless I missed something.

I'm not really sure if this needs a unique test as you are more just improving an existing solution.

Agreed. I don't think we need a new test since, as you mentioned, nothing fundamentally new was added.

@nodejs-github-bot
Copy link
Collaborator

@Ethan-Arrowood Ethan-Arrowood added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 30, 2024
@Ethan-Arrowood
Copy link
Contributor

Looks good to me, and CI is passing! Thank you, lets gets this merged.

@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 30, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/55896
✔  Done loading data for nodejs/node/pull/55896
----------------------------------- PR info ------------------------------------
Title      lib: resolve the issue of not adhering to the specified buffersize (#55896)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     cu8code:55723 -> nodejs:main
Labels     fs, needs-ci
Commits    1
 - lib: resolve the issue of not adhering to the specified buffer size
Committers 1
 - cu8code <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/55896
Refs: https://github.com/nodejs/node/issues/55764
Reviewed-By: Ethan Arrowood <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55896
Refs: https://github.com/nodejs/node/issues/55764
Reviewed-By: Ethan Arrowood <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 17 Nov 2024 17:27:03 GMT
   ✔  Approvals: 1
   ✔  - Ethan Arrowood (@Ethan-Arrowood): https://github.com/nodejs/node/pull/55896#pullrequestreview-2470703507
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-11-30T01:14:02Z: https://ci.nodejs.org/job/node-test-pull-request/63800/
- Querying data for job/node-test-pull-request/63800/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/12093348479

@Ethan-Arrowood
Copy link
Contributor

Hm i thought the CI run succeeded. I'll see if a retry helps.

@Ethan-Arrowood Ethan-Arrowood added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 30, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2024
@nodejs-github-bot
Copy link
Collaborator

We create a `queueHandler`, and in every iteration we execute
the handlers in the `queueHandler` until we get a non-null result.
@Ethan-Arrowood Ethan-Arrowood added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 9, 2024
@Ethan-Arrowood Ethan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 9, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/55896
✔  Done loading data for nodejs/node/pull/55896
----------------------------------- PR info ------------------------------------
Title      lib: resolve the issue of not adhering to the specified buffersize (#55896)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     cu8code:55723 -> nodejs:main
Labels     fs, needs-ci
Commits    1
 - lib: resolve the issue of not adhering to the specified buffer size
Committers 1
 - cu8code <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/55896
Refs: https://github.com/nodejs/node/issues/55764
Reviewed-By: Ethan Arrowood <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55896
Refs: https://github.com/nodejs/node/issues/55764
Reviewed-By: Ethan Arrowood <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 17 Nov 2024 17:27:03 GMT
   ✔  Approvals: 1
   ✔  - Ethan Arrowood (@Ethan-Arrowood): https://github.com/nodejs/node/pull/55896#pullrequestreview-2489049523
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-12-09T15:00:05Z: https://ci.nodejs.org/job/node-test-pull-request/63954/
- Querying data for job/node-test-pull-request/63954/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/12241884962

@cu8code cu8code closed this Dec 11, 2024
@Ethan-Arrowood
Copy link
Contributor

Why did you close this? I think only flaky tests were failing.

@cu8code cu8code reopened this Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-failed An error occurred while landing this pull request using GitHub Actions. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants