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

Mocking a module API pains #5969

Closed
davidnormo opened this issue Apr 11, 2018 · 23 comments
Closed

Mocking a module API pains #5969

davidnormo opened this issue Apr 11, 2018 · 23 comments

Comments

@davidnormo
Copy link
Contributor

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 and jest.doMock are not apparent from the names alone, I have to check the docs to remind myself. Same goes for jest.unmock and jest.dontMock. And jest.resetAllMocks and jest.restoreAllMocks. And mockFn.mockReset and mockFn.mockRestore. I think you get the picture...

jest.mock hoisting

jest.mock calls are hoisted to the top of the scope (via babel-jest). The issues I see here:

  1. 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:

    // original
    import foo from './foo'
    jest.mock('./foo')
    foo() // is this call mocked?
    // transpiled output
    jest.mock('./foo')
    import foo from './foo'
    foo() // yes! it is mocked.
  2. 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:

    let mockedFoo = jest.replace('./foo')
    let mockedBar = jest.replace('./bar').default // in this case bar is a default export
    let subject = require('./subject') // subject now depends on mocked foo and bar

    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:

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.

@SimenB
Copy link
Member

SimenB commented Apr 11, 2018

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

@nfantone
Copy link

I second this. jest mock API is convoluted, at best, and generally confusing, IMHO.

  • Do I need to reset, restore or clear a mock?
  • Why do restore work only on spies?
  • Is it a module or a mock that needs restoring?
  • Do I use genMockFromModule or plain .mock to mock a module?
  • Was it .mock that gets automatically hoisted... or is it doMock?
  • Why is there an unmock?
  • How do I partially mock a module's API?
  • Why is it not straightforward to declare mocks per test?

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 jest. I end up losing myself in the docs and/or SO more than I feel I should.

@SimenB
Copy link
Member

SimenB commented Jun 22, 2018

@mattphillips where did the issue we worked on during the summit go?

@rickhanlonii
Copy link
Member

@SimenB #6180 (comment)

@nfantone
Copy link

nfantone commented Jun 22, 2018

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.

@mattphillips
Copy link
Contributor

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.

@SimenB
Copy link
Member

SimenB commented Jun 22, 2018

Regarding reset vs restore vs clear, I think we should just give them better names. clearInvocations, restoreOriginal, resetImplementations etc. More verbose, but you don't have to check the docs

@SimenB
Copy link
Member

SimenB commented Jun 22, 2018

And more clearly separate generic mock functions from module mocks would be good

@nfantone
Copy link

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 fn()) was never a pain point. Just take a look at TC's list.

Thoughts?

@aaronjensen
Copy link
Contributor

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 mockReturnValue in a test then the next test will get that return value too if I don't manually restore the mock.

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.

__mocks__/service.js

export default {
  foo: jest.fn(() => 'original')
}

__tests__/mytest.js

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 jest.mock to be at the module level makes it more likely that jest.fn()'s will be created at the module level. Basically, every time I've used jest I've felt pushed to create mocks at the module level only to find that, like the north, mocks remember.

What I think would be fantastic is if:

  1. There was a single function to completely reset mocks to the state they were in when they were created.
  2. That function worked on jest.fn()'s created at the module level (or in manual mocks).
  3. That function was called between tests by default.

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.

@nfantone
Copy link

nfantone commented Jun 26, 2018

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:

  • All of its core depends on a foreign build tool (babel-jest).
  • All of its API is declared on the global object (never understood the reason behind neglecting imports).
  • Many non-obvious things can be happening in the background through config (like automatic reset of mocks).
  • Mocks remember state through tests.
  • Generally speaking, test order matters when using mocks.
  • Mocks can be shared trough test suites.
  • Mocks are automatically "hoisted" so what you write is not necessarily what gets executed.

The API mentioned above looks nicer, but, at least for me, this is the much bigger pain point.

☝️This. I believe none of the listed points are really covered by the new design, unfortunately.

@rickhanlonii
Copy link
Member

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: resetMocks. Turning this on will reset the mock calls and implementation (effectively re-declaring the mock) for every test. Does this fix that concern for you?

@aaronjensen
Copy link
Contributor

@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 .fn is just shorthand for .mockImplementation. I'd expect it to be "stickier" than that so when a reset happens it'd reset to what I passed it originally.

@nfantone
Copy link

nfantone commented Jun 26, 2018

@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:

  • Import functions and matchers that I want to use to test a particular piece of functionality.
  • Declare mocks that are scoped to test blocks and do not leak state outside them.
  • Run tests independent of their order.
  • Copy a single test(...) and move to it to another file with no consequences.

Unit tests are built around the concept of isolation. I think we should be applying the same ideas to the creation of tests.

@nfantone
Copy link

nfantone commented Jun 26, 2018

Contributing to the discussion, I'm not quite sold on @aaronjensen expectations on the shown examples.

  • If you declare a mock function that's scoped to the entire module, I'd expect it to behave exactly like it does now. In my opinion, if you change its internal state, that's on you.
  • If you want different mocks for different tests, create them where they belong.
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.

@aaronjensen
Copy link
Contributor

@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 jest.mock, which must (without some hoop jumping) be done at the module level. The API for jest.fn makes it appear as if you are setting the "default" state. For example:

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 beforeEach with foo.mockImplementation(() => 'original'), in which case you can/should remove the argument to jest.fn. That is obviously a contrived example as typically you'd be importing from the actual module under test and testing that (that module would, in turn, import foo).

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 jest.fn with a default implementation that survives test runs and is temporarily overridable.

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 jest.restoreAllMocks() and restoreMocks: true from working as intended: #6059 #5790 so it's possible that that is adding to the confusion.

@davidnormo
Copy link
Contributor Author

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 resetMocks config option. I don't think that will help, however the clearMocks option might. @aaronjensen does that cover the cases you've shared? If so, I think it should be enabled by default.

@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 it. The test runner already has some "magic" for each test and I think it's good and helpful if it's known and it's semantics are clear. Hence why I'm not a fan of jest.mock or the naming of various mock helper functions.

I want to work on a new API a bit, lacking time at the moment, perhaps end of this week or early next week

@aaronjensen
Copy link
Contributor

@aaronjensen does that cover the cases you've shared? If so, I think it should be enabled by default.

No, it does not. There is no setting afaict that causes jest.fn(...) to reset to the ... implemenation.

@nfantone
Copy link

nfantone commented Jun 27, 2018

@aaronjensen You raise good points by pointing out how docs make things out to be. And yes, jest.mock is an entirely different beast. Have you tried mockImplementationOnce? I think it might be a good fit for your example:

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...

@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 it.

I was actually referring to jest.fn(...), but I understand where you're coming from. Broadly speaking, I'm on board with the idea of "create once and re-use" if it is module mocking we are talking about. Unfortunately, in my experience, that kind of neat scenario doesn't come around too often. There's always this instance in which you would like to have a "default mock" implementation, but override it or make it behave differently for this one test. And jest API doesn't play very well with this kind of handling, unless you take very good care of crafting your tests and avoid falling into the pitfalls we are discussing here. My point being that "global module mocking" is only ever useful in contrived situations (http and other native modules mocking come into mind) and not generally applicable as they tend to favor state sharing and brake encapsulation.

@aaronjensen
Copy link
Contributor

Have you tried mockImplementationOnce? I think it might be a good fit for your example:

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 😄

@nfantone
Copy link

@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 mockImplementationOnce calls if your test is expected to call your mock more than once.

@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label Feb 25, 2022
@github-actions
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants