test: fix test-fs-utimes on non-Y2K38 file systems by Trott · Pull Request #37707 · nodejs/node · GitHub

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

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

Still need to skip on SmartOS and possibly others. Putting into Draft until I get around to looking more closely.

I think skipping as was done before it better. The sets it to off by one, so allowing off by one is the same thing.

The previous skipped platforms are a separate issue that I was optimistically hoping this change would fix. If we dont allow the max - 1 value, then we need to skip all Linux hosts.

I think the thing to do is restore the previous skipped platforms and allow max - 1.

OK, this is ready for review.

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

Labeling this wip pending resolution of #36591 (comment).

I think this is now ready for review. @targos @richardlau @mhdawson

sorry, the skip is not working correctly with my system and the test fails as before.

If it can help:

$ touch -t 204001020304 /tmp/y2k32-test

$ date -r /tmp/y2k32-test "+%Y%m%d%H%M"
203801190414

(Non-blocking observation.)
FWIW I believe GNU date is available via the coreutils package from the AIX toolbox but we don't currently have that installed on our CI hosts.

It is correct that the native date command doesn't support the -r option: https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/d_commands/date.html

What about stat -c '%Y' ?

Edit: updated to %Y
On my machine, it returns 2147483647, wich corresponds to 2038-01-19T03:14:07.000Z

AIX doesn't natively have stat but does have istat: https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/i_commands/istat.html

Like GNU date, the GNU version of stat looks to be available in the coreutils AIX toolbox package.

On macOS:

stat: illegal option -- c
usage: stat [-FlLnqrsx] [-f format] [-t timefmt] [file ...]

If there's something that will work on AIX, we can have a separate AIX path to do the check. Ditto for Windows. But I think those can also be added at a later date.

The IBM i check for Y2K38 support passed but the test failed when fs.utimesSync() threw with EINVAL. Since IBM i is not fully supported (yet?) and has lots of skipped tests, including the current version of this one, I'll go back to skipping-for-now in IBM i.

Current result on my machine:

$ node test/parallel/y2k38.js
1..0 # Skipped: File system appears to lack Y2k38 support (date failed)

Helps to push your changes before running the test....

IBM i test to confirm skip logic: https://ci.nodejs.org/job/node-test-commit-ibmi/298/ ✅

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

Move Y2K38-specific parts of test-fs-utimes to test-fs-utimes-y2K38.js. On non-Windows, check for Y2K38 support and skip if it is unsupported. Fixes: nodejs#36591 PR-URL: nodejs#37707 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

Landed in ab6c7dd

The IBM i check for Y2K38 support passed but the test failed when fs.utimesSync() threw with EINVAL. Since IBM i is not fully supported (yet?) and has lots of skipped tests, including the current version of this one, I'll go back to skipping-for-now in IBM i.

FWIW Just to follow this up... on IBM i process.platform === 'aix' (same as AIX), see

node/test/common/index.js

Lines 801 to 805 in d3417bb

    
           // On IBMi, process.platform and os.platform() both return 'aix', 
        
           // It is not enough to differentiate between IBMi and real AIX system. 
        
           get isIBMi() { 
        
             return require('os').type() === 'OS400'; 
        
           },

so common.isAIX is true and the check for Y2k38 support skipped. If I remove the check for common.isAIX, the Y2K38 support check fails (as expected) on IBM i.

Move Y2K38-specific parts of test-fs-utimes to test-fs-utimes-y2K38.js. On non-Windows, check for Y2K38 support and skip if it is unsupported. Fixes: #36591 PR-URL: #37707 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

On some platforms `date` may not support the `-r` option. Optimistically allow the test to proceed in that case as the previous `touch` had succeeded -- we were just not able to easily validate the file date. PR-URL: #37825 Refs: #37707 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Move Y2K38-specific parts of test-fs-utimes to test-fs-utimes-y2K38.js. On non-Windows, check for Y2K38 support and skip if it is unsupported. Fixes: #36591 PR-URL: #37707 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>

On some platforms `date` may not support the `-r` option. Optimistically allow the test to proceed in that case as the previous `touch` had succeeded -- we were just not able to easily validate the file date. PR-URL: #37825 Refs: #37707 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Trott requested review from mhdawson, richardlau and targos March 11, 2021 03:20

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Mar 11, 2021

Trott marked this pull request as draft March 11, 2021 16:57

mhdawson reviewed Mar 12, 2021

View reviewed changes

Trott marked this pull request as ready for review March 13, 2021 16:44

Trott force-pushed the fix-test-fs-utimes branch from f64b2f5 to 9b7fe78 Compare March 13, 2021 16:44