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

Fix tests on node 20 #387

Merged
merged 5 commits into from
Sep 26, 2024
Merged

Fix tests on node 20 #387

merged 5 commits into from
Sep 26, 2024

Conversation

everett1992
Copy link

@everett1992 everett1992 commented Sep 25, 2024

This includes @andrewnicols's changes from #381

All unit tests pass on node 20 (and 22)
I have not written any new tests, or checked that any new APIs work correctly.
This at-least partially fixes #384. I tested these changes in an application that was failing with mock-fs 5.2.0 and node 20

andrewnicols and others added 4 commits September 25, 2024 11:04
Both `fs.promises.open().then(fh => fh.write()` and `fs.openSync().write()`
call write without a callback, but fs.promises must return a Promise,
while openSync must not.

I added _isPromise to the descriptor so we can return the right type
for the handle open was called with.

This doesn't feel right, but I couldn't figure out a different way to
call it.
Node 20 seems to change changed `atime` ect to be a non enumerable
property, so `stats.hasOwnProperty('atime')` is false, and
`Object.assign({}, stats)` doesn't copy atime.

I updated the file and directory creation to handle either atime, or
atimeMs fields, and updated a test to compare either.
@tschaub
Copy link
Owner

tschaub commented Sep 25, 2024

Thanks, @everett1992!

Could you add 20 to the list of node versions here:

node:
- 16
- 18

And then the readme can be updated here:

Tested on Linux, OSX, and Windows using Node 16 through 18. Check the tickets for a list of [known issues](https://github.com/tschaub/mock-fs/issues).

It looks like tests also pass on Node 22. If you find the same, we could add 22 to the list as well.

@tschaub
Copy link
Owner

tschaub commented Sep 25, 2024

Oh, I forgot about all the ignored tests. So not right to imply that all "tests pass" on Node 20 and 22.

Do you know if there are tests that are currently ignored that would pass with your changes?

@everett1992
Copy link
Author

Doesn't look like I fixed any of those

1) The API
       mock.fs()
         generates a mock fs module with a mock file system:
     TypeError: mock.fs is not a function
      at Context.<anonymous> (test/lib/index.spec.js:350:27)
      at process.processImmediate (node:internal/timers:491:21)

  2) The API
       mock.fs()
         passes options to the FileSystem constructor:
     TypeError: mock.fs is not a function
      at Context.<anonymous> (test/lib/index.spec.js:361:27)
      at process.processImmediate (node:internal/timers:491:21)

  3) The API
       mock.fs()
         accepts an arbitrary nesting of files and directories:
     TypeError: mock.fs is not a function
      at Context.<anonymous> (test/lib/index.spec.js:376:27)
      at process.processImmediate (node:internal/timers:491:21)

@everett1992
Copy link
Author

I think those disabled mock.fs tests should be removed, since mock.fs was removed in v4

Upgrading to version 4
The mock.fs() function has been removed. This returned an object with fs-like methods without overriding the built-in fs module.

https://github.com/tschaub/mock-fs/blob/main/changelog.md#400

@tschaub
Copy link
Owner

tschaub commented Sep 26, 2024

I think those disabled mock.fs tests should be removed, since mock.fs was removed in v4

Thanks for the reminder! Those tests could have been removed instead of ignored in https://github.com/tschaub/mock-fs/pull/182/files#diff-e78e0b8d4a6016fffbd3160920548d5860afed84b611d22f4f6e0c42609d7ce6.

@tschaub tschaub merged commit 3e39479 into tschaub:main Sep 26, 2024
12 checks passed
@tschaub
Copy link
Owner

tschaub commented Sep 26, 2024

Thank you @everett1992 and @andrewnicols!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix compatibility with Node 20
3 participants