-
-
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
feat: implement mockRestoreInitialImplementation
and restoreAllInitialMockImplementations
#9270
Conversation
Hi jeppester! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Hi @jeppester, thanks a lot for the PR! I've also come across this case a lot. What I've usually done is to move the initial implementation from the mock declaration into a Pro:
Regarding the pro: I would very much like to preserve that and not encourage a structure where it is easy for one test case to mess everything up. However it's not a deal breaker for me if there are significant upsides to the new approach. Thoughts @SimenB @thymikee @jeppester? |
…tialMockImplementations`
4f91e32
to
3050885
Compare
Codecov Report
@@ Coverage Diff @@
## master #9270 +/- ##
==========================================
+ Coverage 64.93% 64.99% +0.05%
==========================================
Files 278 278
Lines 11905 11895 -10
Branches 2935 2927 -8
==========================================
Hits 7731 7731
+ Misses 3544 3535 -9
+ Partials 630 629 -1
Continue to review full report at Codecov.
|
Hi @jeysal. Thank you for the quick feedback! After having tested the code with a manual mock, I'm not sure that there's a gap for close there. It appears to be working just fine - If you have some specific issue in your mind, please let me know, and I'll add a test for that. Here's how I've tested it (added to In import React from 'react';
export default () => <div>Real</div>; In import React from 'react';
export default jest.fn(() => <div>Mock!</div>); In jest.mock('../src/Test');
import Test from '../src/Test';
describe('mockRestoreInitialImplementation', () => {
beforeEach(() => {
Test.mockRestoreInitialImplementation();
});
it('allows a test to override the manual mock', () => {
Test.mockImplementation(() => <span>Changed for a single spec!</span>);
const tree = renderer.create(<Test />).toJSON();
expect(tree.children).toEqual(['Changed for a single spec!']);
});
it('allows a following test to still have the manual mock', () => {
const tree = renderer.create(<Test />).toJSON();
expect(tree.children).toEqual(['Mock!']);
});
});
Should I add a new |
Sorry if my message was not ideally structured, "Pro" and "Con" referred to the "what I've usually done" here |
Now I understand your feedback much better 😄 - thank you for clarifying. The pro you've mentioned is definitely a good thing, I do however think it's just as good to have a Also I think a lot of people currently start out with a bunch of mocks using If you start out that way, and you at some point discover that At least it wasn't obvious to me, and my search for a way to rewind my test-local mock changes is what eventually lead to this PR. In regards to manual mocks, I haven't used them enough to make a good judgement on how often you would need this feature. I do however think it's really nice if this feature will prevent manual mocks from having an annoying downside that you might not discover until you have a test where it makes a lot of sense to replace a mock implementation. |
I think you've convinced me, especially with the starting out and later discovering you need to restructure thing that I can very much relate to 😄 To mitigate the potential problem of forgetting to restore the initial impl at the end of a test, maybe we can just write docs that encourage a Would be interested what other maintainers think about the API in general before getting into detail review @SimenB @thymikee |
This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days. |
This PR has been waiting for feedback for soon 3 years. It would be nice if someone would finally have a look at it. What are you thoughts @SimenB and @thymikee? (If you are still maintainers) |
I'm a bit reluctant to add more APIs here as I don't see a big advantage to it, and our
You don't have to cross your fingers, you can just use |
I'm absolutely with you here. It's the one part of jest that I just can't get right 100% of the time. I can see why those names where chosen -for simplicity, but more verbose names would help a lot, like for instance For that reason I was never 100% happy about my own suggestions here, as they might at another layer of confusion.
While this is useful it adds a layer of often unnecessary complexity when testing something that might call the mock a number of times, especially if the number of times is hardly relevant to the test. That the method has built-in stacking is also counterintuitive in my opinion. I'm now thinking that we could instead have an API on mocks to set af specific implementation, then run a callback, and finally reset the mock to whatever it was. Something like mock.withImplementation(implementation, () =>
{
Logic using the temporary implementation
}) I'm not entirely sure that this idea i perfect (for instance how it would fare with multiple mocks -maybe well with decorators?). An API like that would however be very intuitive IMO, and not add to the reset/clear/restore confusion at all. What are your thoughts? |
It'd have to be async I guess, but other than that I think I like it! As you say it'd probably not work too well with multiple mocks, but then again, you can probably nest them within each other? |
It's the nesting that might become a little bit too much indenting, but I'll give the idea a go and make a new PR. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
This is an attempt to cover a commen use case (for me) that I think needs improvement:
I would currently solve this in one of two ways:
mockImplementationOnce
and cross my fingers that the mock is called exactly oncebeforeEach
that overwrite each implementation one by one. This takes up a lot of space and doesn't work very well with manual mocks.This PR causes each mock to remember it's initial implementation, and provides a way to restore it, so that:
mock.mockRestoreInitialImplementation()
*.jest.restoreAllInitialMockImplementations()
* (possibly in a beforeEach/afterEach hook)* I'm not 100% happy about these long method names, I would prefer something shorter but still precise.
I don't know for sure if the current commit is enough forjest.mock
(and manual mocks) to work, if not, point me in the right direction and I'll do my best to make it work.Update: This works with
jest.mock
, and also with manual mocks if they are jest functions - which makes a lot of sense since you would otherwise not be able to override one for a single spec.I'll be happy to add documentation, additional tests etc. should you decide to like the idea.
Test plan
I added a tests for the two added methods
mockRestoreInitialImplementation
andrestoreAllInitialMockImplementations
.