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

test: fix inconsistency write size in test-fs-readfile-tostring-fail #51141

Merged
merged 2 commits into from
Dec 25, 2023
Merged

test: fix inconsistency write size in test-fs-readfile-tostring-fail #51141

merged 2 commits into from
Dec 25, 2023

Conversation

pluris
Copy link
Contributor

@pluris pluris commented Dec 13, 2023 •
edited
Loading

In this test, more space is being used than the space being checked.
It's actually using more capacity in stream.write() than is written to kStringMaxLength.
There are 201 buffers used in the for loop.

Even if this code is fixed, I'm not sure if the unstable tests are resolved.

node/test/pummel/test-fs-readfile-tostring-fail.js

Lines 18 to 20 in 1b60054

if (!tmpdir.hasEnoughSpace(kStringMaxLength)) {
common.skip(`Not enough space in ${tmpdir.path}`);
}

node/test/pummel/test-fs-readfile-tostring-fail.js

Lines 29 to 36 in 1b60054

const size = kStringMaxLength / 200;
const a = Buffer.alloc(size, 'a');
let expectedSize = 0;
for (let i = 0; i < 201; i++) {
stream.write(a, (err) => { assert.ifError(err); });
expectedSize += a.length;
}

Refs: #51133

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Dec 13, 2023
@pluris pluris changed the title test: fix inconsistency in write size for `test-fs-readfile-tostring-… test: fix inconsistency in write size for test-fs-readfile-tostring-fail Dec 13, 2023
@pluris pluris force-pushed the fix/test_fs_readfile branch from d90fc7d to adecf4d Compare December 13, 2023 06:06
@pluris pluris changed the title test: fix inconsistency in write size for test-fs-readfile-tostring-fail test: fix inconsistency write size for test-fs-readfile-tostring-fail Dec 13, 2023
@pluris pluris force-pushed the fix/test_fs_readfile branch from adecf4d to 7c5a47c Compare December 13, 2023 06:12
@pluris pluris changed the title test: fix inconsistency write size for test-fs-readfile-tostring-fail test: fix inconsistency write size in test-fs-readfile-tostring-fail Dec 13, 2023
@pluris pluris closed this Dec 13, 2023
@pluris pluris reopened this Dec 13, 2023
@pluris pluris force-pushed the fix/test_fs_readfile branch 2 times, most recently from 1499cb9 to be0fb37 Compare December 13, 2023 08:26
lpinca
lpinca approved these changes Dec 13, 2023
View reviewed changes
test/pummel/test-fs-readfile-tostring-fail.js Outdated
@@ -9,13 +9,15 @@ const assert = require('assert');
const fs = require('fs');
const cp = require('child_process');
const kStringMaxLength = require('buffer').constants.MAX_STRING_LENGTH;
const size = kStringMaxLength / 200;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const size = kStringMaxLength / 200;
const size = Math.floor(kStringMaxLength / 200);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review. I fixed it. 😄

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 14, 2023
@nodejs-github-bot
Copy link
Collaborator

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

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

Commit Queue failed
- Loading data for nodejs/node/pull/51141
✔  Done loading data for nodejs/node/pull/51141
----------------------------------- PR info ------------------------------------
Title      test: fix inconsistency write size in `test-fs-readfile-tostring-fail` (#51141)
Author     Jungku Lee  (@pluris)
Branch     pluris:fix/test_fs_readfile -> nodejs:main
Labels     test, needs-ci
Commits    2
 - test: fix inconsistency write size in `test-fs-readfile-tostring-fail`
 - fixup! test: fix inconsistency write size in test-fs-readfile-tostrin…
Committers 1
 - pluris 
PR-URL: https://github.com/nodejs/node/pull/51141
Refs: https://github.com/nodejs/node/issues/51133
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51141
Refs: https://github.com/nodejs/node/issues/51133
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - fixup! test: fix inconsistency write size in test-fs-readfile-tostrin…
   ℹ  This PR was created on Wed, 13 Dec 2023 06:03:46 GMT
   ✔  Approvals: 1
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/51141#pullrequestreview-1780419297
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-12-14T20:38:30Z: https://ci.nodejs.org/job/node-test-pull-request/56294/
- Querying data for job/node-test-pull-request/56294/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/7316742704

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 24, 2023
@deokjinkim deokjinkim added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 25, 2023
@deokjinkim deokjinkim added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 25, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 25, 2023
@nodejs-github-bot nodejs-github-bot merged commit 26d39e8 into nodejs:main Dec 25, 2023
66 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 26d39e8

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
@RafaelGSS
test: fix inconsistency write size in test-fs-readfile-tostring-fail
684dd9d 
PR-URL: #51141
Refs: #51133
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
Merged
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@richardlau
test: fix inconsistency write size in test-fs-readfile-tostring-fail
cab7737 
PR-URL: #51141
Refs: #51133
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deokjinkim deokjinkim deokjinkim approved these changes

@lpinca lpinca lpinca 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. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pluris @nodejs-github-bot @lpinca @deokjinkim