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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: allow disabling of require-mock feature #10540

Closed
rix0rrr opened this issue Sep 21, 2020 · 11 comments
Closed

feature: allow disabling of require-mock feature #10540

rix0rrr opened this issue Sep 21, 2020 · 11 comments

Comments

@rix0rrr
Copy link

rix0rrr commented Sep 21, 2020

馃殌 Feature Proposal

A flag to allow the disabling of the (recursive) replacement of a module's require function, to improve testing speed for projects that don't need it.

Motivation

We have a sizeable JavaScript project with many files. So many files in fact that the replacement of the require function with a requireModuleOrMock function is starting to make a large difference in the runtime of our tests.

A small subset of our tests, when run with native jest 26.4.2

$ time jest -i
       24.88 real        26.00 user         3.82 sys

When putting in the following (hacky) patch to disable the require replacement:

jest-runtime/index.js
-----------------------------
    if (filename.endsWith('.test.js')) {
      // To keep the magic globals available in the test files themselves
      Object.defineProperty(localModule, 'require', {
        value: this._createRequireImplementation(localModule, options)
      });
    } else {
      // Otherwise use a stock 'require' function
      Object.defineProperty(localModule, 'require', {
        value: require('module').createRequireFromPath(filename)
      });
    }

Our runtime looks like this:

$ time jest -i
       10.05 real        10.86 user         1.14 sys

In our project, the runtime overhead of this feature seems to be about 150%. We have hundreds of test suites like this one, each of them paying this penalty.

I'd gladly trade the ability to be able to mock the importing of a module--and rewrite the handful of tests that use it--for a 60% reduction in test times.

Example

Add a new flag moduleMocking (default: true):

$ jest --no-module-mocking [...]

// or configured through jest.config.js

Pitch

Why does this feature belong in the Jest core platform?

The Jest core platform implements this functionality without offering a way to disable it. I don't want it yet I'm forced to pay for it in execution time.

@SimenB
Copy link
Member

SimenB commented Sep 21, 2020

It's not just mocking, it's also what gives transformation of source code (for both future/custom syntax and code coverage) and code execution within a sandbox (e.g. allowing you to use window when writing tests for the browser and isolation between tests). So disabling the custom require (and import) would mostly defeat the purpose of using Jest over e.g. mocha in the first place (except for the watch mode, possibly?).

To speed up tests you might wanna consider reducing the dependency tree (meaning less files to evaluate). But disabling such a core feature of Jest is not really something we wanna do.

As a last resort you can plug in your own runtime that does whatever you want. I don't think it's documented, but it's moduleLoader: https://github.com/facebook/jest/blob/d955dc0640ea710b4f7c59dac5caf540f3ac94c1/packages/jest-runner/src/runTest.ts#L113-L115

@rthreei
Copy link

rthreei commented Nov 8, 2020

@rix0rrr did you manage to alleviate the overhead?

@rthreei
Copy link

rthreei commented Dec 6, 2020

@SimenB I'm curious why jest-runtime recursively requireModuleOrMock the children of node_modules. For example, my projects depends on sequelize and when I profile a jest execution, I see that jest-runtime is requireModuleOrMock on node_modules/sequelize/lib/data-types.js (see screenshot below). There is no reasonable prospect of needing to mock that module. I admit that I may be missing something obvious; ultimately just curious if this behaviour is by design? Thanks.

Screen Shot 2020-12-06 at 11 02 22 AM

@rix0rrr
Copy link
Author

rix0rrr commented Dec 22, 2020

Oh that's a very fair point. I didn't even think that far ahead, but you are correct.

@SimenB
Copy link
Member

SimenB commented Dec 22, 2020

@rthreei yes, that's how requiring modules work - it's depth first following each require statement as we find them. In the example of sequelize I assume not every test you have needs a real sequelize, so you can mock it out and remove that entire tree from those tests. Unless you actually fire up a DB you can probably mock sequelize in every single test you have and insert some Map based simple mock to get huge speedups. Of course testing with the real module is very useful for more integration type tests, but for unit tests you often don't need it

@rix0rrr
Copy link
Author

rix0rrr commented Dec 22, 2020

@SimenB I think the suggestion is that there may be no need to use your own hooked version of require for transitive dependencies in node_modules.

The chances we'd be interested in on-the-fly-compilation, mocking, and code coverage of those modules is small.

  • Compilation, code coverage: (typically) only our own code
  • Magic globals: definitely only our own code
  • Mocking: (typically) only our first-level dependencies

There might be (potentially substantial) performance gains to be had by deferring to built-in require() for transitive dependencies, especially for libraries that have complex dependency trees.

Yes, mocking the direct dependency (thereby preventing the transitive tree from being loaded) would technically work, but it seems like a hard-to-use and blunt tool to achieve something that could be transparent. Plus, it probably reduces the value of your tests as you are now only testing up to the library boundary, and no longer testing the integration of your code and theirs.

(Of course, of those features, mocking is the most contentious one. I can imagine someone to have mocked a transitive dependency鈥攅ven though they probably shouldn't have. For backwards compatibility it should therefore be an option)

@SimenB
Copy link
Member

SimenB commented Dec 22, 2020

That's still ignoring the sandboxing I brought up in my previous comment. We need to load all code per sandbox so they can run in their own environment (DOM or Node) and so that anything that messes with globals are contained and predictable. And we cannot execute some code within one realm and some in another.

Your suggestion will make it incredibly hard to reason about what can do e.g. global.fetch = and work and what wouldn't. And you would 100% break integration with libraries as e.g. instanceof checks does not work between realms.


Jest will never add an option to purposefully break the sandbox and the guarantees that brings - that's the basis of Jest's value proposition. But if you wanna ignore those guarantees because it works for your specific use case I again recommend providing your own moduleLoader via config and overriding requireModuleOrMock (and possibly some more stuff) to just use node's native require

@rix0rrr
Copy link
Author

rix0rrr commented Dec 23, 2020

I guess I don't understand how sandboxing works well enough. 馃槄

Thanks for being patient! Got the message loud and clear.

@SimenB
Copy link
Member

SimenB commented Dec 24, 2020

That's perfectly fine, I'm sorry if my message came across as confrontational. I know not many people have poked at the vm API of node, so it's sorta hard to know what details to provide in responses. I very much want to make Jest faster and allow optimizations to occur, so please keep ideas coming. But module loading/caching is probably not the avenue to pursue as it'll mean compromising on some core features of Jest. That said, it's almost guaranteed that we could so parts of it in a better way, so it's not like it's a "don't touch topic".

@rthreei
Copy link

rthreei commented Jan 4, 2021

Thanks @SimenB !

But if you wanna ignore those guarantees because it works for your specific use case I again recommend providing your own moduleLoader via config and overriding requireModuleOrMock (and possibly some more stuff) to just use node's native require

I couldn't find documentation on moduleLoader. Is there much to it?

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants