-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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]: jest.restoreAllMocks behavior changed/broken #13925
Comments
Could you check if the below sample is passing before v29.4.3 mockedItem = jest.fn().mockImplementation(() => {return 'hi'});
beforeEach(() => {
mockedItem.mockRestore();
});
describe("mockTest", () => {
test("test1", () => {
expect(mockedItem()).toEqual('hi');
});
test("test2", () => {
expect(mockedItem()).toEqual('hi');
});
}) I mean, calling |
Sorry i updated the bug it actually fails on Mac now, I had version 29.4.2 on the Mac and it was working. Once i updated the Mac it failed as well. |
Unfortunately i'm unsure how to get a complete jest version at 29.4.2, to test your example as it pull in transitive dependencies at 29.4.3 |
By setting the following dependencies my test will pass:
But the test @mrazauskas posted fails. I'm not sure why the jest team uses the "^" in the version for the transitive jest dependenciest? It Makes it very hard to downgrade to a previous version. |
Thanks for checking. What I am trying to is: beforeEach(() => {
mockedItem.mockRestore();
}); and beforeEach(() => {
jest.restoreAllMocks();
}); as well as This is not the case in 29.4.2, but is fixed in 29.4.3 via #13867. That’s the change you are seeing. |
If this is the case, which makes sense by reading the documentation,, I think restoreAllMocks has been broken for a very long time, and a lot of our developers are using it incorrectly in their tests. Maybe a breaking fix like this should be saved for the v30 release? |
not sure exactly what is happening yet, but our test suite is broken due to a mock not being called at all, and hardcoding the listed subdependencies above fixes it. There is not reset/restore involved. |
Just in case, check if you don’t have |
I agree, but it did not look breaking: #13867. In the other hand, I think swapping |
Yes, we fixed our test, after a day of trying to figure out what went wrong. Please don't backport this fix to v27 or we will have a very big problem on our hands with all our old apps that just stop building.... The thing that makes this even worse is there is no way to "pin" a. specific version of jest because of the pointless use of carets "^" throughout all the nested dependencies: #3391 Which makes trying to test against 29.4.2 almost impossible when 29.4.3 is released. |
This is enough for us to fix the issue. Still not sure it is really the same as the one raised here, though. We do have |
With const mockedItem = jest.fn().mockImplementation(() => 'tests');
test("example", () => {
mockedItem.mockRestore(); // <- should be called, because you ask for that in configuration (this did not work before 29.4.3, but is fixed now )
mockedItem(); // returns `undefined` in v29.4.3
});
}) |
I'm also having the same problem with v29.4.3 when Renovate (in this PR) tried to update to latest version of Jest in Lerna-Lite that I maintain (which by the way, is also used by Jest itself to publish new releases). I only have 1 unit test failing git-push.spec.ts which is the only unit test using EDIT
swapping to |
I found from older documentation it says "restoreAllMocks()" only affects spies, but now affects all:
|
@ghiscoding Just checked your case. All you need is The test file has many assertions around
Currently only first tests will pass and all the others will have the mock restored (with implementation removed). This is what you see running this test file. |
Prior to v29.4.3 the ES6 Class Mocks example from the official docs worked fine with
Here is the repro case: https://github.com/asapach/jest-mock-issue |
@ghiscoding Wait.. I missed |
thanks for checking, I'll wait a bit more then before making the swap to |
doing this fixed the majority of the test failures for us. The few that remained were all legit test issues that I'm surprised worked in earlier versions. |
Because previously the restoreMocks almost did nothing, but now the so it just need remove |
I'm a bit confused what the new practice should be. We have dozens of global mocks which are loaded via It seems like we have two options:
If I'm understanding correctly, when |
What does bleed exactly? The state of a mock, i.e. count of calls etc; or you are not interested to have your mocks as mocks anymore?
|
The fact that it is mocked. Let me explain it another way hopefully this will be clearer. I see there to be two different kinds of mocks/spies. There's test-specific ones and global ones.
(A simple way to think of this is at the start of every I know we could manually restore and clear each one individually, but this is very error prone and the whole reason the I had thought the previous behavior (maybe this was the bugged behavior) was closer to what I described above. Not sure if it wasn't working exactly as I had expected, but it sure seemed like it. Now though after this has been "fixed", I'm not sure how to accomplish what I described in a generic way. I either need to re-mock every global mock before every test, or make sure to restore every spy/mock that was created within a test before it ends.
So I'm guessing there's no way to restore mocks that were created during a test before the next test? So if I spied on a method, changed the implementation to trigger some special behavior for that test case, now I need to make sure that I manually restore it before that test case ends? And somehow also restore it if the test case fails? That's a big footgun. 😞 I totally understand that this may have been long-standing bugged behavior and I'm glad it's fixed, but that might mean we need to add a new config option to accomplish what people were intending to do prior which is now very difficult to achieve. |
Mocks are not scoped unfortunately. I guess, to make them scope aware they should be implemented somehow in Sounds like a big job, but perhaps it is worth opening a feature request? By the way, did you consider |
What was the actual behavior of |
Documentation did not change. It might lack some detail here and there, but in general it is correct. Start from Prior behaviour is easy to check installing Jest 28. The following example will pass on Jest 28, but will fail on current version: mockedItem = jest.fn().mockImplementation(() => {
return "hi";
});
beforeEach(() => {
// mockedItem.mockRestore();
jest.restoreAllMocks();
});
describe("mockTest", () => {
test("test1", () => {
expect(mockedItem()).toEqual("hi");
});
test("test2", () => {
expect(mockedItem()).toEqual("hi");
});
});
EDIT Another interesting detail. Try replacing |
Okay so the bugged version of |
Hm.. Not exactly. That is puzzling part. On surface it looks like it was restoring mocks created with
If it is equivalent of calling My understanding is the following: const obj = {
methodOne: () => true,
methodTwo: () => false,
};
beforeEach(() => {
jest.restoreAllMocks();
});
describe("jest.spyOn", () => {
jest.spyOn(obj, "methodOne").mockReturnValue(false);
test("test1", () => {
expect(obj.methodOne()).toEqual(true);
expect(jest.isMockFunction(obj.methodOne)).toBe(false);
});
});
describe("jest.fn", () => {
// manually assigned, hence cannot be restored
obj.methodTwo = jest.fn().mockReturnValue(false);
test("test2", () => {
// in the following assertion Jest 28 gives `false` instead of `undefined`,
// because `.mockRestore()` was not called on every mocked function although that was promised
expect(obj.methodTwo()).toEqual(undefined);
// mock is not restored, because it was manually assigned
expect(jest.isMockFunction(obj.methodTwo)).toBe(true);
});
}); Might look like const obj = {
methodOne: () => true,
methodTwo: () => false,
};
test("test1", () => {
const spy = jest.spyOn(obj, "methodOne").mockReturnValue(false);
// called once
expect(obj.methodOne()).toEqual(false);
expect(jest.isMockFunction(obj.methodOne)).toBe(true);
jest.restoreAllMocks();
// called twice
expect(obj.methodOne()).toEqual(true);
expect(obj.methodOne()).toEqual(true);
expect(jest.isMockFunction(obj.methodOne)).toBe(false);
// on Jest 28 the call count is `1`. Seems like mock was not cleared,
// this is the same count just like before calling `jest.restoreAllMocks()`
//
// current version give `0` and this was what I was fixing,
// i.e. `jest.restoreAllMocks()` was not any clearing the mocks
// and that is why it was not touching mocks created using `jest.fn()`
expect(spy.mock.calls).toHaveLength(1);
}); |
Wow! I really appreciate you trying to explain this. It begs the question, with how complex this behavior was before (which existed for a long time) and many people depended on that functionality thinking it was intentional, why was this changed in a patch with no info? This is clearly very confusing behavior. It definitely is more clear now since it's consistent, but also definitely breaking since it's notably different than what it was doing for a long time. My takeaway from all of this is that if you were using Example:Before:
After:
|
I agree that it was unfortunate that this change came out in a patch release. At the same time, it was hard to see that the impact will be so huge. Jest has many tests, which are using all kinds of mocks. Nothings was breaking. It looked like just another insignificant change. In the PR I was mostly adding new tests, see #13867 |
@mrazauskas is there any way we can help those who will be struggling with this problem after us? Maybe an amendment to the CHANGELOG which links to migration guide and calls it out as an unexpectedly breaking change. Or a pinned issue which is a summary of the problem and a few options on what someone can do going forward, etc. I checked the npm download stats and there's a significant number of users on versions prior to this change so even though it's already been out for a month, there's definitely millions of more people who will run into this issue. |
If this leads to making more of our tests better, I'm all for it. We'll be making the changes in our end even if it means in the thousands (we'll find a way to automate them). |
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. |
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. |
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. |
Version
29.4.3
Steps to reproduce
This worked prior to version 29.4.3
Expected behavior
Previous behavior test passed on all platforms.
Actual behavior
Now restoreAllMocks actually deletes all mocks
Additional context
No response
Environment
The text was updated successfully, but these errors were encountered: