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): support type inference for function parame… #13268

Merged
merged 24 commits into from
Sep 20, 2022

Conversation

royhadad
Copy link
Contributor

@royhadad royhadad commented Sep 15, 2022

…ters in CalledWith assertions

Summary

Closes #13267
Let me know where I missed something, I'm a first-time contributor 😊

Test plan

To test it out, either use expect or jestExpect with toBeCalledWith, you should get automatic type inference for the function arguments assertion.
Any ideas for a way to add an automated test?

Screen Shot 2022-09-15 at 17 55 51

@facebook-github-bot
Copy link
Contributor

Hi @royhadad!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@mrazauskas
Copy link
Contributor

mrazauskas commented Sep 15, 2022

Would be nice to add type tests here:

https://github.com/facebook/jest/blob/e703e6ed1861595d56e25a764642da5a32b32c5a/packages/jest-types/__typetests__/expect.test.ts#L200-L205

Also note that aliased matchers will be removed with the next released. Perhaps it would make sense to add the new test targeting only non-aliased matcher?

EDIT: to run the type tests, you have to build the library and then yarn test-types.

@SimenB
Copy link
Member

SimenB commented Sep 15, 2022

Thanks! Can you add some type tests?

https://github.com/facebook/jest/tree/main/packages/expect/__typetests__

You run them with yarn test-types in the root

@@ -118,32 +118,39 @@ export interface AsymmetricMatchers {
stringMatching(sample: string | RegExp): AsymmetricMatcher;
}

type PromiseMatchers = {
// if T is a function, return its parameters array type, otherwise return an unknown array type
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm.. In this use case, can it be not a function? I mean, isn’t it that toHaveBeenCalledWith should be used with functions. Or?

I just look at toMatchSnapshot again thinking that constraining types for the matcher would make sense here too. Do I miss something?

https://github.com/facebook/jest/blob/e703e6ed1861595d56e25a764642da5a32b32c5a/packages/jest-snapshot/src/types.ts#L38-L39

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrazauskas
Yes, toHaveBeenCalledWith should only be called on functions, but we don't have a way to constrain it at that level (because the Matcher also handles non-function matchers. I tried doing something on this branch but it is not really possible.

So to sum up, yes, in this use case it can still not be a function (and will likely fail the test at run time)

CHANGELOG.md Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
@royhadad royhadad changed the title feat(expect, jest-expect): support type inference for function parame… feat(expect, @jest/expect): support type inference for function parame… Sep 16, 2022
@royhadad
Copy link
Contributor Author

royhadad commented Sep 18, 2022

@mrazauskas

I've added a single automated test (for now), but can't make it pass on my local machine.
I've run yarn build, yarn test-types and got the following failure:

Screen Shot 2022-09-18 at 16 07 35

Any idea what I'm missing?

@mrazauskas
Copy link
Contributor

Do you see an error in IDE?

expectError is silencing errors, if you run yarn test-types, but IDE still should show these errors. The errors are silenced by codes and these are hardcode. Not the best solution. Only that if you see an error in IDE, this message means that I should add another error code to tsd-lite (the type testing library). That’s why I ask.

@royhadad
Copy link
Contributor Author

Do you see an error in IDE?

expectError is silencing errors, if you run yarn test-types, but IDE still should show these errors. The errors are silenced by codes and these are hardcode. Not the best solution. Only that if you see an error in IDE, this message means that I should add another error code to tsd-lite (the type testing library). That’s why I ask.

@mrazauskas
I don't get an IDE error for my test (line 225), but there are errors for other tests (line 212).
Either it's a problem with the expectError function OR I haven't built my project properly (and therefore using the previous version).
Maybe you can try running it on your machine? ❤️
Screen Shot 2022-09-18 at 16 51 25

packages/expect/src/types.ts Outdated Show resolved Hide resolved
packages/jest-types/__typetests__/expect.test.ts Outdated Show resolved Hide resolved
royhadad and others added 2 commits September 18, 2022 17:15
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
CHANGELOG.md Outdated
@@ -2,9 +2,10 @@

### Features

- `[feat(@jest/environment, jest-runtime)]` Allow `jest.requireActual` and `jest.requireMock` to take a type argument ([#13253](https://github.com/facebook/jest/pull/13253))
Copy link
Member

Choose a reason for hiding this comment

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

Wanna send a separate PR for these to changes? That way CI will trigger when you push without me having to press a button 🙂

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'm not totally sure what you mean, maybe suggest a change?

Copy link
Member

Choose a reason for hiding this comment

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

Extract the removal of feat from the changelog to a separate PR

Copy link
Contributor Author

@royhadad royhadad Sep 18, 2022

Choose a reason for hiding this comment

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

I tried doing that and the workflow did not automatically run: 8969360

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I meant make those changes in a separate PR which I'll merge. Then approval is no longer needed (once you have a merged contribution)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@royhadad royhadad requested review from SimenB and mrazauskas and removed request for mrazauskas and SimenB September 18, 2022 15:01
@royhadad
Copy link
Contributor Author

@SimenB @mrazauskas
Made the appropriate fixes and test cases 😃

royhadad and others added 2 commits September 18, 2022 18:38
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
@royhadad
Copy link
Contributor Author

One test is failing due to a timeout, I don't think it has anything to do with my PR

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.

nice!

@@ -217,6 +217,38 @@ expectType<void>(expect(jest.fn()).toBeCalledWith('value', 123));
expectType<void>(expect(jest.fn()).toHaveBeenCalledWith());
expectType<void>(expect(jest.fn()).toHaveBeenCalledWith(123));
expectType<void>(expect(jest.fn()).toHaveBeenCalledWith(123, 'value'));
expectType<void>(expect(jest.fn()).toHaveBeenCalledWith(123, 'value'));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expectType<void>(expect(jest.fn()).toHaveBeenCalledWith(123, 'value'));

same as above, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch! This was not on purpose :)

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@royhadad
Copy link
Contributor Author

Fixed 😊
Is there anything else blocking this pull request from merging? How are we handling backward compatibility?

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.

This is awesome, thanks!

As for backwards compat, I think this is fine. Some tests might break if you typecheck them, but if you typecheck your tests you probably want that.

If people complain we can always revert and re-land in the next major

@SimenB SimenB merged commit d0f1b0a into jestjs:main Sep 20, 2022
@SimenB
Copy link
Member

SimenB commented Sep 28, 2022

@glasser
Copy link

glasser commented Sep 29, 2022

It looks like this PR is causing typechecking failures for us. I see @SimenB says that this may be expected for buggy tests, but my tests don't look obviously wrong to me.

CI failure: https://app.circleci.com/pipelines/github/apollographql/apollo-server/17609/workflows/e4053b3a-7b24-457e-be76-5f4786248ddc/jobs/139043
One of the locations of the failure: https://github.com/apollographql/apollo-server/blob/5f33280ecca4818ec4f226cd97779e2ca43bdbbe/packages/integration-testsuite/src/httpServerTests.ts#L2379-L2388

It seems like the issues specifically relate to expect.objectContaining: it's complaining that the thing returned by that function doesn't have all the expected fields, but that's the whole point of objectContaining?

@mrazauskas
Copy link
Contributor

See #13339.

Indeed the asymmetric matchers like expect.objectContaining got broken, because of buggy typings. All is fine with your tests. Thanks for reporting.

@SimenB
Copy link
Member

SimenB commented Sep 30, 2022

@glasser
Copy link

glasser commented Sep 30, 2022

Thanks, that release fixes our tests! This does seem like a cool feature if it gets polished.

@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 31, 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.

[Feature]: support type inference for function parameters expect(someFunction).toBeCalledWith()
5 participants