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

fix: independently mock each instance's methods for mocked class #4564

Merged
merged 20 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion packages/vitest/src/runtime/mocker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,29 @@ export class VitestMocker {
const spyModule = this.spyModule
if (!spyModule)
throw this.createError('[vitest] `spyModule` is not defined. This is Vitest error. Please open a new issue with reproduction.')
const mock = spyModule.spyOn(newContainer, property).mockImplementation(() => undefined)

const primitives = this.primitives
const mock = spyModule.spyOn(newContainer, property).mockImplementation(function (this: any) {
// jest reference
// https://github.com/jestjs/jest/blob/2c3d2409879952157433de215ae0eee5188a4384/packages/jest-mock/src/index.ts#L678-L691

// check constructor call
if (this instanceof newContainer[property]) {
// mock each mothod of mocked class instance
for (const { key } of getAllMockableProperties(this, false, primitives)) {
const value = this[key]
const type = getType(value)
const isFunction = type.includes('Function') && typeof value === 'function'
if (isFunction) {
// TODO: ability to restore?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to restore it in jest?

Copy link
Contributor Author

@hi-ogawa hi-ogawa Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the behavior on Jest here https://stackblitz.com/edit/jest-example-z6bfab?file=mocked-class-restore.test.js
I might be using mockRestore incorrectly, but Jest doesn't seem to support such use case.
I added a similar test case for Vitest 5b60bbc and mockRestore is currently not working either but in a different way.
Would you wish to align this behavior? (or maybe make it better?)
Personally I think supporting this use case could be done separately as a nice-to-have feature.


Actually, I haven't checked what current Vitest's behavior is. I'll compare with that too later.
(EDIT: here it is https://stackblitz.com/edit/vitest-dev-vitest-hao9hx?file=test%2Fmocked-class-restore.test.ts)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I missed one important context #3438 and the rational of this mockRestore override:

mock.mockRestore = () => {
mock.mockReset()
mock.mockImplementation(() => undefined)
return mock
}

I don't think my current implementation considers global mock restore use case. I think I understand what's the expected behavior, so let me deal with this and I'll add an appropriate test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I update the code so that constructor mocking is preserved after vi.restoreAllMocks, which I think is a desired behavior aligning with #3438.

I made separate test cases mocked-class-restore-explicit.test.ts (to test mockFn.mockRestore) and mocked-class-restore-all.test.ts (to test vi.restoreAllMocks). The "explicit" scenario is still not aligned with Jest's behavior but I feel this use case is unusual and might be difficult to support.
I'd like to know what you think about this scenario. Thanks!

// mock and delegate calls to original prototype method, which should be also mocked already
const original = this[key]
// TODO: fix type error for symbol key?
spyModule.spyOn(this, key as string).mockImplementation((...args: any[]) => original.apply(this, args))
hi-ogawa marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
})
mock.mockRestore = () => {
mock.mockReset()
mock.mockImplementation(() => undefined)
Expand Down
11 changes: 11 additions & 0 deletions test/core/src/mockedE.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export const symbolFn = Symbol.for('symbolFn')

export class MockedE {
public testFn(arg: string) {
return arg.repeat(2)
}

public [symbolFn](arg: string) {
return arg.repeat(2)
}
}
115 changes: 115 additions & 0 deletions test/core/test/mocked-class.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
import { expect, test, vi } from 'vitest'

import { MockedE, symbolFn } from '../src/mockedE'

vi.mock('../src/mockedE')

test(`each instance's methods of mocked class should have independent mock function state`, () => {
const instance1 = new MockedE()
const instance2 = new MockedE()
expect(instance1).not.toBe(instance2)
expect(instance1.testFn).not.toBe(instance2.testFn)
expect(instance1.testFn).not.toBe(MockedE.prototype.testFn)
expect(vi.mocked(instance1.testFn).mock).not.toBe(vi.mocked(instance2.testFn).mock)

instance1.testFn('a')
expect(instance1.testFn).toBeCalledTimes(1)
expect(instance2.testFn).toBeCalledTimes(0)
expect(MockedE.prototype.testFn).toBeCalledTimes(1)
expect(vi.mocked(instance1.testFn).mock.calls).toMatchInlineSnapshot(`
[
[
"a",
],
]
`)
expect(vi.mocked(MockedE.prototype.testFn).mock.calls).toMatchInlineSnapshot(`
[
[
"a",
],
]
`)

instance2.testFn('b')
expect(instance1.testFn).toBeCalledTimes(1)
expect(instance2.testFn).toBeCalledTimes(1)
expect(MockedE.prototype.testFn).toBeCalledTimes(2)
expect(vi.mocked(instance2.testFn).mock.calls).toMatchInlineSnapshot(`
[
[
"b",
],
]
`)
expect(vi.mocked(MockedE.prototype.testFn).mock.calls).toMatchInlineSnapshot(`
[
[
"a",
],
[
"b",
],
]
`)

instance1.testFn('c')
expect(instance1.testFn).toBeCalledTimes(2)
expect(instance2.testFn).toBeCalledTimes(1)
expect(MockedE.prototype.testFn).toBeCalledTimes(3)
expect(vi.mocked(instance1.testFn).mock.calls).toMatchInlineSnapshot(`
[
[
"a",
],
[
"c",
],
]
`)
expect(vi.mocked(instance2.testFn).mock.calls).toMatchInlineSnapshot(`
[
[
"b",
],
]
`)
expect(vi.mocked(MockedE.prototype.testFn).mock.calls).toMatchInlineSnapshot(`
[
[
"a",
],
[
"b",
],
[
"c",
],
]
`)

// test same things for symbol key method
expect(instance1[symbolFn]).not.toBe(instance2[symbolFn])
expect(instance1[symbolFn]).not.toBe(MockedE.prototype[symbolFn])
expect(vi.mocked(instance1[symbolFn]).mock).not.toBe(vi.mocked(instance2[symbolFn]).mock)

instance1[symbolFn]('d')
expect(instance1[symbolFn]).toBeCalledTimes(1)
expect(instance2[symbolFn]).toBeCalledTimes(0)
expect(MockedE.prototype[symbolFn]).toBeCalledTimes(1)
expect(vi.mocked(instance1[symbolFn]).mock.calls).toMatchInlineSnapshot(`
[
[
"d",
],
]
`)
expect(vi.mocked(instance2[symbolFn]).mock.calls).toMatchInlineSnapshot(`[]`)
expect(vi.mocked(MockedE.prototype[symbolFn]).mock.calls).toMatchInlineSnapshot(`
[
[
"d",
],
]
`)
})
Loading