test: add and use tmpdir.hasEnoughSpace() by tniessen · Pull Request #47767 · nodejs/node · GitHub

In general, we assume that the tmpdir will provide sufficient space for most tests. Some tests, however, require hundreds of megabytes or even gigabytes of space, which often causes them to fail, especially on our macOS infrastructure. The most recent reliability report contains more than 20 related CI failures.

This change adds a new function hasEnoughSpace() to the tmpdir module that uses statfsSync() (thanks @SheikhSajid and @cjihrig!) to guess whether allocating a certain amount of space within the temporary directory will succeed.

This change also updates the most frequently failing tests to use the new function such that the relevant parts of the tests are skipped if tmpdir has insufficient space. Hopefully, this will significantly reduce the number of related CI failures.

Refs: nodejs/reliability#549

what about hasEnoughSpace so it will return true if the operation is possible

what about hasEnoughSpace so it will return true if the operation is possible

We usually have a conditional skip() before the actual test definition, so with hasEnoughSpace(), either all call sites would need to use a negation or the order of skip() and test would need to be reversed.

what about hasEnoughSpace so it will return true if the operation is possible

We usually have a conditional skip() before the actual test definition, so with hasEnoughSpace(), either all call sites would need to use a negation or the order of skip() and test would need to be reversed.

I wouldn't block on it, but that is exactly what we're currently doing for the similar methods in common: e.g.

node/test/pummel/test-crypto-timing-safe-equal-benchmarks.js

Lines 3 to 7 in b31d587

    
           if (!common.hasCrypto) 
        
             common.skip('missing crypto'); 
        
           if (!common.enoughTestMem) 
        
             common.skip('memory-intensive test');

That's also a good point @richardlau. I don't feel strongly about it. Personally, I think that hasEnoughSpace() === true might be misleading, because it does not imply that there will actually be enough space (because file system block allocations aren't entirely predictable, because of race conditions, etc.). On the other hand, hasTooLittleSpace() === true pretty much always means that it's not worth trying.

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

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

I am +1 on this, but how do we prevent this method from hiding real issues?

@MoLow Could you clarify what you are concerned about?

Based on my understanding, for this change to hide real issues, one of these must be true: either there is a bug in hasTooLittleSpace() that causes it to incorrectly return true, or running the test despite hasTooLittleSpace() == true would uncover a real issue. Neither seems likely to me.

Ideally, within our own test infrastructure, hasTooLittleSpace() should never return true, thus, on ideal infrastructure, this change has no effect. Of course, if, for example, all of our macOS infrastructure always had too little space for a certain test, and that particular test would uncover a bug that only manifests on macOS, then this change could hide said bug.

I would prefer hasEnoughSpace() to be consistent with the common.enoughTestMem boolean, but I understand your concern about the ambiguity of "enough" and I don't feel strongly about it. I'm only commenting to point out the existing enoughTestMem which has not been mentioned yet, in case that consistency is compelling.

I guess I'd also prefer hasInsufficientSpace() to hasTooLittleSpace(). But again, not strongly enough to block, and if I wasn't already leaving a comment, I might not say anything.

if, for example, all of our macOS infrastructure always had too little space for a certain test, and that particular test would uncover a bug that only manifests on macOS, then this change could hide said bug.

I am not sure how unlikely such a scenario is. this recently landed PR is an example of specific filesystem behaviors on different platforms.
assuming this check will only be used with tests that require very large disk space, that will probably reduce the likelihood

In general, we assume that the tmpdir will provide sufficient space for most tests. Some tests, however, require hundreds of megabytes or even gigabytes of space, which often causes them to fail, especially on our macOS infrastructure. The most recent reliability report contains more than 20 related CI failures. This change adds a new function hasEnoughSpace() to the tmpdir module that uses statfsSync() to guess whether allocating a certain amount of space within the temporary directory will succeed. This change also updates the most frequently failing tests to use the new function such that the relevant parts of the tests are skipped if tmpdir has insufficient space. Refs: nodejs/reliability#549

@marco-ippolito @richardlau @Trott @MoLow Alright, I've changed it to hasEnoughSpace().

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

This needs to be reapproved before the commit-queue label can be added.

Landed in 609d2b0

In general, we assume that the tmpdir will provide sufficient space for most tests. Some tests, however, require hundreds of megabytes or even gigabytes of space, which often causes them to fail, especially on our macOS infrastructure. The most recent reliability report contains more than 20 related CI failures. This change adds a new function hasEnoughSpace() to the tmpdir module that uses statfsSync() to guess whether allocating a certain amount of space within the temporary directory will succeed. This change also updates the most frequently failing tests to use the new function such that the relevant parts of the tests are skipped if tmpdir has insufficient space. Refs: nodejs/reliability#549 PR-URL: #47767 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Richard Lau <rlau@redhat.com>

In general, we assume that the tmpdir will provide sufficient space for most tests. Some tests, however, require hundreds of megabytes or even gigabytes of space, which often causes them to fail, especially on our macOS infrastructure. The most recent reliability report contains more than 20 related CI failures. This change adds a new function hasEnoughSpace() to the tmpdir module that uses statfsSync() to guess whether allocating a certain amount of space within the temporary directory will succeed. This change also updates the most frequently failing tests to use the new function such that the relevant parts of the tests are skipped if tmpdir has insufficient space. Refs: nodejs/reliability#549 PR-URL: nodejs#47767 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Richard Lau <rlau@redhat.com>

tniessen added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels Apr 28, 2023

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 28, 2023

jasnell approved these changes Apr 28, 2023

View reviewed changes

tniessen added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 29, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2023

cjihrig approved these changes Apr 29, 2023

View reviewed changes

Comment thread test/common/README.md Outdated

tniessen force-pushed the test-tmpdir-space branch from f5a4908 to d444ceb Compare April 29, 2023 15:25

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2023

anonrig approved these changes Apr 30, 2023

View reviewed changes

MoLow approved these changes Apr 30, 2023

View reviewed changes

tniessen force-pushed the test-tmpdir-space branch from d444ceb to a74116b Compare April 30, 2023 10:27

tniessen changed the title test: add and use tmpdir.hasTooLittleSpace() test: add and use tmpdir.hasEnoughSpace() Apr 30, 2023

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2023

cjihrig approved these changes May 1, 2023

View reviewed changes

richardlau approved these changes May 1, 2023

View reviewed changes

tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label May 1, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 1, 2023

nodejs-github-bot merged commit 609d2b0 into nodejs:main May 1, 2023

targos mentioned this pull request May 2, 2023

v20.1.0 release proposal #47820

Merged

This was referenced May 4, 2023

CI Reliability 2023-05-04 nodejs/reliability#555

Open

CI Reliability 2023-05-05 nodejs/reliability#556

Open

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

test: add and use tmpdir.hasEnoughSpace() by tniessen · Pull Request #47767 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: add and use tmpdir.hasEnoughSpace() - #47767

test: add and use tmpdir.hasEnoughSpace()#47767nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom tniessen:test-tmpdir-spacetniessen/node:test-tmpdir-spaceCopy head branch name to clipboard

Conversation

tniessen commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marco-ippolito commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Apr 28, 2023

Uh oh!

richardlau commented Apr 28, 2023

Uh oh!

tniessen commented Apr 28, 2023

Uh oh!

nodejs-github-bot commented Apr 29, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 29, 2023

Uh oh!

MoLow commented Apr 29, 2023

Uh oh!

tniessen commented Apr 29, 2023

Uh oh!

Trott commented Apr 30, 2023

Uh oh!

MoLow commented Apr 30, 2023

Uh oh!

tniessen commented Apr 30, 2023

Uh oh!

nodejs-github-bot commented Apr 30, 2023

Uh oh!

tniessen commented May 1, 2023

Uh oh!

nodejs-github-bot commented May 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

test: add and use tmpdir.hasEnoughSpace()#47767
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
tniessen:test-tmpdir-space

tniessen commented Apr 28, 2023 •
edited
Loading

marco-ippolito commented Apr 28, 2023 •
edited
Loading