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 1 commit
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
Prev Previous commit
Next Next commit
fix: support symbol key method
  • Loading branch information
hi-ogawa committed Nov 22, 2023
commit 3936c76093fcfd95eafdfc4f9a6a78baad9e48af
20 changes: 13 additions & 7 deletions packages/vitest/src/runtime/mocker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -328,20 +328,26 @@ 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(function (this: any) {

const primitives = this.primitives
const mock = spyModule.spyOn(newContainer, property).mockImplementation(function (this: unknown) {
// 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]) {
const instance = this as any
// mock each class instance's method
// TODO: more sound way to loop prorotype methods?
for (const key in this) {
if (typeof this[key] === 'function') {
for (const { key } of getAllMockableProperties(instance, false, primitives)) {
const value = instance[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 by delegating calls to original prototype method, which should be also mocked already
const original = this[key]
const _mockInner = spyModule.spyOn(this, key).mockImplementation((...args: any[]) => original.apply(this, args))
// mock and delegate calls to original prototype method, which should be also mocked already
const original = instance[key]
// TODO: fix type error for symbol key?
spyModule.spyOn(instance, key as string).mockImplementation((...args: any[]) => original.apply(this, args))
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions test/core/src/mockedE.ts
Original file line number Diff line number Diff line change
@@ -1,5 +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)
}
}
27 changes: 26 additions & 1 deletion test/core/test/mocked-class.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { expect, test, vi } from 'vitest'

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

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

Expand Down Expand Up @@ -87,4 +87,29 @@ test(`each instance's methods of mocked class should have independent mock funct
],
]
`)

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