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

Add the ability to specify modules to reset in jest.resetModules #6638

Closed
benmj opened this issue Jul 5, 2018 · 10 comments
Closed

Add the ability to specify modules to reset in jest.resetModules #6638

benmj opened this issue Jul 5, 2018 · 10 comments

Comments

@benmj
Copy link

benmj commented Jul 5, 2018

🚀 Feature Proposal

Allow passing variable arguments to jest.resetModules to specify which modules should be reset.

Motivation

I have several files that contain local state which I would like to reset between tests. However, I do not want to reset and re-import all of the dependencies in the test file because this seems 1) unnecessary and 2) error prone and easy to forget.

Example

Current

Here is a somewhat simplified example:

jest.mock('logger');

let logger;
let myModule;

beforeEach(() => {
    jest.clearAllMocks()
        .resetModules();

    logger = require('logger');
    myModule = require('../myModule');
});

Desired

jest.mock('logger');
const logger = require('logger');

let myModule;

beforeEach(() => {
    jest.clearAllMocks()
        .resetModules('../myModule'); // pass in the desired file to be reset

    /** only reload the module that I want to reset **/
    myModule = require('../myModule');
});

Pitch

It would remove cruft, add specificity, and give finer-grained control.

@benmj benmj changed the title Add the ability to specify modules to reset in jest.resetModules() Add the ability to specify modules to reset in jest.resetModules Jul 5, 2018
@thymikee
Copy link
Collaborator

thymikee commented Jul 5, 2018

How about filling issue template?

@rogeliog
Copy link
Contributor

rogeliog commented Jul 5, 2018

Hi, thanks for reporting!

This seems like an interesting use case. Here is the code for resetModules. https://github.com/facebook/jest/blob/master/packages/jest-runtime/src/index.js#L421-L441

My initial impression is that if we want to support this, it would not be that hard, except for a couple of things:

  • By resetting one module, do we need to invalidate any other parts of the module registry? My guess is that we shouldn't need to.
  • What should be the expected behavior for this._environment.global and envGlobal.mockClearTimers when "module path" arguments are passed?

Given that resetModules has remained untouched for 2+ years, https://github.com/facebook/jest/blame/master/packages/jest-runtime/src/index.js#L421-L441, I guess @cpojer might have better insights.

@rogeliog
Copy link
Contributor

rogeliog commented Jul 6, 2018

@benmj I understand the use case for just resetting one module rather than all of them. I'm trying to reproduce and better understand the implications of:

I also need to re-require other modules that do not contain local state.

Do you mind providing an example?

Is this similar to what you are mentioning?

import x from './withLocalState';
import y from './stateless';

beforeEach(() => {
  jest.resetModules();
});

it('some test', () => {
   // In here `y` is still the correct object right?
})

@rickhanlonii
Copy link
Member

@benmj our Feature Proposal template should be followed for all feature proposals. It has sections for questions that will come up like the ones @rogeliog brings up

I think this is a great proposal, so let us know when you update the description to follow that template and we'll reopen

@SimenB
Copy link
Member

SimenB commented Jul 6, 2018

Would #6174 fulfill the use case?

@benmj
Copy link
Author

benmj commented Jul 6, 2018

Thanks for the comments. I've updated to follow the template.

@rickhanlonii
Copy link
Member

Thanks for the update 👍

@rickhanlonii rickhanlonii reopened this Jul 6, 2018
@rogeliog
Copy link
Contributor

rogeliog commented Jul 6, 2018

I agree with @SimenB about @gaearon's proposal(#6174). Even though it uses a different abstraction, would it be correct to say that both proposals are trying to solve a same or a similar problem?

@rogeliog
Copy link
Contributor

Closing in favor of #6174

@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.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants