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: Type inference in 'have been called with' parameters #15129

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

eyalroth
Copy link
Contributor

@eyalroth eyalroth commented Jun 15, 2024

Fixes #15034.

Summary

See #15034.

This feature has been attempted at the past (#13268), but introduced a breaking change (#13337) with overloaded functions, and thus was reverted.

To reach the desired typing -- especially the handling of overloaded functions vs no-args function vs the default jest.fn() type -- I eventually found this code and modified it slightly to fit better to the use-case.

Test plan

See the tests added to the PR.

Copy link

linux-foundation-easycla bot commented Jun 15, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Jun 15, 2024

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 02ea166
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/66796463e250cf0008e919f5
😎 Deploy Preview https://deploy-preview-15129--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@eyalroth eyalroth force-pushed the 15034-type-inference-parameters branch 3 times, most recently from 34acf3e to c515dc0 Compare June 18, 2024 13:45
@mrazauskas
Copy link
Contributor

mrazauskas commented Jun 19, 2024

Interesting to see some progress in this field. Some time ago I was walking more or less the same path. All was fun until I realised that asymmetric matcher can be also nested. Also that expected value and target arguments even can have different shapes:

expect(
  jestExpect(
    jest.fn<(date: Date, name?: [string, string]) => void>(),
  ).toHaveBeenCalledWith(jestExpect.any(Date), [
    jestExpect.any(String),
    jestExpect.any(String),
  ]),
).type.toBeVoid();

expect(
  jestExpect(
    jest.fn<(date: Date, name?: [string, string]) => void>(),
  ).toHaveBeenCalledWith(jestExpect.any(Date), jestExpect.any(Array)),
).type.toBeVoid();

@eyalroth
Copy link
Contributor Author

asymmetric matcher can be also nested

@mrazauskas Thanks, this is a good use/test case!

I think it's solvable with changing the current WithAsymmetricMatchers from:

type WithAsymmetricMatchers<P extends Array<any>> =
  Array<unknown> extends P ? never : {[K in keyof P]: P[K] | AsymmetricMatcher};

to:

type WithAsymmetricMatchers<P extends Array<any>> =
  Array<unknown> extends P
    ? never
    : {[K in keyof P]: DeepAsymmetricMatcher<P[K]>};

type DeepAsymmetricMatcher<T> = T extends object
  ? AsymmetricMatcher | {[K in keyof T]: DeepAsymmetricMatcher<T[K]>}
  : AsymmetricMatcher | T;

I would've preferred that all the asymmetric-matchers-producing functions will return the type that the matcher is supposed to stand for:

jestExpect.stringContaining('') // returns `string`
jestExpect.any(Number) // returns `number`

As it will allow for even better type checking. However, I'm not sure it's possible, and even if it is, it's likely going to break a lot of things.


Do you mind addressing to questions regarding this PR that I posted in the issue?

Before completing it, I wanted to know if that's the right direction.

I also had two specific questions about the PR:

  1. I added an import type from jest-mock to jest-types package.
    Should I change something in the dependencies?

  2. The type-checking of "examples and tests" (yarn typecheck:tests) fails on a few cases:
    a) In throwMatcher.test.ts, mockedMatch is defined with jest.fn and a no-args function, causing later expect to fail since it passes an argument to toHaveBeenCalledWith.
    b) In Farm.test.ts the same is happening with computeWorkerKey.
    c) In BaseWorkerPool.test.ts the type of the argument given to expect(Worker).toHaveBeenNthCalledWith() does not exactly match the type of the mock argument (WorkerOptions).

These failures seems to me like the expected behavior of this new feature, meaning that it's likely to introduce breaking changes to existing code. What are your thoughts on this?

@mrazauskas
Copy link
Contributor

  1. You should add jest-mock to devDependencies of jest-types and list it in references of packages/jest-types/tsconfig.json.
  2. As you noticed, the failures are expected. Since Jest currently is releasing alphas, it is fine to land any breaking changes. Just update the tests to make the check pass.

By the way, I updated the example in my previous comment. Finally I recalled that the biggest challenge was to handle optional arguments like name?.

@eyalroth eyalroth force-pushed the 15034-type-inference-parameters branch 2 times, most recently from c26fc92 to 604d3a7 Compare June 22, 2024 13:47
@eyalroth eyalroth marked this pull request as ready for review June 22, 2024 13:48
@eyalroth
Copy link
Contributor Author

@mrazauskas

  1. You should add jest-mock to devDependencies of jest-types and list it in references of packages/jest-types/tsconfig.json.

That didn't work for me. yarn build failed on compilation tsc.

By the way, I updated the example in my previous comment. Finally I recalled that the biggest challenge was to handle optional arguments like name?.

The code seems to handle this as well. See my updated commit with all the new test cases.

@mrazauskas
Copy link
Contributor

Ah.. Both package.json and tsconfig.json in jest-mock are listing @jest/types as a dependency. That is a mistake, because the dependency was removed in #7971 (five years ago). This can be cleaned up (in separate PR).

I see currently you are importing one type from jest-mock to expect. Is it possible to declare jest-mock as a devDependency in the expect package? Or build script is complaining?

Some time later I will try out updated types. Really interesting.

@eyalroth eyalroth force-pushed the 15034-type-inference-parameters branch from 604d3a7 to 7d3c48a Compare June 22, 2024 14:28
@eyalroth
Copy link
Contributor Author

I see currently you are importing one type from jest-mock to expect. Is it possible to declare jest-mock as a devDependency in the expect package? Or build script is complaining?

@mrazauskas That works 😃 I amended my last commit with this addtional change.

@eyalroth eyalroth force-pushed the 15034-type-inference-parameters branch from 7d3c48a to 10837e7 Compare June 22, 2024 14:29
@mrazauskas
Copy link
Contributor

If I got it right, your implementation only supports type inference for the Mock type. Only wondering why did you decide so? I mean, args of in the following (this is one examples from Jest's docs) is still inferred as Array<unknown>:

jest.spyOn(globalThis, 'setTimeout');

jest(setTimeout).toHaveBeenLastCalledWith(jest.any(Function), 1000);

Not a blocker. That is fine, of course. I just wanted to check if that is by design or this got overlooked?

@eyalroth eyalroth force-pushed the 15034-type-inference-parameters branch from 10837e7 to 43b166a Compare June 24, 2024 09:50
@eyalroth
Copy link
Contributor Author

your implementation only supports type inference for the Mock type

@mrazauskas That's an oversight :)

I adjusted the code to support both MockInstance (broader than Mock) and functions without mock type wrapping. I also added a few minimal test cases for these mock-less functions.

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.

thanks for working on this!

and thanks for reviewing @mrazauskas 🙏

@@ -31,7 +31,8 @@
"@fast-check/jest": "^1.3.0",
"@jest/test-utils": "workspace:*",
"chalk": "^4.0.0",
"immutable": "^4.0.0"
"immutable": "^4.0.0",
"jest-mock": "workspace:*"
Copy link
Member

Choose a reason for hiding this comment

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

should be in dependencies

Copy link
Member

Choose a reason for hiding this comment

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

I love that you've added a bunch of tests (thanks!), but this is getting pretty big. maybe we should have a packages/jest-types/__typetests__/expect/mocks.test.ts or something? or packages/jest-types/__typetests__/expect/toHaveBeenCalled.test.ts to group by matcher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB My thoughts exactly :)

I like the idea of having an expect/ directory with a file per function. I'll see about arranging the PR so it's clear which tests were moved and which were added.

@@ -307,3 +308,148 @@ export interface Matchers<R extends void | Promise<void>, T = unknown> {
*/
toThrow(expected?: unknown): R;
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the extensive comments 👍

@@ -20,6 +20,7 @@
- `[jest-environment-jsdom]` [**BREAKING**] Upgrade JSDOM to v22 ([#13825](https://github.com/jestjs/jest/pull/13825))
- `[@jest/environment-jsdom-abstract]` Introduce new package which abstracts over the `jsdom` environment, allowing usage of custom versions of JSDOM ([#14717](https://github.com/jestjs/jest/pull/14717))
- `[jest-environment-node]` Update jest environment with dispose symbols `Symbol` ([#14888](https://github.com/jestjs/jest/pull/14888) & [#14909](https://github.com/jestjs/jest/pull/14909))
- `[expect, @jest/expect]` [**BREAKING**] Add type inference for function parameters in `CalledWith` assertions ([#15129](https://github.com/facebook/jest/pull/15129))
Copy link
Member

Choose a reason for hiding this comment

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

why is this a breaking change? the odl type was just Function or something?

also, only expect seems to be impacted? I don't see any changes to @jest/expect at least 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB I'm thinking it will likely break some existing tests that use "have been called" functions due to incorrect typing, like it did in tests/examples in this repo. I'm not sure what are the full implications of this "tag", so obviously I will let you decide.

As for @jest/expect, I merely copied it from the last PR that tried to introduce this feature. I can obviously remove it.

Copy link
Member

Choose a reason for hiding this comment

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

It's just used as a way to catch the reader's eye.

I really should have included a migration guide thing when I started landing breaking changes, where how the breaking change might affect you would be listed 😅 I'll have to add one

@eyalroth eyalroth force-pushed the 15034-type-inference-parameters branch from 43b166a to 64be9af Compare June 24, 2024 12:19
@eyalroth eyalroth force-pushed the 15034-type-inference-parameters branch from 64be9af to 02ea166 Compare June 24, 2024 12:19
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.

Thanks!

@eyalroth
Copy link
Contributor Author

@mrazauskas @SimenB Thanks for your help on this PR. I couldn't have done it without you ❤️

Please let me know if you have more concerns.

As for the changelog, I'm keeping it for you to decide whether to include the "breaking" tag and whether @jest/expect should be mentioned as well.

@SimenB SimenB merged commit d65d4cc into jestjs:main Jun 24, 2024
83 of 84 checks passed
@mrazauskas
Copy link
Contributor

@eyalroth I was playing with updated types. All works as expected. Great job!

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 Jul 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: support type inference for function parameters
3 participants