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

Merged tschaub/mock-fs into Slayer95/mock-fs for getting Node 8 support #1

Closed
wants to merge 42 commits into from
Closed

Conversation

webextensions
Copy link

No description provided.

tschaub and others added 30 commits November 1, 2016 06:42
Only override process.binding('fs')
Document breaking changes in the upcoming 4.0.0-beta.1 release
* Useful for tests where you wish to make 'general' assertions over the
  whole filesystem (e.g. security testing!)
* Also addresses an issue where the bindings were having 'extraneous' keys
  left when the mock was restored. (Was unable to provide an isolated test
  for this as the 'bindings' part is internal to the mock.
…ystem

Exposes the root mocked filesytem if the mock is currently applied.
Binding.open not following symlinks correctly
…haub#197)

Node 7.7 changed the behavior of the `binding.{stat,lstat,fstat}` functions (see nodejs/node#11522). This updates the binding functions used in mock-fs to match the new Node binding behavior, while still maintaining compatibility with old Node versions. The new behavior is detected when the second argument to `binding.{stat,lstat,fstat}` is a `Float64Array`, which would be an invalid argument for previous versions of the binding.
…at-lstat-fstat

Update fs.stat(), fs.lstat(), and fs.fstat() for Node 7.7+ (fixes tschaub#197)
Add support for fs.mkdtemp and fs.mkdtempSync
Fixing test failures on node 8
@@ -65,7 +75,7 @@ describe('The API', function() {
mock.restore();
});

it('uses the real fs module in require() calls', function() {
xit('uses the real fs module in require() calls', function() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of this fork is that this test should pass :|

@webextensions
Copy link
Author

My bad ... didn't realize that part in the merge ... the tests are erroring out with Node 8, so attempted if this could be a quickfix.

Like tschaub#203, would it be possible for you to extend support to Node 8 ?

@Slayer95
Copy link
Owner

Slayer95 commented Jun 28, 2017

I definitely want to extend support to Node 8.

However, there have been plenty of non-trivial changes in the Node.js internals, specially regarding stat. Therefore, I must have a deeper look at the problem and decide whether to stick with the mock-fs@3 approach of copying the fs sources -with appropriate edits-, or rebase on mock-fs@4 and reimplement module-load hooks to ensure the right behavior with require.

I am currently leaning towards the second option. However, working out the details is not my top priority right now.

@webextensions
Copy link
Author

Thanks for considering ... Closing my invalid PR :-)

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.

7 participants