tschaub / mock-fs

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

Implicit createCwd even with createCwd: false #100

Open HatiGamedev opened 8 years ago

HatiGamedev commented 8 years ago

While tinkering with mountfs and mock-fs combination, I discovered a weird behaviour of mock-fs.

Following code will create the process.cwd() even if not so desired by using the option.createCwd = false

let mockfs = ...
const dummyFS = {
  'foo': 'bar'
}

mockfs(dummyFS, {createCwd: false});

Currently my solution is using a root directory - but that's one extra indentation :/

let mockfs = ...
const dummyFS = {
 '/data': {
  'foo': 'bar'
 }  
}

mockfs(dummyFS, {createCwd: false});

This behavior seems counter intuitive, especially if you want to use the mockfs#fs functionality.

tschaub commented 8 years ago

You should be able to provide the absolute path to the file you want to create. E.g.

mock({'/data/foo': 'bar'}, {createCwd: false});

What would you expect the absolute path to your foo file to be in the first case above?

HatiGamedev commented 8 years ago

It simply should not exist. Because there is no root relation. createCwd does magic here. Placing an file not supposed to be in ~/

The main issue arises if you use MockFs#fs function. Which you could mount using MountFs. Where 'foo' from Example#1 should exist in mount.

tschaub commented 8 years ago

@HatiEth can you provide a test that demonstrates the behavior you expect?

I agree there could be something that needs fixing. It would be good to get more specific about the expected behavior.

HatiGamedev commented 8 years ago

Yeah, mostly what I ment before (wrote it on phone...):

createCwd does not implicitly states that it will also be used as such.

So adding another placeInsideCwd or something would be good option? To keep old behaviour?

Test code: (discussable, just wrote down right now)

'use strict';

const should = require('should');
const fs = require('fs');
const mountFS = require('mountfs');
const mockfs = require('mock-fs');

mountFS.patchInPlace();

const dummyFS = {
  '/': {
    'file': 'some random file content'
  },
  '/directly_parented': 'directly_parented content',
  'unparent_file': 'unparent_file important content'
};

let cwd = process.cwd();

describe.only('mockfs#fs', function() {
  before(function() {
    let mock = mockfs.fs(dummyFS, { createCwd: false, createTmp: false });
    fs.mount('/mocks', mock);
    /*
    let files = fs.readdirSync('/mocks');
    console.log(files);
    */
  });
  after(function() {
    fs.unmount('/mocks'); // unmock '/mocks...' fs
    fs.unpatch(); // unpatch patched fs
  });

  /**
   * Should exists, but would be mountfs issue.
   */
  it('/mocks exists', function(done) {
    fs.stat('/mocks', (err, stats) => {
      should.not.exist(err);
      stats.isDirectory().should.be.true();
      done();
    });
  });

  /**
   * Make sure cwd is not created as asked. (createCwd: false)
   */
  it('cwd does not exists', function(done) {
    fs.stat(cwd, (err, stats) => {
      should.not.exist(err);
      stats.isDirectory().should.be.false();
      done();
    });
  });

  describe('/mocks/file', function() {
    /**
     * Should exists because in root dir? => but '/' gets converted to undefined
     */
    it('exists', function(done) {
      fs.stat('/mocks/file', (err, stats) => {
        should.not.exist(err);
        stats.isFile().should.be.true();
        done();
      });
    });

    it('has expected content');
  });

  /**
   * Expected to exists but is in ${cwd}/unparent_file
   */
  describe('/mocks/unparent_file', function() {
    it('exists', function(done) {
      fs.stat('/mocks/unparent_file', (err, stats) => {
        should.not.exist(err);
        stats.isFile().should.be.true();
        done();
      });
    });
    it('has expected content');
  });

  describe('/mocks/directly_parented', function() {
    it('exists', function(done) {
      fs.stat('/mocks/directly_parented', (err, stats) => {
        should.not.exist(err);
        stats.isFile().should.be.true();
        done();
      });
    });
    it('has expected content');
  });

  describe('cwd', function() {
    it('pure should exists, because mounted to /mocks not /', function(done) {
      fs.stat(`${cwd}`, (err, stats) => {
        should.not.exist(err);
        stats.isDirectory().should.be.true();
        done();
      });
    });
    it('/mocks/${cwd} should not exist, explicit said no createCwd', function(done) {
      fs.stat(`/mocks${cwd}`, (err, stats) => {
        should.not.exist(err);
        stats.isDirectory().should.be.false();
        done();
      });
    });
  });
});
gyandeeps commented 8 years ago

Same thing happened to me:

        finalMockDir = {
             "eslint/fixtures/config-hierarchy": {}
         };
        mockFs(finalMockDir, {
            createCwd: false,
            createTmp: true
        });

This always creates the above structure inside my process.cwd() but doesnt not create it inside os.tmpdir().

I think the issue is here: https://github.com/tschaub/mock-fs/blob/master/lib/filesystem.js#L15 This always resolves it according to process.cwd()

tschaub commented 8 years ago

It is still not clear to me what you would expect from this code:

var mock = require('mock-fs');
mock({foo: {}}, {createCwd: false});

What sort of assertion would you expect to be able to make after this?

gyandeeps commented 8 years ago
finalMockDir = {
             "eslint/fixtures/config-hierarchy": {}
         };
        mockFs(finalMockDir, {
            createCwd: false,
            createTmp: true
        });

Lets say if I execute the above code, then I should be able to say

fs.readdirSync(path.join(os.tmpdir(), "eslint")); // -> should not throw error. currently this will throw error.
tschaub commented 8 years ago

@gyandeeps - you'd need to provide the full path to eslint in your mock config. For example, this currently works:

var assert = require('assert');
var path = require('path');
var mock = require('mock-fs');
var fs = require('fs');
var os = require('os');

var dir = path.join(os.tmpdir(), 'new-directory');
assert.throws(() => fs.accessSync(dir));

mock({
  [dir]: {}
});
assert.doesNotThrow(() => fs.accessSync(dir));

mock.restore();
assert.throws(() => fs.accessSync(dir));

Using just those same modules (i.e. no mocha, no mountfs, etc.), I'm still not clear what someone would expect to be able to assert after doing this:

mock({foo: 'bar'}, {createCwd: false});
// what would you be able to assert here?

Currently, you can assert this:

assert.doesNotThrow(() => fs.accessSync(path.resolve('foo')));

But it sounds like @HatiEth might expect that to throw instead.

HatiGamedev commented 8 years ago
mock({foo: 'bar'}, {createCwd: false});
// what would you be able to assert here?

Well what would be the most obvious result to expect? The file is inaccessible. If the convention says, {'pure-file': 'content'} with pure-file being only a direct file path ( no / or \ ) - more like ./ then I would say the file is simply inaccessible because I expicitly said not to create the Cwd

I would not expect anything thrown on your side, if it is documented properly. But on fs side i would expect an error to be risen - because the file is destined to not exist.