5.0.0 Test suite broken on Node.js 8.x and 9.x

[MylesBorins]

• Operating System: OSX
• Node.js version: 8.x / 9.x
• fs-extra version: 5.0.0 || master

The test for this module is not currently passing with Node.js 8.x or 9.x. and it appears to be 0bd52793a2e15b593ce3f19167d2550e822665a3 that broke it

[RyanZim]

Confirmed issue; investigating.

[RyanZim]

This is weird; it seems the Node.js copyFile and copyFileSync methods aren't consistent between Mac and Linux. On Mac, it seems to always preserve timestamps, while the timestamp is changed on Linux. Here's a test case repo: https://github.com/RyanZim/fs-copy-file-test Note that this is testing the fs methods directly; not fs-extra. The tests pass on Linux, as the timestamps are changed; they fail on Mac since the timestamps are preserved.

I'm not sure how fs-extra should handle this.

[MylesBorins]

You should open an issue against the main repo.... This may be something broken in libuv

On Jan 10, 2018 9:45 AM, "Ryan Zimmerman" notifications@github.com wrote:

This is weird; it seems the Node.js copyFile and copyFileSync methods aren't consistent between Mac and Linux. On Mac, it seems to always preserve timestamps, while the timestamp is changed on Linux. Here's a test case repo: https://github.com/RyanZim/fs-copy-file-test Note that this is testing the fs methods directly; not fs-extra. The tests pass on Linux, as the timestamps are changed; they fail on Mac since the timestamps are preserved.

I'm not sure how fs-extra should handle this.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jprichardson/node-fs-extra/issues/536#issuecomment-356622984, or mute the thread https://github.com/notifications/unsubscribe-auth/AAecV10JVQAWkfeDHad2VddVi5dC8kI6ks5tJM0FgaJpZM4RY1jo .

[RyanZim]

@MylesBorins I assume you mean https://github.com/nodejs/node when you say the main repo?

[MylesBorins]

Indeed, sorry for not being exact

On Jan 10, 2018, at 10:02 AM, Ryan Zimmerman notifications@github.com wrote:

@MylesBorins https://github.com/mylesborins I assume you mean https://github.com/nodejs/node https://github.com/nodejs/node when you say the main repo?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jprichardson/node-fs-extra/issues/536#issuecomment-356628356, or mute the thread https://github.com/notifications/unsubscribe-auth/AAecVzXGxxLTAtjMn_302WG5eYwAfpkIks5tJNECgaJpZM4RY1jo.

[RyanZim]

Just did a quick search, and it seems this was already reported on the end of another issue: https://github.com/nodejs/node/issues/15793#issuecomment-351230606. Not sure if it's wise to open a new issue about this.

[joyeecheung]

That seems to be a different issue. It's requesting the exact option tested in the broken test: when preserveTimestamp is true, call utimes to preserve the timestamps. The test suite of fs-extra fails because when preserveTimestamp is false, the timestamps are still the same because of platform defaults. To fix that fs-extra (or the core, but it's unlikely) need to manually update the timestamps to a different value when preserveTimestamp is false, or don't assume the timestamps would be different by default on those platforms. I would say updating the tests seems to be more correct because the doc does not guarantee the timestamps would be different by default either.

[RyanZim]

@joyeecheung The issue is about a slightly different thing, but the linked comment is what we're talking about:

At present the behavior on Linux differs from that on Windows and Mac in an unexpected manner. While all three platforms preserve file mode metadata, only Windows and Mac preserve the modification time.

[RyanZim]

From https://github.com/nodejs/node/issues/15793#issuecomment-356696390:

If someone still thinks it's an excellent idea, he or she should open a pull request and we'll see where the discussion leads.

I don't know C, so I can't do any hacking on libuv. Should we try to just hack around this in JS internally?

[joyeecheung]

@RyanZim Even if you implement preserveTimestamp in core, the timestamps will still be the same by default on Mac and Windows, so the tests will still fail unless you deliberately update the timestamps when the option is false.

[RyanZim]

@MylesBorins So do you think there's any chance of this getting fixed at the Node/libuv level, or do we just have to deal with this?

[rally25rs]

Hey guys 👋 I came across this issue while hunting down the same problem in yarn. IIRC fs.copyFile is a thin wrapper over the native filesystem's file copy, which could explain the difference between OSs.

I handled it by calling fs.futimes() to manually change the timestamps back to the source file's timestamps. You end up having to re-open the destination file, but it doesn't seem to kill performance.

While I agree that the consistency issue should be fixed in node or libuv, this might be a usable workaround.

Hope that helps!

[RyanZim]

I discussed this with @jprichardson and we've decided that when preserveTimestamps is false, timestamps may or may not be preserved. This will be officially documented starting in fs-extra v6.

[RyanZim]

Done in https://github.com/jprichardson/node-fs-extra/pull/563; will be released later this month.