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

Add type tests for all expect matchers #11949

Merged
merged 5 commits into from Oct 15, 2021

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Oct 11, 2021

Summary

@SimenB As promised, here is an attempt to add type tests for all expect matchers.

Of course, I was looking through the code while working with typings.

Found one issue with the typings of <...>CalledWith matchers. Previous typings allowed to use the matchers without any arguments. I improved these, but strangely mlh-tsd is refusing to understand the errors (expecting an error, getting that error, "but found none"):

Screenshot 2021-10-11 at 11 03 19

Test plan

This PR is mainly adding missing tests. All improvements are covered by tests too.

packages/expect/src/types.ts Outdated Show resolved Hide resolved
packages/expect/src/types.ts Outdated Show resolved Hide resolved
packages/expect/src/types.ts Outdated Show resolved Hide resolved
@SimenB
Copy link
Collaborator

@SimenB SimenB commented Oct 11, 2021

While I don't mind the combined PR, we could split out the JSDoc changes into its own PR to reduce the diff here to be focused on the type changes?

@mrazauskas
Copy link
Contributor Author

@mrazauskas mrazauskas commented Oct 11, 2021

While I don't mind the combined PR, we could split out the JSDoc changes into its own PR to reduce the diff here to be focused on the type changes?

Sure. Also will be easier to work on changes.

@mrazauskas mrazauskas force-pushed the fix-expect-add-types-test branch from 3b3c47c to cad8d6f Compare Oct 11, 2021
@@ -431,7 +431,7 @@ const expectExport = expect as Expect;

declare namespace expectExport {
export type MatcherState = JestMatcherState;
export interface Matchers<R> extends MatcherInterface<R> {}
export interface Matchers<R, T> extends MatcherInterface<R, T> {}
Copy link
Contributor Author

@mrazauskas mrazauskas Oct 11, 2021

Choose a reason for hiding this comment

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

T is consumed in toMatchSnapshot and toMatchInlineSnapshot. Might be useful somewhere else too.

propertyMatchers: Partial<T>,
snapshotName?: string,
): R;
toMatchSnapshot(hint?: string): R;
Copy link
Contributor Author

@mrazauskas mrazauskas Oct 11, 2021

Choose a reason for hiding this comment

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

Less specific overloads goes first for better errors.


expectType<void>(expect(jest.fn()).toBeCalledWith('value'));
expectType<void>(expect(jest.fn()).toBeCalledWith('value', 123));
// expectError(expect(jest.fn()).toBeCalledWith());
Copy link
Contributor Author

@mrazauskas mrazauskas Oct 11, 2021

Choose a reason for hiding this comment

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

All comment out do not work with mlh-tsd at the moment. All is as expected with type checks inside VS Code, but tests fail:
136754163-8611b77b-7949-4f6b-895d-6ecbe95b26cd

@mrazauskas mrazauskas force-pushed the fix-expect-add-types-test branch from cad8d6f to e336b25 Compare Oct 11, 2021
@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Oct 11, 2021

Codecov Report

Merging #11949 (9e9454c) into main (46c9c13) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #11949   +/-   ##
=======================================
  Coverage   68.76%   68.76%           
=======================================
  Files         322      322           
  Lines       16620    16618    -2     
  Branches     4795     4794    -1     
=======================================
- Hits        11429    11428    -1     
+ Misses       5159     5158    -1     
  Partials       32       32           
Impacted Files Coverage Δ
packages/expect/src/index.ts 91.78% <ø> (ø)
packages/expect/src/jestMatchersObject.ts 85.71% <100.00%> (+1.93%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46c9c13...9e9454c. Read the comment docs.

@@ -182,7 +170,7 @@ export interface Matchers<R> {
* Ensure that an object is an instance of a class.
* This matcher uses `instanceof` underneath.
*/
toBeInstanceOf(expected: Function): R;
toBeInstanceOf(expected: unknown): R;
Copy link
Contributor Author

@mrazauskas mrazauskas Oct 11, 2021

Choose a reason for hiding this comment

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

Somehow unknown here feels like better idea, or?

Copy link

@Smrtnyk Smrtnyk Oct 14, 2021

Choose a reason for hiding this comment

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

I honestly do not understand why it was Function before
since one can pass anything in it I agree with unknown

toHaveBeenNthCalledWith(nthCall: number, ...args: Array<unknown>): R;
toHaveBeenNthCalledWith(
nth: number,
...expected: [unknown, ...Array<unknown>]
Copy link

@Smrtnyk Smrtnyk Oct 14, 2021

Choose a reason for hiding this comment

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

why is ...expected: [unknown, ...Array<unknown>] better than ...expected: unknown[]? isn't it the same?

Copy link
Collaborator

@SimenB SimenB Oct 14, 2021

Choose a reason for hiding this comment

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

it ensures length of array is >= 1 instead of >= 0

@mrazauskas
Copy link
Contributor Author

@mrazauskas mrazauskas commented Oct 15, 2021

Fixed the above mentioned tsd bug SamVerschueren/tsd#126

Waiting for SamVerschueren/tsd#127

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Oct 15, 2021

@mrazauskas can we remove (or comment out) the assertions that needs a newer version of tsd so we can land the other stuff?

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Oct 15, 2021

Ergh, CI is happy... Can I land this? 😀

@mrazauskas mrazauskas changed the title [wip]: add type tests for all expect matchers Add type tests for all expect matchers Oct 15, 2021
@mrazauskas mrazauskas marked this pull request as ready for review Oct 15, 2021
@mrazauskas
Copy link
Contributor Author

@mrazauskas mrazauskas commented Oct 15, 2021

Looked through. I think it is good to land. Not at computer right now, can’t add change log

@SimenB SimenB merged commit ae1f04b into facebook:main Oct 15, 2021
30 checks passed
@mrazauskas mrazauskas deleted the fix-expect-add-types-test branch Oct 15, 2021
@github-actions
Copy link

@github-actions github-actions bot commented Nov 15, 2021

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 Nov 15, 2021
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.

None yet

5 participants