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

Feature Request: Create spyReset to only reset spies, not other mocks #7348

Open
4 tasks done
NateRadebaugh opened this issue Jan 23, 2025 · 7 comments · May be fixed by #7359
Open
4 tasks done

Feature Request: Create spyReset to only reset spies, not other mocks #7348

NateRadebaugh opened this issue Jan 23, 2025 · 7 comments · May be fixed by #7359

Comments

@NateRadebaugh
Copy link

Clear and concise description of the problem

With the new 3.0 release, "vi.spyOn Reuses Mock if Method is Already Mocked" is a breaking change without a simple migration strategy.

https://vitest.dev/guide/migration.html#vi-spyon-reuses-mock-if-method-is-already-mocked

While we can configure "mockReset" in the vitest config, or explicitly call "mockRest" in the "beforeEach", this also blows away other mocks that may have been configured.

It would be great if we could have a new spyReset to only reset spies. Right now we have a number of tests verifying a spy was called N times, and explicitly cleaning up each spy manually using onTestFinished callback is klunky at best.

Suggested solution

Create new spyReset that can be configured in vitest config, or called manually, to only reset spyOn spies instead of all mocks.

Alternative

No response

Additional context

No response

Validations

@hi-ogawa
Copy link
Contributor

Right now we have a number of tests verifying a spy was called N times, and explicitly cleaning up each spy manually using onTestFinished callback is klunky at best.

Can you show some examples? From the description alone, it's hard to feel klunky-ness.

@NateRadebaugh
Copy link
Author

I have a project with over 4,000 tests across hundreds of files written by dozens of developers. In the case of this breaking change, it caused failures in a small number of tests that were expecting spies "to have been called N times".

A workaround is to triage each failure individually and add that onTestFinished after each spy is created. However, since there are multiple spies littered throughout, it ends up making the test much more verbose.

The main concern is that there are now tests that will pass because they expect "has been called (ever)" which is now true when a previous instance of the spy was already called -- not great. In all of those cases the tests are now not doing the right thing and would need to be re-implemented to always hardcode a N calls in the "expect".

I'd add the "mockReset" flat, but it is resetting all categories of mocks, which causes even more test failures, so the minimal solution would be to expose a way to just reset mocked spies, effectively returning to the pre-3.0 results.

@sheremet-va
Copy link
Member

I think when the spy is called, the previous mock state should be reset, only the mock descriptor should be kept

@NateRadebaugh
Copy link
Author

I think when the spy is called, the previous mock state should be reset, only the mock descriptor should be kept

Yeah maybe this is all I'm looking for -- if the "spyOn" function would zero out the counters so that each spy starts at 0 after it's grabbed, that likely would restore my tests to "good" state without code changes.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Jan 25, 2025

@NateRadebaugh I just wanted to see a (pseudo) code instead of clarification with more texts 😄 A few questions I had in mind was like this.

// is "thing" declared outside of `test`?
// is this coming from `vi.mock("some-lib") + import * as thing from "some-lib"`?
let thing;

test("x", () => {
  // do you use `mockImplementation`?
  vi.spyOn(thing, "someFn").mockImplementation(() => {})

  // is "thing" re-spied in a same test?
  vi.spyOn(thing, "someFn")...
})

// or is "thing" re-spied in multiple tests?
test("y", () => {
  vi.spyOn(thing, "someFn")...
})

@NateRadebaugh
Copy link
Author

Oh sorry, yes it is generally spied on in different tests. Sometimes via a utility to get the spy instance, but ultimately the expectation is that each time the counter it reset in that new context.

So mainly "thing" is re-spied in multiple tests.

@hi-ogawa hi-ogawa linked a pull request Jan 25, 2025 that will close this issue
6 tasks
@hi-ogawa
Copy link
Contributor

hi-ogawa commented Jan 25, 2025

I'm also on the side of resetting seems more natural (and here is the PR #7359), but let me just layout out a common pattern I've seen. If "thing" is available in an outer scope, then it might be possible to use beforeEach/afterEach to ensure spy is reset between each test.

let thing;

afterEach(() => {
  if (vi.isMockFunction(thing.someFn)) {
    thing.someFn.mockReset();
  }
})

test("x", () => {
  const spy = vi.spyOn(thing, "someFn")...
  expect(spy)...
})

test("y", () => {
  const spy = vi.spyOn(thing, "someFn")...
  expect(spy)...
})

Also moving "spy` to outside:

let thing;
let spy;

beforeEach(() => {
  spy = vi.spyOn(thing, "someFn");
  return () => {
    spy.mockReset();
  }
})

test("x", () => {
  expect(spy)...
})

test("y", () => {
  expect(spy)...
})

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

Successfully merging a pull request may close this issue.

3 participants