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

Feature/typesafe assertions #13444

Closed
wants to merge 5 commits into from

Conversation

jesrodri
Copy link

Summary

This PR resolve #13334

As requested on the issue, I added type checking on the jest-expect to avoid it from infering the type as unknown and to throw a compile error in order to speed up development.

Test plan

After changing the files, I added a type test in packages/jest-types/typetests/expect.test.ts, rebuilt the project, and ran yarn test-types and yarn jest to ensure all tests are passing.

Copy link
Contributor

@mrazauskas mrazauskas left a comment

Choose a reason for hiding this comment

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

Thanks. I still think that the approach which I mentioned in #13334 (comment) makes more sense, because it would cover more use cases:

expect("abc").toBe<string>("abc");

// all following does not work with currently proposed implementation
expect("abc").not.toBe<number>(123);
expect(Promise.resolve(123)).resolves.toBe<number>(123);
expect(Promise.resolve(123)).resolves.not.toBe<string>("abc");
expect(Promise.reject("err")).rejects.toBe<string>("err");
expect(Promise.reject("err")).rejects.not.toBe<string>("abc");

The same solution could be used for .toEqual(), .toHaveReturned(), .toHaveBeenCalled() and other matchers. It also becomes a pattern which is also easy to document.

Making one or other case work is nice, of course. I am trying to explain that looking at a larger picture makes me sceptical about currently proposed implementation.

Inverse<JestMatchers<void, T>> &
PromiseMatchers<T>;
Inverse<JestMatchers<void, unknown>> &
PromiseMatchers<unknown>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm.. This looks unexpected.

Copy link
Author

Choose a reason for hiding this comment

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

it avoids type checking in these matchers, as it would break existing tests

@jesrodri
Copy link
Author

Thanks. I still think that the approach which I mentioned in #13334 (comment) makes more sense, because it would cover more use cases:

expect("abc").toBe<string>("abc");

// all following does not work with currently proposed implementation
expect("abc").not.toBe<number>(123);
expect(Promise.resolve(123)).resolves.toBe<number>(123);
expect(Promise.resolve(123)).resolves.not.toBe<string>("abc");
expect(Promise.reject("err")).rejects.toBe<string>("err");
expect(Promise.reject("err")).rejects.not.toBe<string>("abc");

The same solution could be used for .toEqual(), .toHaveReturned(), .toHaveBeenCalled() and other matchers. It also becomes a pattern which is also easy to document.

Making one or other case work is nice, of course. I am trying to explain that looking at a larger picture makes me sceptical about currently proposed implementation.

I see your point but I agree with this comment
We could add the same typecheck to the other matchers and stick with the unkown, what do you think?

@mrazauskas
Copy link
Contributor

I understand this looks like and simple to implement and useful feature. That is just the surface. The trick is that a matcher is not just another function which should inform TypeScript about typings of its args. The matcher is special function in a way that it will check the value at runtime.

The examples below are edge cases, but they illustrate well what I try to say:

Screenshot 2022-10-18 at 23 13 31

@@ -152,7 +152,7 @@ export interface Matchers<R extends void | Promise<void>> {
* Checks that a value is what you expect. It calls `Object.is` to compare values.
* Don't use `toBe` with floating-point numbers.
*/
toBe(expected: unknown): R;
toBe(expected: E): R;

Choose a reason for hiding this comment

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

Looking better at this, I'm not sure this is wanted in all the cases. 🤔
For example.

  1. toBe, in theory, should not do type check (e.g. expect('1').toBe(1)) because there is toStrictEqual for that case.
  2. Other matches may want type check as well, for consistency, but that makes things more complex. (e.g. toHaveReturnedWith(expected?: ReturnType<E>): R)

@jesrodri
Copy link
Author

Hey guys! Hope you had a great Christmas!
Have we reached an agreement on how to proceed with this issue?

@jesrodri
Copy link
Author

Hey guys, what should be our next steps on this PR?

@mrazauskas
Copy link
Contributor

There was some discussion going on in the linked issue. Currently there is no consensus if this change is needed.

@github-actions
Copy link

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label May 17, 2023
@github-actions
Copy link

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this Jun 16, 2023
@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 Jul 17, 2023
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]: typesafe assertions
4 participants