spyOn docs misleading?

[lydell]

The jest.spyOn example in the docs looks like this:

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

test('plays video', () => {
  const spy = jest.spyOn(video, 'play');
  const isPlaying = video.play();

  expect(spy).toHaveBeenCalled();
  expect(isPlaying).toBe(true);

  spy.mockReset();
  spy.mockRestore();
});

My questions is: Is that a good example? If an expectation fails, later tests that also use video will unexpectedly get a mocked .play() method.

Here's a concrete example:

const product = {
  getPrice: () => 1337,
};

describe("product", () => {
  test("getPrice 1", () => {
    const spy = jest.spyOn(product, "getPrice").mockImplementation(() => 0);
    // This expectation fails (`0 !== 42`).
    expect(product.getPrice()).toBe(42);
    spy.mockRestore();
  });

  test("getPrice 2", () => {
    // This expectation _unexpectedly_ fails, because `spy.mockRestore()` never
    // gets a chance to run.
    expect(product.getPrice()).toBe(1337);
  });
});

Above, both tests are failing and the user has to find out which test to "fix" first.

Lots of people, including me, copy stuff from docs all the time. So it is a good thing if the examples are really good :)

Unfortunately, I don't know what the best practice is here, otherwise I could have made a PR.

[SimenB]

Not sure what we should recommend here.

Personally I always add clearMocks: true to my config. resetMocks would probably not fix your use case either (https://facebook.github.io/jest/docs/en/configuration.html#resetmocks-boolean). Do we need a restoreMocksconfig? Might be perceived as surprising behavior anyways, though. At least clearing mocks at the top level.

However, if you fix your test all is good (the next test is failing because of collateral damage*), so maybe adding a comment about the gotcha would be enough? Or adding restoreMock in an afterEach?

*no idea if that sentence works in English, best translation I can come up with from Norwegian :P

[SimenB]

We could perhaps just always restore. If you want to mock every single invocation, you could add it in a beforeEach. That'd be a change in code though, and not the docs

[lydell]

I always avoid spies and mocks as long as I can because I think they are complicated by nature :)

I wrote some tests for a browser extension a few years ago, where I had to modify and assert lots of global stuff. What I did there was passing a teardown function to every test, which allowed to register a callback to run after the test regardless of whether the test failed or not (example). Using that, I guess I could write my example test like this:

  test("getPrice 1", (teardown) => {
    const spy = jest.spyOn(product, "getPrice").mockImplementation(() => 0);
    teardown(() => {
      spy.mockRestore();
    });
    expect(product.getPrice()).toBe(42);
  });

But maybe I'm missing how you're supposed to mock things. Maybe I shouldn't create one spy per test? Like, creating a few "global" spies and mocks, and reset them between all tests?

[mrchief]

I think I ran into a side effect of this behavior. First, to ensure restoration of mocks, I used afterEach block like so:

describe('my tests', () => {
  let spy

  afterEach(() => {
    if(spy) {
      spy.mockReset()
      spy.mockRestore()
    }
  })

  test("getPrice 1", () => {
    spy = jest.spyOn(product, "getPrice").mockImplementation(() => 0)
    expect(product.getPrice()).toBe(42)
  })
})

This works great until I added another test that spied as well:

describe('my tests', () => {
  let spy

  afterEach(() => {
    if(spy) {
      spy.mockReset()
      spy.mockRestore()
    }
  })

  test("getPrice 1", () => {
    spy = jest.spyOn(product, "getPrice").mockImplementation(() => 0)
    expect(product.getPrice()).toBe(42)
  })

  test("getPrice 2", () => {
    spy = jest.spyOn(product, "getPrice").mockImplementation(() => 42)
    expect(product.getPrice()).toBe(42)
  })
})

However test 2 fails and I suspect its got something to do with the fact that spy is a shared variable between both tests.

So if we can't rely on afterEach then I guess there is no reliable way to tackle this.

[borela]

@lydell It's not that mocks are complicated, you are being mislead by the const keyword here:

const product = {
  getPrice: () => 1337,
};

It's easy to think that the object is a const but the spyOn is modifying it, only the variable product can't be changed, but the object can... calling spy.mockRestore on afterEach would solve the issue but you would have to share another variable between tests.

Another and better alternative is to generate a new product object for each test.

@mrchief Your example works for me, I can't reproduce the error. Try using jest.only to check if the specific test works on its own.

[mrchief]

@borela I've refactored my tests since then but I'll try and see if I can find a reproducible sample for you.

[lydell]

@borela I know how const works. That's not the problem. What I'm wondering is how spies are supposed to be used.

[borela]

@lydell They modify the function/method.

If possible, try to create a new test object for each test, it'll prevent many gotchas:

let product

beforeEach(() => {
  product = {
    getPrice: () => 1337,
  };
})

[lydell]

try to create a new test object for each test

Ah, I see!

I guess the documentation example could be rewritten to:

const Video = require('./video');

test('plays video', () => {
  const video = new Video();
  const spy = jest.spyOn(video, 'play');
  const isPlaying = video.play();

  expect(spy).toHaveBeenCalled();
  expect(isPlaying).toBe(true);

  spy.mockReset();
  spy.mockRestore();
});

But really, when creating a new object in every time there's not even a need for resetting stuff?

const Video = require('./video');

test('plays video', () => {
  const video = new Video();
  const spy = jest.spyOn(video, 'play');
  const isPlaying = video.play();

  expect(spy).toHaveBeenCalled();
  expect(isPlaying).toBe(true);
});

I guess we might still want an example where spy.mockReset() and spy.mockRestore(), such as when testing a singleton, though.

[borela]

I agree it needs better examples.

If you are sure your tests don't modify the test object you are safe to share it, but in most cases creating a new one is extremely fast and the benefit is total isolation between tests which as you mentioned, you wouldn't need to reset anything.

Only in cases where setting up the test object is costly(test databases, etc... integration tests) that I would share the test object and consider using either mockReset or mockRestore.

mockReset has a gotcha:

Beware that mockReset will replace mockFn.mock, not just mockFn.mock.calls and mockFn.mock.instances. You should therefore avoid assigning mockFn.mock to other variables, temporary or not, to make sure you don't access stale data.

Unless I had to set up thousands of mocks which could make my tests slow by setting the mocks up on beforeEach and mockRestore on afterEach, I would avoid mockReset because someday I will be sleepy, screw up my tests and get stuck debugging what's going on because some tests might be referencing stale data.

[geoffreyyip]

What's the point in calling both spy.mockReset() and spy.mockRestore()?

I've done some digging through the issues and PRs. Seems like the bundle of reset, restore and clear configs and methods have quite a bit of overlap.

So far my understanding is this:

• clear will reset mock usage data only.
• reset will reset mock usage data and remove fake implementations.
• restore will reset mock usage data, remove fake implementations, and restore initial implementations.

In other words, clear < reset < restore.

And it also seems like restore is only used for spies, as mentioned here: #4045

So if restore does all the stuff reset does, then why bother calling spy.mockReset() before calling spy.mockRestore()?

[rickhanlonii]

@geoffreyyip yeah, you probably wouldn't want to use both

what about updating the example to:

const video = require('./video');
const spy = jest.spyOn(video, 'play');

test('plays video', () => {  
  const isPlaying = video.play();

  expect(spy).toHaveBeenCalled();
  expect(isPlaying).toBe(true);
});

This gets you going without any gotchas - it just assumes that you'll use the mock for the entire suite

[geoffreyyip]

@rickhanlonii I need to reset/restore/clear the mock between tests.

The file I'm testing has recursion. Functions depend on each other.

Imagine I had functionA() and function B(). function A() depends on function B(). I want to test function A() as a unit and check that it calls function B(). So I spy on function B().

But then I finish testing function A() and want to test function B(). How do I restore/reset/clear function B() back to its original implementation?

It feels like I'd use restore. But the docs show both reset and restore. It feels like I should only need one.

[rickhanlonii]

@geoffreyyip you probably want restore as it will restore the original implementation, but this issue tracker is not a help forum so we'll need to take this to our discord channel or StackOverflow any help questions 👌

[geoffreyyip]

@rickhanlonii With all due respect, I have to disagree. This issue is about potentially misleading documentation. Docs are part of the repo and deserve attention when there is ambiguity.

restore, reset and clear are a source of ambiguity. A few reviewers have mentioned so here: #4045

Listing both spy.mockReset() and spy.mockRestore() in the spyOn docs implies that they do different things and that both are required for a complete reboot. So far, it seems that mockRestore() does the complete reboot and mockReset() is redundant. Can't really tell.

const Video = require('./video');

test('plays video', () => {
  const video = new Video();
  const spy = jest.spyOn(video, 'play');
  const isPlaying = video.play();

  expect(spy).toHaveBeenCalled();
  expect(isPlaying).toBe(true);

  spy.mockReset();
  spy.mockRestore();
});

[rickhanlonii]

Yes I agree, sorry for the confusion. Your previous comment seemed to be a question about your specific use case and not the documentation. The docs should not say to use both, and I think we explain the difference between the three pretty well here

[geoffreyyip]

Hmm... now that you point it out, my question is about a specific use case. And there is a judgment call on whether the example should be updated to be mockReset(), mockRestore(), or neither. In my case, knowing about the mockRestore() would have made my life easier, but it might have confused somebody else.

I suppose I'm expecting Jest to teach me about spies and mocks, b/c Sinon is more configuration heavy on this front. Maybe that's expecting too much, haha.

I have some more thoughts on reset, restore and clear, but I'll post those in another issue some other time.

Thanks!

[rickhanlonii]

No sweat!

I think that for this particular location in the docs, mentioning any of the mockClear/Reset/Restore functions just begs more questions - that behavior can be outlined in a spyOn guide where there's room to explain how and when to use each

[darylszehk01]

being faked by spyOn's doc on mockReset/ mockRestore/ mockClear for an hour to figure our mockClear is what i am looking for.

[rickhanlonii]

@darylszehk01 any suggestions on how we can improve the docs now that you understand the difference?

[github-actions]

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.

[lydell]

Here’s a test I wrote recently:

describe("Init change cmd", () => {
  const originalConsoleLog = console.log;

  afterEach(() => {
    console.log = originalConsoleLog;
  });

  test("Init change cmd", async () => {
    const mockConsoleLog = jest.fn();
    console.log = mockConsoleLog;

    doStuff();

    expect(mockConsoleLog.mock.calls).toMatchInlineSnapshot();
  });
});

I’ve noticed this pattern in my tests lately: Sometimes I wrap a test in an “unnecessary” describe just so I can abuse afterEach or afterAll as a cleanup hook. (I have no intention of ever adding more tests in that describe.) I’ve done the same with Promise.reject. Those tests are admittedly a bit weird, but the point is that I didn’t want to mock console.log and Promise.reject all the time, just in a single test.

[SimenB]

Yeah, having a clean way of adding setup and teardown to a single test would be great.

But for this issue about the docs - would adding an admonition about this gotcha be enough?

[lydell]

If there is no better solution currently, then yes.

[SimenB]

@lydell in your specific example from OP, restoreAllMocks would work, like

describe("product", () => {
  afterEach(() => {
    jest.restoreAllMocks();
  });
});

Could also set that in config. I.e., just ensure that the cleanup always runs instead of within the test function itself. Ideas on how to include that info in the docs?

[lydell]

What about updating the example in the docs to include an appropriate afterEach then?

afterEach(() => {
  jest.restoreAllMocks();
});

• And some text explaining why (what would have been put in the admonition I guess). Either as regular text or as a code comment in that afterEach block.
• Also link to restoreAllMocks.

[SimenB]

Yeah, that sounds like a good idea to me.

[MiguelYax]

Hi @SimenB, I can work on it, what do you think?

[github-actions]

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.