test: fix test-fs-utimes on non-Y2K38 file systems #37707

Trott · 2021-03-11T03:20:18Z

Fixes: #36591

nodejs-github-bot · 2021-03-11T03:20:48Z

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

nodejs-github-bot · 2021-03-11T13:57:15Z

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

Trott · 2021-03-11T16:57:18Z

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

mhdawson · 2021-03-12T21:32:16Z

test/parallel/test-fs-utimes.js

  const Y2K38_mtime = 2 ** 31;
  fs.utimesSync(path, Y2K38_mtime, Y2K38_mtime);
  const Y2K38_stats = fs.statSync(path);
-  assert.strictEqual(Y2K38_stats.mtime.getTime() / 1000, Y2K38_mtime);
+  const mtimeStamp = Y2K38_stats.mtime.getTime() / 1000;
+  // Off-by-one value is permissible for file systems that don't support Y2K38.


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.

Trott · 2021-03-13T16:44:32Z

OK, this is ready for review.

nodejs-github-bot · 2021-03-14T18:39:43Z

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

Trott · 2021-03-16T14:54:35Z

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

Trott · 2021-03-17T06:24:57Z

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

targos · 2021-03-17T07:19:46Z

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

targos · 2021-03-17T07:22:16Z

If it can help:

$ touch -t 204001020304 /tmp/y2k32-test

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

test/parallel/test-fs-utimes-y2K38.js

richardlau · 2021-03-17T16:03:35Z

test/parallel/test-fs-utimes-y2K38.js

-if (!common.isWindows) {
+// Check for Y2K38 support. For Windows and AIX, assume it's there. Windows
+// doesn't have `touch` and `date -r` which are used in the check for support.
+// AIX lacks `date -r`.


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

Trott · 2021-03-17T18:48:46Z

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.

targos · 2021-03-17T19:16:01Z

Current result on my machine:

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

Trott · 2021-03-17T19:49:08Z

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/ ✅

nodejs-github-bot · 2021-03-18T03:55:18Z

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 <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Trott · 2021-03-18T04:50:25Z

Landed in ab6c7dd

richardlau · 2021-03-20T00:45:55Z

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 <[email protected]> Reviewed-By: Richard Lau <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Richard Lau <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

Trott requested review from targos, richardlau and mhdawson 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