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

feat(expect, @jest/expect): infer type of *ReturnedWith matchers argument #13278

Merged
merged 9 commits into from
Sep 20, 2022
Merged

feat(expect, @jest/expect): infer type of *ReturnedWith matchers argument #13278

merged 9 commits into from
Sep 20, 2022

Conversation

mrazauskas
Copy link
Contributor

Summary

Similar to #13268, but *ReturnedWith matchers. I have added logic to infer types of *ReturnedWith matchers argument from the type of received expression.

Test plan

Type tests added.

@mrazauskas
Copy link
Contributor Author

@royhadad What you think about this approach? I create this PR just to illustrate the idea of restricting type of the received expression. It is not restricted directly, but TS compiler (or IDE) would show an error if a user would do this:

expect(123).toHaveLastReturnedWith(123);

Only function can be passed as received. Curious about your opinion: any down sides?

expectError(expect(jest.fn()).nthReturnedWith());
expectError(expect(jest.fn()).nthReturnedWith(2));

expectType<void>(expect(jest.fn()).toHaveNthReturnedWith(1, 'value'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works with () => unknown just like before. Infers return value, if there is one. Errors if received is not a function.

/**
* Ensures the last call to a mock function was provided specific args.
*/
lastCalledWith(...expected: Array<unknown>): R;
/**
* Ensure that the last call to a mock function has returned a specified value.
*/
lastReturnedWith(expected: unknown): R;
lastReturnedWith<U extends EnsureFunctionLike<T>>(expected: ReturnType<U>): R;
Copy link
Contributor

Choose a reason for hiding this comment

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

you are a wizard :)
I'll add this to my PR as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks (; Glad you liked it. So much easier to explain with an example.

@royhadad
Copy link
Contributor

@royhadad What you think about this approach? I create this PR just to illustrate the idea of restricting type of the received expression. It is not restricted directly, but TS compiler (or IDE) would show an error if a user would do this:

expect(123).toHaveLastReturnedWith(123);

Only function can be passed as received. Curious about your opinion: any down sides?

@mrazauskas
I see just one downside, it is a bit less backward compatible than the other approach (which is also not totally backward compatible)
Therefore I think your solution is better.

@@ -325,7 +339,7 @@ export interface Matchers<R extends void | Promise<void>> {
/**
* Ensure that a mock function has returned a specified value at least once.
*/
toReturnWith(expected: unknown): R;
toReturnWith<U extends EnsureFunctionLike<T>>(expected: ReturnType<U>): R;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change to this syntax? it works and causes less indirection

Suggested change
toReturnWith<U extends EnsureFunctionLike<T>>(expected: ReturnType<U>): R;
toReturnWith(expected: ReturnType<EnsureFunctionLike<T>>): R;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. That’s good idea. I was hoping that type argument could allow something like: expect(123).toHaveReturnedWith<() => number>(123). But it doesn’t.

royhadad added a commit to royhadad/jest that referenced this pull request Sep 18, 2022
@@ -16,7 +16,8 @@ import {
} from 'expect';
import type * as jestMatcherUtils from 'jest-matcher-utils';

type M = Matchers<void>;
type M = Matchers<void, unknown>;
type N = Matchers<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this N for? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just three test – takes two type args, but allows passing juts one and (below) errors if type args are missing.

Some time ago I added second arg without default. That was a breaking change and a user raised an issue. This test was added back in these days, but became unnecessary then @jest/expect package was introduced. Now it is needed again.

One day this will be wrapped with nice test("required type arguments", () => { /.... Good enough for now (;

};

export interface Matchers<R extends void | Promise<void>> {
type EnsureFunctionLike<T> = T extends (...args: any) => any ? T : never;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately anys are necessary for toHaveBeenCalled* matchers. See #13268 (comment)

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

awesome stuff!

@SimenB SimenB merged commit 3d626a5 into jestjs:main Sep 20, 2022
@mrazauskas
Copy link
Contributor Author

Thanks! I was wondering if toBe should be typed in similar fashion. I mean, something like:

<T>expect(actual: T).toBe(expected: T)

Simple thing. Does it make sense?

@mrazauskas mrazauskas deleted the feat-typed-ReturnedWith branch September 20, 2022 06:33
@royhadad
Copy link
Contributor

Thanks! I was wondering if toBe should be typed in similar fashion. I mean, something like:

<T>expect(actual: T).toBe(expected: T)

Simple thing. Does it make sense?

Seems about right. For weird edge cases the user can always pass any.
I see a lot of matchers where it's possible to replace the expected unknown with something more accurate

/packages/expect/src/types.ts

  toBe(expected: unknown): R;
  toContain(expected: unknown): R;
  toContainEqual(expected: unknown): R;
  toEqual(expected: unknown): R;
  toMatchObject(
    expected: Record<string, unknown> | Array<Record<string, unknown>>,
  ): R;
  toStrictEqual(expected: unknown): R;

@mrazauskas
Copy link
Contributor Author

Just wanted to say that some of these are allowed to take asymmetric type matchers and that can be complicated. Quick look at the docs, there is also an example with toBeCalledWith and asymmetric matchers (added types for testing):

class Cat {}
function getCat(fn: (c: Cat) => void) {
  return fn(new Cat());
}

{
  const mock = jest.fn<(c: Cat) => void>();
  getCat(mock);
  expect(mock).toBeCalledWith(expect.any(Cat));
}

function randomCall(fn: (n: number) => void) {
  return fn(Math.floor(Math.random() * 6 + 1));
}

{
  const mock = jest.fn<(n: number) => void>();
  randomCall(mock);
  expect(mock).toBeCalledWith(expect.any(Number));
}

First example works, but second one errors: "Argument of type 'AsymmetricMatcher_2' is not assignable to parameter of type 'number'." Ups.. That’s a regression.

@mrazauskas
Copy link
Contributor Author

Same issue with *ReturnWith. Asymmetric matchers can be used, but currently TS complains about the following snipped:

expect(jest.fn<() => {one: string; two: string}>()).toHaveReturnedWith({
  one: expect.stringContaining('a'),
  two: expect.stringContaining('x'),
});

I was trying to see if it is possible to fix toBeCalledWith. Seems to be complex or even impossible. No luck so far. Unfortunately.

@SimenB
Copy link
Member

SimenB commented Sep 28, 2022

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants