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

Scoped module initialization / cleanup #6174

Closed
gaearon opened this issue May 12, 2018 · 16 comments · Fixed by #6701
Closed

Scoped module initialization / cleanup #6174

gaearon opened this issue May 12, 2018 · 16 comments · Fixed by #6701

Comments

@gaearon
Copy link
Contributor

gaearon commented May 12, 2018

🚀 Feature Proposal

Like jest.resetModules(), but only forgets whatever was initialized inside an explicit scope.

jest.withResetModules(() => {
  // Jest will snapshot the module registry when we enter,
  // and reset any newly loaded (!) modules when we exit.
});

Open to better naming.

Motivation

We often use jest.resetModules() in the React codebase to isolate individual tests.

However, sometimes we also use jest.resetModules() for another purpose. We have some code that's shared between different packages, but ends up being bundled (i.e. copied) into all of them after the build time. But we run Jest on the source (building would've been too slow).

As a result, with our setup there might be some accidentally shared state when running the tests that doesn't end up being shared in practice after the build. This can lead to weird bugs where tests tell us everything is okay, but it breaks in production. We've reduced this somewhat by running a subset of tests on compiled bundles, but this is still complicating some further work we need to do.

Usually we work around it like this:

React = require('react');
ReactDOM = require('react-dom');

jest.resetModules();
ReactDOMServer = require('react-dom/server');

This ensures ReactDOM and ReactDOMServer are properly isolated even if they happen to share some internal modules. The problem is that this wipes out all the modules, including React. In some cases it matters for us that React stays the same, but modules that were loaded when initializing ReactDOM or ReactDOMServer are discarded.

Example

var React;
var ReactDOM;
var ReactTestUtils;
var ReactDOMServer;

// React (and whatever it loaded) should be reused
React = require('react');

// Load some modules and then clear them from the cache
jest.withResetModules(() => {
  ReactDOM = require('react-dom');
  ReactTestUtils = require('react-dom/test-utils');
  // When we exit the scope, not only react-dom and react-dom/test-utils
  // are cleared from the cache, but also any their transitive dependencies
  // that have been loaded for the first time inside the current scope.
});

// Load other modules and then clear them from the cache
jest.withResetModules(() => {
  ReactDOMServer = require('react-dom/server');
});

Pitch

I tried my best above. Maybe if there was a way to clear individual modules that would work for me although I need to clear all recursive dependencies that haven't already been loaded earlier.

@SimenB
Copy link
Member

SimenB commented May 13, 2018

So basically, within the scope, don't put modules which weren't in the cache into it? Seems simple enough, just check before putting it in here: https://github.com/facebook/jest/blob/16de9ac22f2817899990f15d535ecfb362952141/packages/jest-runtime/src/index.js#L322, and fix the return statement here: https://github.com/facebook/jest/blob/16de9ac22f2817899990f15d535ecfb362952141/packages/jest-runtime/src/index.js#L338.

Would you expect it to care about the mocked modules registry? Or core modules?

@gaearon
Copy link
Contributor Author

gaearon commented May 13, 2018

don't put modules which weren't in the cache into it?

I think it’s a bit trickier. They should be cached while within the scope. Because a stateful module may be required by more than one module.

But they should be evicted as soon as we leave the scope.

Would you expect it to care about the mocked modules registry? Or core modules?

Not sure what the specific considerations there need to be. My mental model for this is that when we go in the scope, we “fork” the registry to a sandbox. Whatever happens there (new modules loaded, new mocks set up) only affects the code inside the scope. When we exit the scope, we restore the previous registry.

@rogeliog
Copy link
Contributor

I like this idea and I'm trying implement it and better understand the problem.

Would the proposed API be something like this?

let counter;
jest.withResetModules(() => {
  beforeEach(() => {
    counter = require('./counter');
  });
  
  it('increments by one', () => {
    counter.increment();
    counter.increment();
    expect(counter.getCount()).toBe(2)
  });
  
  it('and keeps incrementing', () => {
    expect(counter.getCount()).toBe(3)
  })
});

it('but it is cleared here', () => {
  counter = require('./counter');
  expect(counter.getCount()).toBe(1)
});

@SimenB
Copy link
Member

SimenB commented Jul 13, 2018

In my mind that won't work as beforeEach and it are executed later (and may be async), right?

@rogeliog
Copy link
Contributor

rogeliog commented Jul 13, 2018

Right! That’s exactly what I’ve been having trouble when trying to wrap my head around it... How do you imagine the API?

@SimenB
Copy link
Member

SimenB commented Jul 13, 2018

That it has to happen within a single test case (it/test). To achieve reuse the user can extract it into a function, but I don't think it's possible to make this persist between/across tests

@rogeliog
Copy link
Contributor

rogeliog commented Jul 13, 2018

Awesome! Thanks, that is how I was originally thinking about It! I might put up a PoC tomorrow

@gaearon
Copy link
Contributor Author

gaearon commented Jul 13, 2018

I was planning to call it inside beforeEach.

@SimenB
Copy link
Member

SimenB commented Jul 13, 2018

beforeEach should work I think, as it's just sugar for

it('foobar', () => {
  myBeforeEach();
});

It just can't wrap around test declarations, as long as it runs within the context of a test I think we should be good 🙂

@rogeliog
Copy link
Contributor

I've been working on a PoC today. Given that #6638 is fairly similar, which API would we prefer for resetting individual modules?

cc: @benmj do you have any thoughts around this?

@benmj
Copy link

benmj commented Jul 16, 2018

@rogeliog I actually prefer this proposal because I believe it gives a finer degree of control than in my proposed syntax. Like you point out, they are trying to solve the same problem, but this way might solve it more elegantly!

@gaearon
Copy link
Contributor Author

gaearon commented Jul 16, 2018

Btw, if there's a working proof of concept, let me know and I can give it a try in a real codebase and give my feedback.

@rogeliog
Copy link
Contributor

@gaearon here is the PoC #6701, altough it is still kind of rough.

@benmj Awesome, I'll close #6638 in favor of this

@malhotraashna
Copy link

Hey got into the use case where I would love if I am able to reset just a single module. I see a commit around 1 month ago. Can I please know when will it be available for us to use or if it is already rolled out (though I don't see anything in the documentation) ?

@SimenB
Copy link
Member

SimenB commented Aug 17, 2018

Nothing is rolled out, but you can check the pr linked above if you want :)

@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 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants