test: change test limit for atime from 2ms to 5ms

[duncanhealy]

doc: change test limit for atime from 2ms to 5ms
fixes #24593

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[Trott]

I'm concerned that this change masks an actual bug. What's the reason to believe it doesn't?

[addaleax]

@Trott I think there are two changes which got mixed together here…

As for increasing the timeout, I’m okay because the test is just inherently flaky the way it’s written and we can’t really do anything about that.

[Trott]

As for increasing the timeout, I’m okay because the test is just inherently flaky the way it’s written and we can’t really do anything about that.

@addaleax Flaky due to rounding errors, or something else?

[addaleax]

@Trott It’s flaky because the stat calls for the regular version and the bigint version are made at different times, and so they leave a window for stat data changing in between, or at least that’s my understanding of where this comes from

[duncanhealy]

As for increasing the timeout, I’m okay because the test is just inherently flaky the way it’s written and we can’t really do anything about that.

@addaleax Flaky due to rounding errors, or something else?

lstat running twice first with {bigint:true} passed as parameter, on this test I was constistently getting 4ms difference for atime

@Trott It’s flaky because the stat calls for the regular version and the bigint version are made at different times, and so they leave a window for stat data changing in between, or at least that’s my understanding of where this comes from

Could we change the nested call instead? L135-139 test-fs-stat-bigint.js

 fs.lstat(link, { bigint: true }, (err, bigintStats) => {
    fs.lstat(link, (err, numStats) => {
      verifyStats(bigintStats, numStats);
    });

[Trott]

@Trott It’s flaky because the stat calls for the regular version and the bigint version are made at different times, and so they leave a window for stat data changing in between, or at least that’s my understanding of where this comes from

Sounds good to me. Thanks! Maybe a comment explaining that would be useful.

/ping @nodejs/testing

[nodejs-github-bot]

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

[addaleax]

@duncanhealy Could you rebase this against the master branch, or squash the commits here together? Our CI doesn’t deal well with merge conflicts.

[Trott]

Subsystem should be test rather than doc. If this gets fixed in a rebase or whatever, then great. But if not, then it can be fixed when landing.

[duncanhealy]

@duncanhealy Could you rebase this against the master branch, or squash the commits here together? Our CI doesn’t deal well with merge conflicts.

changes squashed

[Trott]

The test does not seem to be failing anymore, so I'm going to close this. Feel free to re-open if you think that's the wrong thing to do and this should land regardless. I'm not acting on a strong opinion. I'm just doing a little tidying up. Thanks!

[Trott]

Stress test on FreeBSD shows this is still A Thing. Re-opening.

[lundibundi]

@Trott this actually fails for me locally on Arch Linux every now and then (the 2ms), so I'd be in favor of merging this unless we find a better solution.

[Trott]

I think if we update the comment and move the 5 magic number to a variable that has the comment right above it, this is good to land (with a clean CI).

[Trott]

LGTM with nits

[Trott]

I addressed the nits, updated the commit message, and pushed the changes to the PR branch.

[Trott]

@lundibundi Do the changes I made just now look good to you? If so, feel free to add the author ready label (or land it if CI is finished and green/yellow).

[nodejs-github-bot]

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

[Trott]

So, this doesn't fully fix the test, but it fixes the issue with times being off. There's still an issue with the block count being wrong, somehow, but I'll open a separate issue for that after this lands.

[Trott]

Landed in 5b0308c...fb437c4