tschaub / mock-fs

Configurable mock for the fs module
https://npmjs.org/package/mock-fs
Other
911 stars 86 forks source link

feat: support nodejs v10 and v11, support experimental fs.promises #260

Closed 3cp closed 5 years ago

3cp commented 5 years ago

I was terrified by the amount of refactor work in my own projects to remove mock-fs. So I jump in to fix mock-fs instead.

First, it passes all tests in nodejs v4, v6, v8, v10, v11 on Linux/Windows/macOS. https://travis-ci.org/huochunpeng/mock-fs/builds/488264371

Note:

  1. the failure of nodejs v10 and v11 on windows is probably due to a bug in nodejs itself. https://github.com/nodejs/node/issues/25913
  2. removed v4 from travis-ci matrix. v4 works fine, but travis-ci windows box just had problem to install npm@3. BTW nodejs v4 is EOL anyway.

Tested on one of the projects I am working on, it finally can pass tests in nodejs v10. https://travis-ci.org/huochunpeng/cli/builds/488267193

What's in this PR:

1. nodejs v10 fs binding has added one more optional parameter ctx to all methods.

It is used to capture the error details when calling fs synced methods. I am too lazy to add the test coverage in lib/binding.spec.js. But all the new code is covered when running lib/index.spec.js in nodejs v10, those fs methods exercised the new code in lib/binding.js.

2. added support of kUsePromises and Binding.prototype.openFileHandle to support experimental fs.promises.

I did not add enough test coverage to this yet. Only got one on readFile. I need to read more on fs.promises, then add more test coverage, probably in another PR.

3. replaced hard coded uv error codes with the runtime errmap in uv binding.

Windows has different errno numbers than posix os.

4. normalized uid/gid to NaN on OS (Windows) does support it.

This fixed some failing tests on Windows. I am not sure this is the correct thing.

5. Disable file write stream's _writev method.

The nodejs v10+ _writev implementation uses unpatched binding.writeBuffers() method.

Details in lib/index.js comments.

// Have to disable write stream _writev on nodejs v10+.
//
// nodejs v8 lib/fs.js
// note binding.writeBuffers will use mock-fs patched writeBuffers.
//
//   const binding = process.binding('fs');
//   function writev(fd, chunks, position, callback) {
//     // ...
//     binding.writeBuffers(fd, chunks, position, req);
//   }
//
// nodejs v10+ lib/internal/fs/streams.js
// note it uses original writeBuffers, bypassed mock-fs patched writeBuffers.
//
//  const {writeBuffers} = internalBinding('fs');
//  function writev(fd, chunks, position, callback) {
//    // ...
//    writeBuffers(fd, chunks, position, req);
//  }
//
// Luckily _writev is an optional method on Writeable stream implementation.
// When _writev is missing, it will fall back to make multiple _write calls.

6. weird fix for read in nodejs v10+.

The read seems suffered similar issue (calling unpatched binding method) as the write stream issue solved in (5). https://github.com/tschaub/mock-fs/issues/254#issuecomment-425320578

When I solved it, I have not read through the above issue. I don't understand what's going on, but anyway, my fix works.

Details in lib/binding.js comments.

// I don't know exactly what is going on.
// If _openFiles is a property of binding instance, there is a strange
// bug in nodejs v10+ that something cleaned up this._openFiles from
// nowhere. It happens after second mockfs(), after first mockfs()+restore().

// So I moved _openFiles to a private var. The other two vars (_system,
// _counter) do not hurt.
// This fixed https://github.com/tschaub/mock-fs/issues/254
// But I did not dig deep enough to understand what exactly happened.
var _system;
var _openFiles = {};
var _counter = 0;

Closes #256, #254, #245, #238 Supersedes #259

3cp commented 5 years ago

The ci check here shows exactly same result as my own travis run. The failure is likely due a nodejs v10 bug on windows. nodejs/node#25913

3cp commented 5 years ago

For those who wants to test this feature, update your package.json with "mock-fs": "huochunpeng/mock-fs#node10". Thx!

tschaub commented 5 years ago

@huochunpeng - what a great contribution! Thank you so much for your effort on this. I'll cut a release shortly.

3cp commented 5 years ago

@tschaub thx for faster-than-expected release!

However, can you review item 4 and item 6 of my changes?

ljharb commented 5 years ago

Rather than removing node 4 entirely, you could also make the windows node 4 an allowed failure

3cp commented 5 years ago

@ljharb I did try reading allow_failures for maybe 2 mins, did not get an obvious answer on how to target both nodejs and os within my patient, so I lazily gave up 😄

ljharb commented 5 years ago
- os: windows
  node_js: 4
3cp commented 5 years ago

Thx, I will give it a try.