-
-
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
Mocking a module API pains #5969
Comments
It's 1 AM here, so I'll save a larger response for later, but this article explains mocks pretty well: https://medium.com/@rickhanlonii/understanding-jest-mocks-f0046c68e53c I do agree the API surface is probably larger than it has to be (and @cpojer has mentioned in the past that he'd like to revamp the API of it). Any concrete proposals (beyond the test double example)? /cc @rickhanlonii |
I second this.
I'm not saying these questions don't have proper answers. I'm making the point that it's the fact that you need to consider those answers and think about them every time you (or at least I) write a test using |
@mattphillips where did the issue we worked on during the summit go? |
That new proposal looks awesome 👍. Adding my two cents: it would actually be beneficial to users if all other functions/methods get deprecated rather than this new chainable API be built on top or sit alongside them. |
The aim of my new proposal is to add the new features alongside to get the feature out there as a minor release and then deprecate the old APIs in the next major release. |
Regarding reset vs restore vs clear, I think we should just give them better names. |
And more clearly separate generic mock functions from module mocks would be good |
Another honest concern that I can think of here is that, while this new API looks nicer and more user-friendly, does not actually address most of outlined problems regarding mocking creation. At least in my experience, writing a mock was never an issue. // This:
const isOdd = n => n % 2 != 0;
const mock = jest.fn()
.when(isOdd)
.thenReturn('🍌');
// Can be written now as:
const mock = jest.fn(n => isOdd(n) ? '🍌' : undefined); Which is arguably equally readable. Defining the contents of the mock function (the argument to Thoughts? |
Hi all, I've been using jest for quite a while now. I agree with OP that the runner itself is fantastic. That said, I've tried a number of times over the many projects I've used jest on to use jest mocks. I always run into confusion and problems. It's very possible that I'm doing something wrong and have some fundamental misunderstanding, but the current API doesn't appear to lead to the pit of success. I think that the problems I've run into boil down to a simple fact: jest mocks are not generally safe to use between tests. That is, they leak state between tests by default. As far as I can tell, they leak recordings and implementations. If I This plays poorly with some of the other features of jest like manual mocks. Here's an example of something I'm trying to do right now that led me to this issue.
export default {
foo: jest.fn(() => 'original')
}
import service from '../service'
it('original', () => {
expect(service.foo()).toEqual('original')
})
it('override', () => {
trelloService.foo.mockReturnValue('override')
expect(service.foo()).toEqual('override')
})
it('original', () => {
// none of these matter:
// jest.restoreAllMocks()
// jest.resetAllMocks()
// jest.clearAllMocks()
expect(service.foo()).toEqual('original') // FAIL
}) So, I know I'm doing something "wrong" here. I probably shouldn't be using manual mocks in these ways. However, the problem doesn't end with manual mocks. Many of the jest examples in the docs show mocks being created at the module level. This fails too: const foo = jest.fn(() => 'original')
it('original', () => {
expect(foo()).toEqual('original')
})
it('override', () => {
foo.mockReturnValue('override')
expect(foo()).toEqual('override')
})
it('original', () => {
expect(foo()).toEqual('original') // FAIL
}) Furthermore, the necessity for What I think would be fantastic is if:
I believe, if those were true, then mocks would "just work" and wouldn't leak state between tests. The API mentioned above looks nicer, but, at least for me, this is the much bigger pain point. |
Agree with most points raised by @aaronjensen above. The runner is phenomenal - but some design decisions baked into the API itself really irk me (and might not be alone here). To me, it seems as though Jest's API (not just mocks) really likes global state and hidden stuff:
☝️This. I believe none of the listed points are really covered by the new design, unfortunately. |
Thanks everyone, this is all great feedback, please keep it coming! @nfantone @aaronjensen you both mentioned the mock state being remembered across tests. We have a config option to opt-out of this behavior: |
@rickhanlonii No, it doesn't, at least not for this: const foo = jest.fn(() => 'original')
it('original', () => {
expect(foo()).toEqual('original')
})
it('override', () => {
foo.mockReturnValue('override')
expect(foo()).toEqual('override')
})
it('original', () => {
expect(foo()).toEqual('original') // FAIL
}) Likely because the arg passed to |
@rickhanlonii Also, manipulating tests behavior through config always seemed arcane to me. I usually try to do without those kind of flags/switches that determine the outcome of expected results, despite having to be more verbose in my test files or write a bunch more code. IMHO, an ideal runner would allow me to:
Unit tests are built around the concept of isolation. I think we should be applying the same ideas to the creation of tests. |
Contributing to the discussion, I'm not quite sold on @aaronjensen expectations on the shown examples.
it('original', () => {
const foo = jest.fn(() => 'original')
expect(foo()).toEqual('original')
})
it('override', () => {
const foo = jest.fn(() => 'override')
expect(foo()).toEqual('override')
})
it('original', () => {
const foo = jest.fn(() => 'original')
expect(foo()).toEqual('original') // PASSES
}) I don't think there should be any "magic" involving mocks that would change their natural variable scope. |
@nfantone I'd totally agree with you if it weren't for the rest of how jest works and what the examples in the documentation show. The example you pulled from my post is the most contrived of all of the examples. The problem is complicated by import foo from 'foo'
jest.mock('foo', () => jest.fn(() => 'original'))
it('original', () => {
expect(foo()).toEqual('original')
})
it('override', () => {
foo.mockReturnValue('override')
expect(foo()).toEqual('override')
})
it('original', () => {
expect(foo()).toEqual('original') // FAIL
}) Suffers from the exact same problems as my original example. The only way to reset it would be to add a Also, see my example with the manual mocks. How would you create different mocks for different tests in that case? It is effectively not possible to create It boils down to defaults and documentation that lead to tests sharing state rather than the pit of success which would be completely isolated test doubles. I really like the idea of keeping things at the module level as it avoids having to declare and assign lets like you would in testdouble but if the functionality doesn't actually work for it without bleeding state between tests, it really shouldn't be used in that way (or documented as such). Perhaps related, it appears that there is actually a bug preventing |
Sorry to have only just caught up now. I'm glad to see other people are having similar trouble to me. @SimenB @rickhanlonii I also agree with @nfantone and @aaronjensen, the new proposal looks cool but doesn't help these issues unfortunately. @rickhanlonii I didn't know about the @nfantone Generally, I'd want to be able to create my mocks once and use them in multiple tests. I don't want the overhead of creating the mocks for a module in every I want to work on a new API a bit, lacking time at the moment, perhaps end of this week or early next week |
No, it does not. There is no setting afaict that causes |
@aaronjensen You raise good points by pointing out how docs make things out to be. And yes, const foo = require('foo');
jest.mock('foo', () => jest.fn(() => 'default'));
it('original', () => {
expect(foo()).toEqual('default');
});
it('override', () => {
foo.mockImplementationOnce(() => 'override');
expect(foo()).toEqual('override');
});
it('original', () => {
expect(foo()).toEqual('default'); // PASSES
}); And since we are on the topic...
I was actually referring to |
Yes, though that assumes that mock is only called the once in my test. That's often a safe assumption, but it's the kind of thing I don't want to have to think about. I typically would use a "once" method if I wanted a mock to only temporarily behave a certain way in the scope of a test. I realize that my mindset is clashing with the design of jest.mocks, but that's the point 😄 |
@aaronjensen Absolutely. Clashing is where we are at. Incidentally -and I know it still suffers from having to do that mental exercise you were denouncing- but you can chain |
This issue is stale because it has been open for 1 year 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 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one. |
Firstly, I think jest is a great test runner and I appreciate all the hard work that has gone in to it. Awesome job 👍
However, the mock API does irk me. So I'd like to open this issue to see if there's something that we can do about it. Some documentation changes may help, some different APIs also may be beneficial.
Do you want to request a feature or report a bug?
Feature
What is the current API?
Here are some of my pain points with the mocking APIs:
Unclear semantics
The difference in semantics between
jest.mock
andjest.doMock
are not apparent from the names alone, I have to check the docs to remind myself. Same goes forjest.unmock
andjest.dontMock
. Andjest.resetAllMocks
andjest.restoreAllMocks
. AndmockFn.mockReset
andmockFn.mockRestore
. I think you get the picture...jest.mock
hoistingjest.mock
calls are hoisted to the top of the scope (viababel-jest
). The issues I see here:Hoisting function calls to the top of the current scope is non-standard JS, it's a language extension. Users probably won't expect it, which may cause them problems until the new behaviour is learnt. For example:
It feels like the hoisting was added to fit in with the way existing tests were written, rather than attempting to change the way people write tests to fit the mocking problem. I think TestDouble.js has an interesting solution to this problem. An obvious rip off of their API might look like:
The CommonJS (CJS) module format is more flexible for messing around with modules than ES6 modules (ESM). I would suggest keeping both the current API for ESM and introducing the new for CJS tests and in some future version suggest new tests should be written in the CJS style. This way the hoisting behaviour, as well as some of the lesser known caveats of ESM, can be avoided.
Large API surface
The API relating to mocking a module as of v22.4.2 consists of:
jest.mock
/jest.unmock
jest.doMock
/jest.dontMock
jest.setMock
jest.genMockFromModule
import * as foo from './foo'
+jest.spyOn
(See How to mock specific module function? #936)require.requireMock
jest.enableAutoMock
/jest.disableAutomock
Are all of these different methods of mocking a module required? Can a simpler, smaller API be found to cover the above cases? I don't have any immediate answers but would love to see what the community come up with.
I can probably fix most of these by using a 3rd party lib but it would be nice if Jest made some changes so I didn't have to. I willing to accept that I hold a minority opinion on these APIs and if so, it may not be worth anyones time to work on this. I thought it was worth raising all the same.
The text was updated successfully, but these errors were encountered: