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

Regression with memfs #515

Closed
gawi opened this issue Mar 20, 2023 · 10 comments
Closed

Regression with memfs #515

gawi opened this issue Mar 20, 2023 · 10 comments

Comments

@gawi
Copy link

gawi commented Mar 20, 2023

This was working in 8.1.0

import { fs, vol } from 'memfs';

const memfs = fs;
jest.doMock('fs', () => memfs);
import { Glob } from 'glob';

import { promisify } from 'util';
const glob = promisify(Glob);

describe('glob', () => {
  beforeEach(() => {
    vol.fromJSON({ '/x': 'abc' });
  });

  it('should match single file with pattern', async () => {
    const expandedFiles = await glob('/**/*', { nodir: true });
    expect(expandedFiles).toStrictEqual(['/x']);
  });

  it('should match single file without pattern', async () => {
    const expandedFiles = await glob('/x', { nodir: true });
    expect(expandedFiles).toStrictEqual(['/x']);
  });
});

The second test case fails with 9.3.0.

Test re-written for 9.3.0:

import { fs, vol } from 'memfs';

const memfs = fs;
jest.doMock('fs', () => memfs);
import { glob } from 'glob';

describe('glob', () => {
  beforeEach(() => {
    vol.fromJSON({ '/x': 'abc' });
  });

  it('should match single file with pattern', async () => {
    const expandedFiles = await glob('/**/*', { nodir: true });
    expect(expandedFiles).toStrictEqual(['/x']);
  });

  it('should match single file without pattern', async () => {
    const expandedFiles = await glob('/x', { nodir: true });
    expect(expandedFiles).toStrictEqual(['/x']);
  });
});
@isaacs
Copy link
Owner

isaacs commented Mar 21, 2023

It's failing because you're giving it a cwd that doesn't exist, according to the fs it has to work with.

Either give it an explicit cwd in the glob options like this:

    const expandedFiles = await glob('/**/*', { nodir: true, cwd: '/' });

Or add the process.cwd() to memfs (but doing so will mean that it turns up in the glob results, of course).

I think this is a bug, though, tbh. It shouldn't require that the cwd exist unless you have a pattern that mounts on the cwd.

@isaacs isaacs closed this as completed in 273deed Mar 21, 2023
@isaacs
Copy link
Owner

isaacs commented Mar 21, 2023

Fixed in v9.3.1

@gawi
Copy link
Author

gawi commented Mar 22, 2023

Hi @isaacs

Thanks for the fast response. Unfortunately, I tested with 9.3.1 and I still got the same behaviour, even when ensuring cwd is /:

import { fs as memfs, vol } from 'memfs';

jest.doMock('fs', () => memfs);
jest.spyOn(process, 'cwd').mockReturnValue('/');

import { glob } from 'glob';

describe('glob', () => {
  beforeEach(() => {
    vol.fromJSON({ '/x': 'abc' }, '/');
  });

  it('should match single file with pattern', async () => {
    const expandedFiles = await glob('/*', {
      nodir: true,
      cwd: '/',
    });
    expect(expandedFiles).toStrictEqual(['/x']); //PASS
  });

  it('should match single file without pattern', async () => {
    const expandedFiles = await glob('/x', {
      nodir: true,
      cwd: '/',
    });
    expect(expandedFiles).toStrictEqual(['/x']); //FAIL
  });
});

It's somehow memfs-related but I can't put the finger on it. Works OK with the real fs.

The tests above are just for reporting the current bug. However we are using memfs to mock the filesystem in other unit tests and we expect glob to work with memfs transparently, i.e. without having to specify the fs option.

I'm not 100% sure it's not my fault. Maybe I'm not mocking fs correctly. But it's weird that glob('/*') works and not glob('/x').

@isaacs isaacs reopened this Mar 22, 2023
@isaacs
Copy link
Owner

isaacs commented Mar 22, 2023

Huh, that's weird. I did add a test using memfs, but I'm not using jest. It's possible it's mocking differently, but also possible I just missed an edge case in there. I'll see if I can reproduce, should be an easy fix if so.

@gawi
Copy link
Author

gawi commented Mar 22, 2023

In your test, you added {fs: memfs} and when I do that, the test passes. But this is not something I can do in my code (i.e. glob should work with fs, regardless of the implementation (well, assuming it's complete).

It may be that we are hitting a memfs limitation preventing glob to work correctly:

isaacs added a commit that referenced this issue Mar 22, 2023
@isaacs
Copy link
Owner

isaacs commented Mar 22, 2023

So I can reproduce it in Jest, but only in Jest. If I just run the program, and mock the fs with the mockfs, then it works.

My only guess is that somehow Jest is not mocking the fs reliably? Or not everywhere? I really don't know how Jest works, it took some doing just to figure out where I had to put the test file to get jest to run it, and then when I did, apparently they cripple console.error() so it doesn't print object dumps?

Idk, whatever is going on here, it really seems like Glob is doing the right thing, and Jest is not mocking the environment like you think it is.

@isaacs
Copy link
Owner

isaacs commented Mar 22, 2023

Oh!! Derp, it's because it needs to mock fs/promises as well as fs.

Change this line:

jest.doMock('fs', () => memfs);

to this:

jest.doMock('fs', () => memfs);
jest.doMock('fs/promises', () => memfs.promises);

And it'll work.

@isaacs isaacs closed this as completed Mar 22, 2023
@isaacs
Copy link
Owner

isaacs commented Mar 22, 2023

Not needed at the moment, but just to future proof it, probably should also add:

jest.doMock('node:fs', () => memfs);
jest.doMock('node:fs/promises', () => memfs.promises);

in case TypeScript ever decides to start putting the node: prefix on there.

@gawi
Copy link
Author

gawi commented Mar 22, 2023

Many thanks! Great job.

@isaacs
Copy link
Owner

isaacs commented Mar 22, 2023

I guess I should probably make path-scurry just load fs and get stuff from the promises object rather than loading fs/promises. I don't remember why I did it that way, there might be a good reason 😅
Thanks for the tip ;)

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

No branches or pull requests

2 participants