-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
chore: extend and improve type tests for jest
object
#12442
Conversation
packages/jest-mock/src/index.ts
Outdated
isMockFunction<T, Y extends Array<unknown> = Array<unknown>>( | ||
fn: (...args: Y) => T, | ||
): fn is Mock<T, Y>; | ||
isMockFunction(fn: unknown): boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first overload will infer return type and types of args if a function is passed. Second one allows to pass any value. This way types do the same job as before, but with added feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should second be isMockFunction(fn: unknown): fn is Mock<unknown, unknown>
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if hardlyMock
is any
or unknown
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are good edge cases. I added tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, and then isMockFunction(fn: unknown): fn is Mock<unknown, unknown>
still doesn't help in setting the correct type in the if
?
expectType<Mock<boolean, [a: string, b: number]>>(maybeMock); | ||
|
||
maybeMock.mockReturnValueOnce(false); | ||
expectError(maybeMock.mockReturnValueOnce(123)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TS is wrapping the function in Mock
type here. I think that’s fine, this branch will never run anyways.
if (!jest.isMockFunction(hardlyMockFn)) { | ||
expectType<string>(hardlyMockFn); | ||
} | ||
|
||
if (jest.isMockFunction(hardlyMockFn)) { | ||
expectType<string>(hardlyMockFn); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works if value is not even a function.
expectType<SpyInstance<Date, [value: string | number | Date]>>( | ||
jest.spyOn(global, 'Date'), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With @types/jest
this is failing, but passing here (;
@@ -1003,27 +1014,34 @@ export class ModuleMocker { | |||
return fn; | |||
} | |||
|
|||
spyOn<T extends {}, M extends NonFunctionPropertyNames<T>>( | |||
spyOn<T extends object, M extends NonFunctionPropertyNames<T>>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{}
as a constrain type allows any non-nullish value. See typescript-eslint/typescript-eslint#2063 (comment). Here it is almost the same as any
, because string
or boolean
are allowed. Might be better to use object
(any non-primitive value).
expectType<SpyInstance<boolean, [arg: any]>>( | ||
jest.spyOn(spiedArray as unknown as ArrayConstructor, 'isArray'), | ||
); | ||
expectError(jest.spyOn(spiedArray, 'isArray')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors, because TS does not see isArray
method without type casting.
jest
objectjest
object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay, this is awesome!
Might be worth reporting a bug |
@@ -995,7 +995,7 @@ export class ModuleMocker { | |||
isMockFunction<T, Y extends Array<unknown> = Array<unknown>>( | |||
fn: (...args: Y) => T, | |||
): fn is Mock<T, Y>; | |||
isMockFunction(fn: unknown): boolean; | |||
isMockFunction(fn: unknown): fn is Mock<unknown>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimenB Might be this is better idea. See tests. Thanks for good questions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect, thanks!
expectType<typeof jest>(jest.useRealTimers()); | ||
expectError(jest.unmock()); | ||
|
||
// Mock Functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we have these tests in jest-mock
, and just have an expectType<JestMock['fn']>(jest.fn)
and expectType<JestMock['isMockFunction']>(jest.isMockFunction)
? Gut feeling is that's more natural since at the jest.*
level it's more of an integration test and not a "unit".
Of course, doesn't really matter, but the tests would be closer to the code it tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good idea. I will move them but later, not at computer right now. Also tests for Mock Function fits better in jest-mock
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks! Let's land this in the meantime 🙂
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Resolves #10118
Resolves #10200
Summary
While working on
jest-mock
types, I noticed thatisMockFunction
andspyOn
are not covered by type tests. So I though it would be better to type tests before refactoring anything to be sure that nothing breaks.Interesting find. Apparently
api-extractor
brokeisMockFunction
types exposed fromjest-environment
. Just look at the build.d.ts
. The return type ofisMockFunction
isfn is unknown
. This is fixed. Also I added an improvement toisMockFunction
typing inspired by #10118. Covered by tests. Will leave an inline comment.The #10200 is puzzling case. It appears that all types referenced there are living inside
@types/jest
. Despite that this is great case for typing and testing.spyOn
function in Jest repo was missing one overload, but@types/jest
had it. This is the overload catching a class (e.g.,Date
class from the issue). I placed the class overload just above the one which is catching a function. That’s the only difference from@types/jest
, but it solves the issue mentioned by the user. Included in tests.Of course, I went through the test suite in
@types/jest
. Adapted what seemed useful.Tests for Mock Functions are still missing. These will follow soon with another PR.
Test plan
New and old tests should pass.