test: add and use tmpdir.hasEnoughSpace()

[tniessen]

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

[marco-ippolito]

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

[tniessen]

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.

[richardlau]

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');

[tniessen]

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.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[MoLow]

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

[tniessen]

@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.

[Trott]

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.

[MoLow]

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

[tniessen]

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

[nodejs-github-bot]

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

[tniessen]

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

[nodejs-github-bot]

Landed in 609d2b0