jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
44.07k stars 6.44k forks source link

Allow mocking `require.resolve` #9543

Open nicolo-ribaudo opened 4 years ago

nicolo-ribaudo commented 4 years ago

🚀 Feature Proposal

The require.resolve function is re-created for every file: this is needed so that it starts resolving from the correct location.

However, this makes it impossible to mock it: require.resolve = jest.fn() doesn't work. I propose adding an API similar to jest.mock specifically for require.resolve's behavior.

Motivation

We have different tests in @babel/core that test plugins/presets aliasing (ref). For example, we test that when someone uses @babel/env in their config Babel tries to resolve @babel/preset-env.

We currently rely on "fake" node_modules folders, however:

  1. We don't want to test node's resolution algorithm, we should only test what we are asking node to resolve.
  2. Using real FS just to check plugins/presets aliasing makes our tests slower.
  3. This approach doesn't work with Yarn PnP, because it changes the resolution behavior (and it shouldn't affect our tests).

Example

Maybe something like this?

jest.mockResolution("@babel/env", __dirname + "/fake/@babel/preset-env");

// test

jest.unmockResolution("@babel/env");

Pitch

Mocks are already handled by the Jest core platform :grin:

SimenB commented 4 years ago

Oooh, I like it! It would essentially be jest.mock but only instead of providing the inline implementation, you provide the path it should resolve to? Would allow people to easily reuse mock modules without putting a file in __mocks__/ directories as well.

Not a huge fan of needing to replicate all of .mock, doMock, unmock and dontMock though... Would overloading be confusing to people? So if a function is provided it's a inline mock, if it's a string, it's the resolution that's mocked? I generally hate overloaded APIs, but in this case I think it might make sense?

nicolo-ribaudo commented 4 years ago

Somehow I didn't realize that my proposal could be directly related to jest.mock :joy: I think that your proposed overload makes sense, and might even be more intuitive than having to use a parallel API.

Do you think that require.resolve should check if the mocked path exists? I'd say no, but then we would need something to mock a file as non-existing.

SimenB commented 4 years ago

We have jest.mock('thing', factory, {virtual: true}) if you don't want the thing you're mocking to exist. I think that should work with require.resolve as well, no?

/cc @jeysal @thymikee @cpojer thoughts on this one?

nicolo-ribaudo commented 4 years ago

Oh, I didn't know about virtual.

If no one from the team has concerns with this feature request, I could start working on an implementation next week.

cpojer commented 4 years ago

I think virtual mocks should cover your use case. Let's try that first before adding more APIs to Jest. I do not think that mocking require.resolve is desirable, there are too many ways to shoot yourself in the foot :D

nicolo-ribaudo commented 4 years ago

Since Babel internally directly calls require.resolve, I don't see how virtual mocks could cover my use case :thinking:

cpojer commented 4 years ago

You should be able to create virtual mocks to ensure that require.resolve doesn't throw. If it throws, you know Babel did something unexpected.

You can also require the module after calling require.resolve and then verify that the mock module function was called etc.

nicolo-ribaudo commented 4 years ago

It doesn't seem to work:

describe.only("virtual mocks", function() {
  it("finds preset", () => {
    try {
      const preset = () => ({});

      // also tested with "../../../node_modules/babel-preset-test-1234"
      jest.doMock("babel-preset-test-1234", () => preset, { virtual: true });

      expect(() =>
        babel.transformSync("code;", {
          presets: ["babel-preset-test-1234"],
          configFile: false,
        }),
      ).not.toThrow();
    } finally {
      jest.dontMock("babel-preset-test-1234");
    }
  });
});

reports

  ● virtual mocks › finds preset

    expect(received).not.toThrow()

    Error name:    "Error"
    Error message: "Cannot resolve module 'babel-preset-test-1234' from paths ['/home/nicolo/Documenti/dev/babel/babel'] from /home/nicolo/Documenti/dev/babel/babel/packages/babel-core/lib/config/files/plugins.js"

          89 | 
          90 |   try {
        > 91 |     return require.resolve(standardizedName, {
             |                    ^
          92 |       paths: [dirname]
          93 |     });
          94 |   } catch (e) {

          at Runtime._requireResolve (node_modules/jest-runtime/build/index.js:892:13)
          at resolveStandardizedName (packages/babel-core/lib/config/files/plugins.js:91:20)
          at resolvePreset (packages/babel-core/lib/config/files/plugins.js:48:10)
          at loadPreset (packages/babel-core/lib/config/files/plugins.js:67:20)
          at createDescriptor (packages/babel-core/lib/config/config-descriptors.js:154:9)
          at packages/babel-core/lib/config/config-descriptors.js:109:50
              at Array.map (<anonymous>)
          at createDescriptors (packages/babel-core/lib/config/config-descriptors.js:109:29)
          at createPresetDescriptors (packages/babel-core/lib/config/config-descriptors.js:101:10)

      14 |           configFile: false,
      15 |         }),
    > 16 |       ).not.toThrow();
         |             ^
      17 |     } finally {
      18 |       jest.dontMock("babel-preset-test-1234");
      19 |     }

      at Object.<anonymous> (packages/babel-core/test/resolution.js:16:13)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 40 skipped, 41 total
Snapshots:   0 total
Time:        0.634s

It doesn't work because the jest replacement for require checks for mocks, but the replacement for require.resolve doesn't.

Also, I don't know what require.resolve would be currently expected to return with mocks, because it must return a string which we aren't providing to jest.mock.

cpojer commented 4 years ago

Thanks for trying it out and checking. That's a good observation that I missed. Now the question is: should require.resolve consider mocks and give a fake temporary file-path or should we allow mocking require.resolve?

nicolo-ribaudo commented 4 years ago

If require.resolve("foo") returned something like "/tmp/jest-virtual-mocks/node_modules/foo" if an only if foo is a mocked module, then it would probably cover our use case. However, Jest should make calls to require("/tmp/jest-virtual-mocks/node_modules/foo") return the mocked implementation otherwise require(require.resolve("foo")) won't work.

I still think that explicitly specifying to Jest what to return when require.resolveing a mocked module would be better. Maybe something like this?

jest.mock("foo", () => {}, { virtual: true, resolve: "/tmp/node_modules/foo" });

Also, I realized that my original proposal doesn't cover a use case we would need in half of our resolution-related tests: checking that the correct error message is thrown when require.resolveing a module that doesn't exist (ref). This currently already works, but it checks every folder up to the root of the real FS because it uses the "real" algorithm.

It probably would need a separate option, which makes a mock throw whenever require or require.resolve are called:

jest.mock("foo", () => {}, { virtual: true, missing: true });
cpojer commented 4 years ago

I'm a bit hesitant of adding new APIs to Jest because you are running into a very limited use case that not many people are encountering. I think I would prefer simply making it so you can overwrite require.resolve, then you can do whatever you like with it.

SimenB commented 4 years ago

What I like about expanding jests API is that you can have reusable mocks (possibly distributed through npm) without __mocks__ directory. Also, it's weird that jest.mock affects require but not require.resolve.

nicolo-ribaudo commented 4 years ago

I'm a bit hesitant of adding new APIs to Jest because you are running into a very limited use case that not many people are encountering.

I agree with this sentiment. However, I don't think that mine is a limited use case:

However, I didn't find anyone else asking how to mark a mocked module as not existing. Let's keep the discussion focused only on the original proposal. Also, it could probably be worked around with @SimenB's original proposal (https://github.com/facebook/jest/issues/9543#issuecomment-583879010).

What I like about expanding jests API is that you can have reusable mocks (possibly distributed through npm) without __mocks__ directory.

This is something I personally don't need, but I definitely see how it would be useful for the rest of the community!

Also, it's weird that jest.mock affects require but not require.resolve.

:100: The require and require.resolve node APIs are designed to be symmetrical (I think that require itself uses require.resolve internally). Currently, this simmetry is broken when using jest.mock.

SimenB commented 4 years ago

I'm down with adding a resolve option next to virtual. I think almost all of the code changes needed in jest-runtime will be the exact same regardless of where the signal comes from (a separate option, string as factory or an entirely new API, or something else we haven't thought of), and the bulk of the work will remain the same no matter what API we land on.

One challenge with overloading is that babel-plugin-jest does some checking on the type provided. You're probably the most qualified person around to make the changes to the Babel plugin, though 😛 https://github.com/facebook/jest/blob/47b8aaec0a6bb80b96f7641d68c76587d86abe23/packages/babel-plugin-jest-hoist/src/index.ts#L97-L102 Should probably be noted though that we might need to validate resolve in a similar way, since hoisting will make it so that e.g. jest.mock('./thing', () => {}, {resolve: findSomeUrl()}) can behave differently. Right now we force you to have everything in scope of the mock factory, be a global or be named mock*Something*. Something like it probably makes sense for whatever we end up adding to allow mocking require.resolve as well

nicolo-ribaudo commented 4 years ago

Sure, I will prepare a draft PR in the next days! I think that I will try to implement both the resolve option and the jest.fn(name, path) overload, because I'm not sure yet about which approach could be more valuable, or would add more complexity to the code. We can then decide which is better by looking at the real implementations/tests :wink:

nicolo-ribaudo commented 4 years ago

Hey, sorry all for the super long time without any response.

I tried implementing it (I wanted to implement both the proposals, to check which one was better), but didn't manage to figure out how the require/resolve code works.

If anyone wants to implement it it would be highly appreciated, but I can still work on updating the Babel plugin if needed.

TheHumbleJester commented 4 years ago

Hey guys ! I know that's not exactly what you're looking for, but here's my workaround to be able to "mock" require.resolve. There's an interesting feature in jest allowing you to define your own module resolver: https://jestjs.io/docs/en/configuration#resolver-string

Having that in mind, here is, basically, what I've done:

let mapping = {};

// Looks for "module-resolution.json" files in all the __tests__ directories glob .sync(${__dirname}/../../packages/**/src/**/__tests__/modules-resolution.json) .forEach((file) => { // For each of them, merges them in the "mapping" object mapping = { ...mapping, ...require(file) }; });

function resolver(path, options) { // If the path corresponds to a key in the mapping object, returns the fakely resolved path // otherwise it calls the Jest's default resolver return mapping[path] || options.defaultResolver(path, options); }

module.exports = resolver;

- Then, I configured it in my jest config file:
```js
module.exports = {
  roots: ['<rootDir>'],
  testMatch: ['<rootDir>/packages/**/__tests__/**/*.test.js'],
  collectCoverageFrom: ['packages/**/src/**/*.js', '!packages/**/src/__tests__/**'],
  resolver: '<rootDir>/test-utils/resolver',
};

This do the job for me so far and I think that, starting from this example, we could do something more complex but more developer friendly ! Of course, that would be even better if this feature could be directly included in jest ! Anyway, I hope this will help some of you 😉

vvanpo commented 4 years ago

I'm a bit hesitant of adding new APIs to Jest because you are running into a very limited use case that not many people are encountering. I think I would prefer simply making it so you can overwrite require.resolve, then you can do whatever you like with it.

@cpojer I think any project that has optional peer dependencies and alters its behaviours based on the existence of those dependencies likely cannot implement full test coverage because require.resolve is not mockable. Such codebases almost certainly have their optional peer dependencies installed as development dependencies, so any require.resolve() call wrapped in a try ... catch will have an untested codepath in the catch block, unless they resort to something hacky like temporarily removing the module from the filesystem, which likely requires consecutive runs of jest and therefore coverage-merging.

mattvb91 commented 3 years ago

Would love to have this functionality too!

My usecase specifically is checking if a directory exists for example require.resolve("@packageName/some-dir/")

SimenB commented 3 years ago

The feature has been accepted, so no need to try to convince us 🙂 PR very much welcome!

kylemh commented 3 years ago

Hey @SimenB

I read

We have jest.mock('thing', factory, {virtual: true}) if you don't want the thing you're mocking to exist. I think that should work with require.resolve as well, no?

Am I to understand that jest.mock('some-node_module-dep', () => {}, { virtual: true }) should make 'some-node_module-dep' fail to resolve in my source code? I'm trying to add tests to assert that we warn users when we're trying to resolve a peerDep they don't have, and that doesn't seem to be working :(

SimenB commented 3 years ago

Correct, that's part of what this issue is about. jest.mock does not affect require.resolve

github-actions[bot] commented 2 years ago

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

t1m0thyj commented 2 years ago

This limitation still exists in Jest where it's not possible to mock require.resolve or require.resolve.paths. I vote to keep this issue open.

JounQin commented 2 years ago

I'm trying to use moduleNameMapper for require.resolve, and not quite sure to understand why it does not work.

Tom910 commented 1 year ago

I found relative problem with @fastify/swagger-ui. This library is using "require.resolve('./static/logo.svg')" and as result test broken with moduleNameMapper configuration