-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
[jest] add type inference #31094
[jest] add type inference #31094
Changes from 2 commits
f8c68f6
5b6edfa
3fdb22e
b337410
3236add
fd07752
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,8 +17,9 @@ | |
// Martin Hochel <https://github.com/hotell> | ||
// Sebastian Sebald <https://github.com/sebald> | ||
// Andy <https://github.com/andys8> | ||
// Antoine Brault <https://github.com/antoinebrault> | ||
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped | ||
// TypeScript Version: 2.3 | ||
// TypeScript Version: 3.0 | ||
|
||
declare var beforeAll: jest.Lifecycle; | ||
declare var beforeEach: jest.Lifecycle; | ||
|
@@ -35,6 +36,8 @@ declare var xtest: jest.It; | |
|
||
declare const expect: jest.Expect; | ||
|
||
type ArgsType<T> = T extends (...args: infer A) => any ? A : never; | ||
|
||
interface NodeRequire { | ||
/** | ||
* Returns the actual module instead of a mock, bypassing all checks on | ||
|
@@ -110,19 +113,15 @@ declare namespace jest { | |
/** | ||
* Creates a mock function. Optionally takes a mock implementation. | ||
*/ | ||
function fn<T extends {}>(implementation: (...args: any[]) => T): Mock<T>; | ||
/** | ||
* Creates a mock function. Optionally takes a mock implementation. | ||
*/ | ||
function fn<T>(implementation?: (...args: any[]) => any): Mock<T>; | ||
function fn<T, Y extends any[]>(implementation?: (...args: Y) => T): Mock<T, Y>; | ||
/** | ||
* Use the automatic mocking system to generate a mocked version of the given module. | ||
*/ | ||
function genMockFromModule<T>(moduleName: string): T; | ||
/** | ||
* Returns whether the given function is a mock function. | ||
*/ | ||
function isMockFunction(fn: any): fn is Mock<any>; | ||
function isMockFunction(fn: any): fn is Mock<any, any[]>; | ||
/** | ||
* Mocks a module with an auto-mocked version when it is being required. | ||
*/ | ||
|
@@ -198,7 +197,9 @@ declare namespace jest { | |
* spy.mockRestore(); | ||
* }); | ||
*/ | ||
function spyOn<T extends {}, M extends keyof T>(object: T, method: M, accessType?: 'get' | 'set'): SpyInstance<T[M]>; | ||
function spyOn<T extends {}, M extends keyof T>(object: T, method: M, accessType: 'get'): SpyInstance<T[M], []>; | ||
function spyOn<T extends {}, M extends keyof T>(object: T, method: M, accessType: 'set'): SpyInstance<void, [T[M]]>; | ||
antoinebrault marked this conversation as resolved.
Show resolved
Hide resolved
|
||
function spyOn<T extends {}, M extends keyof T>(object: T, method: M): T[M] extends (...args: any[]) => any ? SpyInstance<ReturnType<T[M]>, ArgsType<T[M]>> : never; | ||
antoinebrault marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* Indicates that the module system should never return a mocked version of | ||
* the specified module from require() (e.g. that it should always return the real module). | ||
|
@@ -762,12 +763,12 @@ declare namespace jest { | |
new (...args: any[]): any; | ||
} | ||
|
||
interface Mock<T = {}> extends Function, MockInstance<T> { | ||
new (...args: any[]): T; | ||
(...args: any[]): any; | ||
interface Mock<T, Y extends any[]> extends Function, MockInstance<T, Y> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please restore the default type parameter and add one for Y. I don't think making these required is worth the breaking change, and trying to "force people" to use stronger types in tests is very opinionated. A large percentage of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know that is it very opinionated, but shouldn't we, as a community, enforce best practices? I strongly believe that testing should be done the safest way possible, and "any" type should be avoided at all cost. I don't agree that stronger mock types will only help for spyOn. There is a couple of example I changed in jest-tests (mockContextVoid, mockContextString, spy3Mock, spy4) where the typing of arguments would be lost and potentially mislead people. If you don't care about tuple generics, you can still use <any, any[]> or without typing : const spy = spyOn(obj, 'method');
const mock = jest.fn();
const mockContextVoid = jest.fn().mock;
const mock2 = spy.mockImplementation(() => null); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please we want to move away from Arguments: We need this contract for robust integrated tests (no This fix will put jasmine behind compared to jest and will raise the adoption of the library. https://palantir.github.io/tslint/rules/no-any/
Jest needs to guide devs to do it right and also produce quality tests. fallbacking on If you plan to really go with this default value. I see one way to still force it by config: @rickhanlonii can we have your opinion here, please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@jwbay can you explain this with an example of what you can do now and what this change would require? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
To a point. We need to balance the benefits and costs considering the pain of breaking changes. As a data point, it's possible to make a package so annoyingly type-safe the change is reverted: #26813
You are absolutely entitled to that strong opinion in your projects. The issue here is that this change impacts a large number of developers across the world, from enterprise teams to personal projects (this package gets 2 million downloads per week), for value that people will question. This change as-is isn't really just opinionated—it's prescriptive.
That's fair, I didn't qualify my statement correctly. spyOn is the only way to infer types from existing functions. Everything else will either need explicit types, a runtime function to extract them via inference, or some kind of conditional type to convert
That's part of the problem with this. The <any, any[]> is often just noise. Reading noise sucks and writing noise really sucks. Here are a couple examples: test('foo', () => {
someFunction();
// type params are just noise here
(foo as jest.Mock<any, any[]>).mockClear();
someFunction();
expect(fooMock).toHaveBeenCalledTimes(1);
});
test('foo', () => {
// more noise
(foo as jest.Mock<any, any[]>).mockName('MyMock');
const rendered = render(<Component foo={foo} />);
expect(rendered).toMatchSnapshot();
}); I think providing defaults for the cases where the type parameters truly don't matter is a reasonable ask. How about a compromise of defaulting to I'd really like to get the thoughts of other maintainers here considering the impact: @NoHomey, @asvetliakov, @alexjoverm, @epicallan, @ikatyang, @wsmd, @JamieMason, @douglasduteil, @ahnpnl, @JoshuaKGoldberg, @UselessPickles, @r3nya, @Hotell, @sebald, @andys8 As a note for impact, things like this are going to start popping up in codebases consuming this change: // this function only touches one or two properties of a larger interface
mockFoo.mockReturnValue({
x: 42
} as Partial<RealReturnType> as RealReturnType);
mockFoo.mockImplementation((a, _b, _c, _iReallyDontCare, _makeItStop) => {
return Promise.resolve(a)
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated description with breaking change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Although the rest of the changes look excellent (really looking forward to being able to strictly type spies/mocks!), that one point of requiring explicit type definitions on variables is not a good thing for many users. IMO we should default to Agreed that it's often better practice to explicitly type things, but not all users are interested in best practices, or the same best practices. Some users are just interested in having tests work out of the box, and any added pain given to them by TypeScript is a negative - even if it gives better type checking. A few examples:
Speaking more generally: what are "best practices"? If your goals are to have completely strict type safety, then sure, enforcing explicit types helps enforce your personal best practices. But not everybody has that same goal. Some just want to write code quickly, be able to define a few non-object constructs such as interfaces, and get some improved level of IDE features. IMO we shouldn't sacrifice one use case (stricter typing) for others (quicker development, easier TS migration). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand all your arguments and i think it's case by case, team by team, project by project.
So i am calling a crazy idea. ^^ Plan B: to keep There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i did one test that show me a pure inconsistency if the plan is to add only default value so i am fall backing to my plan b ^^ in the meantime There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
plan c: I use it as follow: this can be done at the project level not impacting all the devs around the world (migrating from js to ts ;) ) |
||
new (...args: Y): T; | ||
(...args: Y): T; | ||
} | ||
|
||
interface SpyInstance<T = {}> extends MockInstance<T> {} | ||
interface SpyInstance<T, Y extends any[]> extends MockInstance<T, Y> {} | ||
|
||
/** | ||
* Wrap module with mock definitions | ||
|
@@ -781,14 +782,14 @@ declare namespace jest { | |
* myApi.myApiMethod.mockImplementation(() => "test"); | ||
*/ | ||
type Mocked<T> = { | ||
[P in keyof T]: T[P] & MockInstance<T[P]>; | ||
[P in keyof T]: T[P] & MockInstance<T[P], ArgsType<T[P]>>; | ||
} & T; | ||
|
||
interface MockInstance<T> { | ||
interface MockInstance<T, Y extends any[]> { | ||
/** Returns the mock name string set by calling `mockFn.mockName(value)`. */ | ||
getMockName(): string; | ||
/** Provides access to the mock's metadata */ | ||
mock: MockContext<T>; | ||
mock: MockContext<T, Y>; | ||
/** | ||
* Resets all information stored in the mockFn.mock.calls and mockFn.mock.instances arrays. | ||
* | ||
|
@@ -828,7 +829,7 @@ declare namespace jest { | |
* | ||
* Note: `jest.fn(implementation)` is a shorthand for `jest.fn().mockImplementation(implementation)`. | ||
*/ | ||
mockImplementation(fn?: (...args: any[]) => any): Mock<T>; | ||
mockImplementation(fn?: (...args: Y) => T): Mock<T, Y>; | ||
/** | ||
* Accepts a function that will be used as an implementation of the mock for one call to the mocked function. | ||
* Can be chained so that multiple function calls produce different results. | ||
|
@@ -844,9 +845,9 @@ declare namespace jest { | |
* | ||
* myMockFn((err, val) => console.log(val)); // false | ||
*/ | ||
mockImplementationOnce(fn: (...args: any[]) => any): Mock<T>; | ||
mockImplementationOnce(fn: (...args: Y) => T): Mock<T, Y>; | ||
/** Sets the name of the mock`. */ | ||
mockName(name: string): Mock<T>; | ||
mockName(name: string): Mock<T, Y>; | ||
/** | ||
* Just a simple sugar function for: | ||
* | ||
|
@@ -856,7 +857,7 @@ declare namespace jest { | |
* return this; | ||
* }); | ||
*/ | ||
mockReturnThis(): Mock<T>; | ||
mockReturnThis(): Mock<T, Y>; | ||
/** | ||
* Accepts a value that will be returned whenever the mock function is called. | ||
* | ||
|
@@ -868,7 +869,7 @@ declare namespace jest { | |
* mock.mockReturnValue(43); | ||
* mock(); // 43 | ||
*/ | ||
mockReturnValue(value: any): Mock<T>; | ||
mockReturnValue(value: T): Mock<T, Y>; | ||
/** | ||
* Accepts a value that will be returned for one call to the mock function. Can be chained so that | ||
* successive calls to the mock function return different values. When there are no more | ||
|
@@ -885,11 +886,11 @@ declare namespace jest { | |
* console.log(myMockFn(), myMockFn(), myMockFn(), myMockFn()); | ||
* | ||
*/ | ||
mockReturnValueOnce(value: any): Mock<T>; | ||
mockReturnValueOnce(value: T): Mock<T, Y>; | ||
/** | ||
* Simple sugar function for: `jest.fn().mockImplementation(() => Promise.resolve(value));` | ||
*/ | ||
mockResolvedValue(value: any): Mock<T>; | ||
mockResolvedValue(value: T | PromiseLike<T>): Mock<Promise<T>, Y>; | ||
/** | ||
* Simple sugar function for: `jest.fn().mockImplementationOnce(() => Promise.resolve(value));` | ||
* | ||
|
@@ -909,7 +910,7 @@ declare namespace jest { | |
* }); | ||
* | ||
*/ | ||
mockResolvedValueOnce(value: any): Mock<T>; | ||
mockResolvedValueOnce(value: T | PromiseLike<T>): Mock<Promise<T>, Y>; | ||
/** | ||
* Simple sugar function for: `jest.fn().mockImplementation(() => Promise.reject(value));` | ||
* | ||
|
@@ -921,7 +922,7 @@ declare namespace jest { | |
* await asyncMock(); // throws "Async error" | ||
* }); | ||
*/ | ||
mockRejectedValue(value: any): Mock<T>; | ||
mockRejectedValue(value: any): Mock<Promise<T>, Y>; | ||
|
||
/** | ||
* Simple sugar function for: `jest.fn().mockImplementationOnce(() => Promise.reject(value));` | ||
|
@@ -939,26 +940,22 @@ declare namespace jest { | |
* }); | ||
* | ||
*/ | ||
mockRejectedValueOnce(value: any): Mock<T>; | ||
mockRejectedValueOnce(value: any): Mock<Promise<T>, Y>; | ||
} | ||
|
||
/** | ||
* Represents the result of a single call to a mock function. | ||
*/ | ||
interface MockResult { | ||
type: 'return' | 'throw' | 'incomplete'; | ||
/** | ||
* True if the function threw. | ||
* False if the function returned. | ||
*/ | ||
isThrow: boolean; | ||
antoinebrault marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* The value that was either thrown or returned by the function. | ||
* The value that was either thrown or returned by the function, or undefined if type = 'incomplete' | ||
*/ | ||
value: any; | ||
} | ||
|
||
interface MockContext<T> { | ||
calls: any[][]; | ||
interface MockContext<T, Y extends any[]> { | ||
calls: Y[]; | ||
instances: T[]; | ||
invocationCallOrder: number[]; | ||
/** | ||
|
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.
You need
Y extends never[]
, otherwise this refuses functions that havenever
parameters (rare, but possible).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.
Actually, is there any reason why this is not just
function fn<T extends (...args: never[]) => unknown>(implementation?: T): Mock<T>
, withMock
changed to hold a function generic instead?It makes declaring the
Mock
methods more annoying but it makes using.fn
a lot easier when you want to make sure your mock function conforms to the interface of a function that already exists. Just calljest.fn<typeof myfunc>(/* inline function will even get contextual params/return types */)
.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.
Also, the default type (when implementation is not specified) should be
(...args: any[]) => void
. Perhaps it should be on a 0 argument overload instead of an optional argument to avoid misuse.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.
This is false. I just updated the tests and it doesn't error.
Because this would be a breaking change. By adding a new generic with a default value, existing code will not be affected.
It should not return void. This should compile:
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.
Hmm... I need to brush up on the
...args: never[]
thing, then. I had seen it as a recommendation from the TS team somewhere a few months ago.As for the second one, that's because you're specifying a mock implementation after the
.fn
call but... that is supported API because JS is weird like that 😵If you use
.fn()
without also adding a more specific.mock*
it'll returnvoid
by default. But currently methods cannot alter the type of anything on theirthis
...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.
@antoinebrault Can you help me understand how to use the new:
I understand the T, but what is expected for the Y. After updating to this version, several of the mockImplementations, are throwing typing issues.
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.
@josecolella Y is the arguments types of the implementation function.
When mocking implementations, you have to use the same arguments types as the original function, but arguments are optional.
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.
@antoinebrault What about for mockImplementation of classes? As of now it's complaining that I need to implement all the methods within the class, when in reality only one method needs to be mocked.
With new types this no longer works. Do you have an example of how to use this new version with class mock implementations?
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.
you can use Partial for that use case.
jest.Mocked<Partial<Type>>
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.
@antoinebrault Thanks for your explanation and examples.