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

[Bug]: Module mocks should be called again for each test #14080

Closed
julienw opened this issue Apr 17, 2023 · 17 comments
Closed

[Bug]: Module mocks should be called again for each test #14080

julienw opened this issue Apr 17, 2023 · 17 comments

Comments

@julienw
Copy link

julienw commented Apr 17, 2023

Version

29.5.0

Steps to reproduce

  1. Open this repl: https://replit.com/@felash/jest-issue
  2. click Run
  3. Notice that the test fails

For convenience, here is the full code:

jest.mock("./add", () => jest.fn(() => 42));

const add = require('./add');

describe('add', () => {
  it('should add two numbers', () => {
    expect(add(1, 2)).toBe(42);
  });
});

with

  resetMocks: true,

Expected behavior

The test should run fine.

Actual behavior

In this test we create a module mock using jest.mock where we use a jest.fn() mock with an implementation. But we also configured resetMocks in the jest config file. As a result the jest.fn() mock is reset before the test is run (because it's run in beforeEach).

I think that the mock function should be run again before each test, after mocks have been reset.

Additional context

I filed this in #7573 some time ago but that issue got closed for inactivity.

Note: because of #9896, in our project we call resetAllMocks in afterEach. Then this issue would appear at the second test only. But it would still happen.

A workaround is:

jest.mock("./add")
const add = require('./add');

beforeEach((() => {
  add.mockImplementation(() => 42);
});

describe('add', () => {
  it('should add two numbers', () => {
    expect(add(1, 2)).toBe(42);
  });
});

Environment

System:
    OS: Linux 5.15 Ubuntu 20.04.2 LTS (Focal Fossa)
    CPU: (8) x64 AMD EPYC 7B12
  Binaries:
    Node: 16.18.1 - /nix/store/0l5yh0rdzibk8arj9c8gzy1570jkc3vf-nodejs-16.18.1/bin/node
    npm: 8.19.2 - /nix/store/0l5yh0rdzibk8arj9c8gzy1570jkc3vf-nodejs-16.18.1/bin/npm
  npmPackages:
    jest: ^29.5.0 => 29.5.0
@mrazauskas
Copy link
Contributor

I think that everything here works as expected in most predictable manner.

The "workaround" sample does not look like a workaround. I think this is the actual way you should setup mocks if you prefer to have resetMocks: true in Jest config.


As you noticed Jest is resetting mocks using beforeEach hook, but that one runs before beforeEach from a test file. This way a user is able to setup mock as needed for each test case. Jest does right thing for you, isn’t it?


From your #9896 (comment)

I don't mind doing it in beforeEach, but I believe it should be done in afterEach as well.

Why? Not sure what would that change?

reseting mocks blindly.

Exactly! The resetMocks: true is blindly resetting all mocks before each test case. This is good, because that’s predictable.

For instance, if mocks would be reset after each test case the behaviour would be unpredictable. As you noticed "this issue would appear at the second test only." Mark that second test with .only and suddenly it will pass. How to debug, how to find the problem?

@julienw
Copy link
Author

julienw commented Apr 17, 2023

I think that everything here works as expected in most predictable manner.

Predictable, yes.
Desirable, my opinion is: no.

The "workaround" sample does not look like a workaround. I think this is the actual way you should setup mocks if you prefer to have resetMocks: true in Jest config.

As you noticed Jest is resetting mocks using beforeEach hook, but that one runs before beforeEach from a test file. This way a user is able to setup mock as needed for each test case. Jest does right thing for you, isn’t it?

If Jest was running the mock factory again as I'm suggesting here, this would still be possible.

From your #9896 (comment)

I don't mind doing it in beforeEach, but I believe it should be done in afterEach as well.

Why? Not sure what would that change?

reseting mocks blindly.

Exactly! The resetMocks: true is blindly resetting all mocks before each test case. This is good, because that’s predictable.

For instance, if mocks would be reset after each test case the behaviour would be unpredictable. As you noticed "this issue would appear at the second test only." Mark that second test with .only and suddenly it will pass. How to debug, how to find the problem?

This issue isn't about that, let's focus on the problem described in this issue please.

@mrazauskas
Copy link
Contributor

mrazauskas commented Apr 17, 2023

If Jest was running the mock factory again as I'm suggesting here, this would still be possible.

Interesting, but what if someone might want to op-out?

Currently beforeEach allows easy opt-in without creating new APIs. It would be possible to add jest.reMock() next to jest.mock() and jest.doMock(). Is it worth it? Nor sure. Especially that using beforeEach the code reads well and does the right thing already.


Sorry for bringing in comments from another issue. I misunderstood, it felt like you are opening this issue, because that one got closed.

@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label May 17, 2023
@julienw
Copy link
Author

julienw commented May 18, 2023

up

@github-actions github-actions bot removed the Stale label May 18, 2023
@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Jun 17, 2023
@julienw
Copy link
Author

julienw commented Jun 18, 2023

up

@github-actions github-actions bot removed the Stale label Jun 18, 2023
@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Jul 18, 2023
@julienw
Copy link
Author

julienw commented Jul 18, 2023

up

@github-actions github-actions bot removed the Stale label Jul 18, 2023
@karmeleon
Copy link

I'm running into this too after upgrading jest-mock from 29.1.2 to 29.6.1. I think it's caused by #13867 , since we were able to call restoreAllMocks() and resetAllMocks() with mocked modules prior to the upgrade.

To me, when I run jest.mock('some-module', someFactory), I want to declare that a module should be mocked with the given factory throughout the whole test suite. Resetting the state of all the mocks should clear out the call counts and such, but it shouldn't undo the fact that I told Jest to mock that module in that way.

Looking at it another way, I can't think of a situation where the current behavior is desirable. The resetMocks option effectively makes running jest.mock with a factory pointless, and turning it off to use resetAllMocks() explicitly instead means that you can only keep a module mocked for as long as you can handle holding off needing to reset all the other non-module mocks.

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

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

@github-actions github-actions bot added the Stale label Sep 1, 2023
@julienw
Copy link
Author

julienw commented Sep 8, 2023

up

@julienw
Copy link
Author

julienw commented Sep 8, 2023

I'm running into this too after upgrading jest-mock from 29.1.2 to 29.6.1. I think it's caused by #13867 , since we were able to call restoreAllMocks() and resetAllMocks() with mocked modules prior to the upgrade.

To me, when I run jest.mock('some-module', someFactory), I want to declare that a module should be mocked with the given factory throughout the whole test suite. Resetting the state of all the mocks should clear out the call counts and such, but it shouldn't undo the fact that I told Jest to mock that module in that way.

Looking at it another way, I can't think of a situation where the current behavior is desirable. The resetMocks option effectively makes running jest.mock with a factory pointless, and turning it off to use resetAllMocks() explicitly instead means that you can only keep a module mocked for as long as you can handle holding off needing to reset all the other non-module mocks.

What you mention is IMO another issue which has been fixed in 29.6.3, see #14429.

@github-actions github-actions bot removed the Stale label Sep 8, 2023
@github-actions
Copy link

github-actions bot commented Oct 8, 2023

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

@github-actions github-actions bot added the Stale label Oct 8, 2023
Copy link

github-actions bot commented Nov 7, 2023

This issue was closed because it has been stalled for 30 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 7, 2023
@github-actions github-actions bot removed the Stale label Nov 7, 2023
@julienw
Copy link
Author

julienw commented Nov 7, 2023

Can it be reopened? These bots are painful.

Copy link

github-actions bot commented Dec 8, 2023

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 Dec 8, 2023
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