[v16.x backport] fs: use signed types for stat data

[LiviaMedeiros]

Backport of: #43714

On arm-12+, fsPromises.stat timestamps are returned as 0 even for small positive values. Excluding this platform from test.

[nodejs-github-bot]

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

[aduh95]

On arm-12+, fsPromises.stat timestamps are returned as 0 even for small positive values. Excluding this platform from test.

Doesn't that mean that 9e40873...5311b7d should be fast-tracked on main first?

[richardlau]

On arm-12+, fsPromises.stat timestamps are returned as 0 even for small positive values. Excluding this platform from test.

Doesn't that mean that 9e40873...5311b7d should be fast-tracked on main first?

FWIW as background arm-12+ is referring to https://ci.nodejs.org/job/node-test-binary-arm-12+/ which is the job that runs tests for armv7l on Raspberry Pi devices. They currently aren't being run on main/v18.x as they need updates which is why the failures haven't been seen until the tests were ported here to v16 (where we're still running tests on the Pi's).

Also FWIW I suspect the issue isn't actually Linux on arm but the filesystem being used.

[LiviaMedeiros]

Doesn't that mean that 9e40873...5311b7d should be fast-tracked on main first?

Yes, if similar test environment will be used on newer versions as well.

FWIW as background arm-12+ is referring to https://ci.nodejs.org/job/node-test-binary-arm-12+/ which is the job that runs tests for armv7l on Raspberry Pi devices. They currently aren't being run on main/v18.x as they need updates which is why the failures haven't been seen until the tests were ported here to v16 (where we're still running tests on the Pi's).

Also FWIW I suspect the issue isn't actually Linux on arm but the filesystem being used.

Thanks for clarification! Which FS is used on that test platform? Is there a straightforward way to detect it?
Othewise we can add something like

await fsPromises.utimes(filepath, 1000, 1000);
const { atimeMs, mtimeMs } = await fsPromises.stat(filepath);
if (atimeMs !== 1000 || mtimeMs !== 1000) common.skip('bad platform or fs');

But it still feels weird to literally skip a test if it fails. 😅

[aduh95]

Doesn't that mean that 9e40873...5311b7d should be fast-tracked on main first?

Yes, if similar test environment will be used on newer versions as well.

I disagree, it's not because this project doesn't have a CI for this environment that it's not worth landing it on main. If someone with this env wants to compile node and run the test themself, there's no reason we should impose them a false positive test failure.

[richardlau]

FWIW as background arm-12+ is referring to https://ci.nodejs.org/job/node-test-binary-arm-12+/ which is the job that runs tests for armv7l on Raspberry Pi devices. They currently aren't being run on main/v18.x as they need updates which is why the failures haven't been seen until the tests were ported here to v16 (where we're still running tests on the Pi's).
Also FWIW I suspect the issue isn't actually Linux on arm but the filesystem being used.

Thanks for clarification! Which FS is used on that test platform? Is there a straightforward way to detect it?

They are NFS mounts, e.g.

$ df -Th /home/iojs/build/workspace/node-test-binary-arm/
Filesystem                                                                       Type  Size  Used Avail Use% Mounted on
192.168.1.10:/exports/nodejs/pi/test-requireio_williamkapke-debian10-arm64_pi3-3 nfs   458G  206G  230G  48% /
$

On the Pi's the tests are run in containers and folder above is mounted inside the container on the machine.

[nodejs-github-bot]

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

[targos]

If I understand correctly, after #44174 lands, we can close this PR and remove the backport label from #43714 ?

[LiviaMedeiros]

I disagree, it's not because this project doesn't have a CI for this environment that it's not worth landing it on main. If someone with this env wants to compile node and run the test themself, there's no reason we should impose them a false positive test failure.

Perhaps I worded it incorrectly: test env was related to fast-tracking, i.e. if it should be ported to main ASAP.

As for local user environments, I think it would be reasonable to keep the test as strict as possible, as long as it doesn't break common cases (example: absence of ipv6 support is common, blocked ports for localhost are not), and doesn't unconditionally break CI.

If there is a better approach for such tests, I'm open for suggestions.

They are NFS mounts

I guess NFS might not support atime? On both first and second tries it returned 0, so it doesn't look like fsync-related issue.

If I understand correctly, after #44174 lands, we can close this PR and remove the backport label from #43714 ?

Yes. If #43714 with #44174 will be mergeable into v16 without any problems, the label can be removed and this PR can be closed.

[LiviaMedeiros]

Superseded by #44174