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();
});