Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests are failing on mac #571

Closed
JPeer264 opened this issue Apr 19, 2018 · 12 comments
Closed

Tests are failing on mac #571

JPeer264 opened this issue Apr 19, 2018 · 12 comments

Comments

@JPeer264
Copy link
Collaborator

JPeer264 commented Apr 19, 2018

  • Operating System:
    macOS 10.13.3
  • Node.js version:
    v6.14.1 - 1 test is failing
    v8.11.1 - 3 tests are failing
  • fs-extra version:
    v6-dev - branch

I don't know why they pass on travis though


1 test is the same failing on v6 and v8: utimes.test.js:31:7. It asserts false but get true. The reason might be, that my flash storage is on APFS and not on HFS. I have no clue how to fix this.


The other two in v8 are actually just timestamp checks, which are different for ~4ms:

  • copy-sync-preserve-time.test.js:48:11
  • copy-preserve-time.test.js:51:11

I don't know if that is valid for you, but the assert can be changed to following:

assert.strictEqual(utimes.timeRemoveMillis(toStat.mtime.getTime()), utimes.timeRemoveMillis(fromStat.mtime.getTime()))

The above solution can be a bit flaky as well, if the second timestamp is in another second than the other (according to the timeRemoveMillis method). E.g. toStat is 1524121168999 and fromStat is 1524121169001 then it fail although there are just 2ms in between.

Or we can check if those two timestamps are in between a threshold of, lets say, 5ms.

@RyanZim RyanZim added this to the 6.0.0 milestone Apr 19, 2018
@RyanZim
Copy link
Collaborator

RyanZim commented Apr 19, 2018

I can reproduce. Seems like Mac can be crazy buggy with timestamps. The CI seems to be running OS X 10.12.6 Sierra, while we're seeing this on High Sierra. Wonder what would happen if we changed the osx_image? https://docs.travis-ci.com/user/reference/osx/#OS-X-Version

@JPeer264
Copy link
Collaborator Author

Seems like travis won't fail on the newest image: https://travis-ci.org/jprichardson/node-fs-extra/builds/368613666

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 19, 2018

Odd. Anyhow, I'm not sure how to best fix this.

@JPeer264
Copy link
Collaborator Author

Another solution might also be to use Promise.all to call both at the same time

Promise.all([
  fs.statSync(a),
  fs.statSync(b)
]).then((stat) => {
  assert.strictEqual(stat[0].mtime.getTime(), stat[1].mtime.getTime())
})

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 19, 2018

If they're sync methods, that won't help anything.

@DiegoRBaquero
Copy link

Confirmed. Maybe just make sure abs diff is less than 10ms (or an arbitrary threshold)

@mbargiel
Copy link

I'm currently investigating this (since I need to add tests for issue #629 to confirm my fix). I modified one of the "preserve-timestamp" tests to rewind the source files' atime and mtime to 5 seconds before "now" so that we can be sure the timestamp is properly set. Lo and behold! atime gets modified before the end of the test. So it's not a precision issue, it's something either in the copy code or in the test that touches the copied file after the copy and alters the timestamp.

As for the failing test in utimes.test.js, that's apparently because as of High Sierra, the HFS+ (1-s resolution) has been replaced by APFS (1-ns resolution), so the check in that test which looks for darwin and assumes that it implies 1-s resolution is no longer correct. A better check/heuristic must be put in place - somehow. Perhaps adding a check on os.release() as well? (According to https://en.wikipedia.org/wiki/Darwin_(operating_system), 17.0.0 is the Darwin release for the MacOS version where HFS+ is replaced by APFS)

@mbargiel
Copy link

Oh, and regarding the OP's problem with using flash storage whose FS might not match the OS disk FS, https://apple.stackexchange.com/a/324512/52665 provides a way on MacOS to figure out the file system used for a given path - we could use that if on MacOS at the start of the test, to check the file system of the root directory. An other option might be to mock the file accesses themselves so the test is platform-agnostic...

@mbargiel
Copy link

mbargiel commented Oct 12, 2018

Further investigation notes on the timestamp discrepancy.

I haven't been able to repro a mtime discrepancy. The atime discrepancy, however, happens systematically.

When copying src to dest, the atime set on dest by both the copy call (on MacOS 10.13.6) and the preserveTimestamp option in fs-extra is the atime of src before the copy - however, the copy operation updates the atime of the src file, and this is not captured by the copy in the dest atime. Since the copy is not instantaneous, this explains why creating a file and copying it in a test usually works when the time resolution is 1 sec (that must have been slightly unstable though, with occasional failures), but definitely fails more often when the time resolution is 1 ms or all the time when it's 1 ns. Also mind-boggling: the original src atime is set in the dest atime, even though atime was access later (when it was created!)

It seems to me that attempting to keep atime in sync is doomed to fail since atime maintenance is mainly OS-driven anyway. (EDIT although as long as we copy using node, it might not be that unpredictable...? it would mean a post-copy fs.stat roundtrip, however, to fetch the updated src atime rather using the atime from the pre-copy stats.)

@RyanZim The fix I would suggest for these tests:

  1. remove test checks on atime (too hard to predict in a portable way); alternatively, fix copy/copySync to query the src stats rather than use the pre-copy stats.
  2. rewind the timestamps by a couple seconds on the source files before copying them (e.g. Date.now() / 1000 - 5) - this ensures the timestamp assertions passe by design, and not by luck or due to coarse FS time resolution.

@mbargiel
Copy link

@RyanZim PR #633 implements the suggestions I made above (with the post-copy stats re-query fix for 1.)

@manidlou
Copy link
Collaborator

manidlou commented May 7, 2020

@RyanZim @JPeer264 Is this still valid?

@JPeer264
Copy link
Collaborator Author

JPeer264 commented May 7, 2020

Tested fs-extra@9 against all supported Node versions on latest MacOS 10.15.4. Everything is now working as expected

@JPeer264 JPeer264 closed this as completed May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants