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 #13334

Closed
DetachHead opened this issue Sep 29, 2022 · 36 comments
Closed

[Feature]: typesafe assertions #13334

DetachHead opened this issue Sep 29, 2022 · 36 comments

Comments

@DetachHead
Copy link

🚀 Feature Proposal

this will always fail, but since typescript infers the type as unknown, there's no compile error:

expect('foo').toBe(1) // no error
expect<string>('foo').toBe(1) // no error

Motivation

it will allow you to identify errors in your tests much earlier, speeding up development 🚀🚀🚀

Example

// the new and improved expect 🚀:
declare const expect: <T>(actual: T) => { toBe: (expected: T) => void}

// usage:
expect('foo').toBe(1) // TS2345: Argument of type 'number' is not assignable to parameter of type 'string'.

Pitch

the current expect types make using typescript with jest pretty much useless. i think if jest is going to support typescript, it should support basic functionality like this

@mrazauskas
Copy link
Contributor

Fix it. Just open a PR (;

@SimenB
Copy link
Member

SimenB commented Sep 29, 2022

Would love toBe to work as type predicate 👍 Not sure how feasible it is, tho

@jesrodri
Copy link

Hi, can I try this?

@SimenB
Copy link
Member

SimenB commented Oct 11, 2022

Yeah, go for it!

@jesrodri
Copy link

Hey guys!

I'm facing a problem here.
I've made the following changes in /jest/packages/expect/src/types.ts:

export type Expect = { <T = unknown>(actual: T): Matchers<void, T> & Inverse<Matchers<void, T>> & PromiseMatchers<T>; } & BaseExpect & AsymmetricMatchers & Inverse<Omit<AsymmetricMatchers, 'any' | 'anything'>>;
...
export interface Matchers<R extends void | Promise<void>, E = unknown>
...
toBe(expected: E): R;

and when I test in a local app using
import { type Expect } from '.../jest/packages/expect/build/index';
declare const expect:Expect;
...
expect(1).toBe('1');

it works fine and TypeScript gives me the error Argument of type 'string' is not assignable to parameter of type 'number'.ts(2345) as expected.

But when I add the same assertion to the /jest/examples/typescript/__tests__/calc.test.ts file, TypeScript keeps its current behavior and doesn't identify the type error. It keeps getting unkown as the type of Matchers:
(alias) expect<number>(actual: number): Matchers<void, unknown> & SnapshotMatchers<void, number> & Inverse<JestMatchers<void, number>> & PromiseMatchers<...>

Does anyone know what could be happening here?

@jesrodri
Copy link

jesrodri commented Oct 14, 2022

here is git diff from /jest/packages/expect/src/types.ts

index e2fc97916..d7d6f7cfd 100644
--- a/packages/expect/src/types.ts
+++ b/packages/expect/src/types.ts
@@ -94,9 +94,9 @@ export interface BaseExpect {
 }
 
 export type Expect = {
-  <T = unknown>(actual: T): Matchers<void> &
-    Inverse<Matchers<void>> &
-    PromiseMatchers;
+  <T = unknown>(actual: T): Matchers<void, T> &
+    Inverse<Matchers<void, T>> &
+    PromiseMatchers<T>;
 } & BaseExpect &
   AsymmetricMatchers &
   Inverse<Omit<AsymmetricMatchers, 'any' | 'anything'>>;
@@ -118,20 +118,20 @@ export interface AsymmetricMatchers {
   stringMatching(sample: string | RegExp): AsymmetricMatcher;
 }
 
-type PromiseMatchers = {
+type PromiseMatchers<E = unknown> = {
   /**
    * Unwraps the reason of a rejected promise so any other matcher can be chained.
    * If the promise is fulfilled the assertion fails.
    */
-  rejects: Matchers<Promise<void>> & Inverse<Matchers<Promise<void>>>;
+  rejects: Matchers<Promise<void>, E> & Inverse<Matchers<Promise<void>, E>>;
   /**
    * Unwraps the value of a fulfilled promise so any other matcher can be chained.
    * If the promise is rejected the assertion fails.
    */
-  resolves: Matchers<Promise<void>> & Inverse<Matchers<Promise<void>>>;
+  resolves: Matchers<Promise<void>, E> & Inverse<Matchers<Promise<void>, E>>;
 };
 
-export interface Matchers<R extends void | Promise<void>> {
+export interface Matchers<R extends void | Promise<void>, E = unknown> {
   /**
    * Ensures the last call to a mock function was provided specific args.
    */
@@ -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;
   /**
    * Ensures that a mock function is called.
    */

@mrazauskas
Copy link
Contributor

Most probably you should build the repo and all will work.

By the way, to test the change you should write type tests. The type tests for matchers live here: packages/jest-types/typetests/expect.test.ts

To run them, build the repo and run yarn test-types. Remember to rebuild after making changes.


One use case which looks problematic: expect('abc').not.toBe(123). I mean, the .not modifier is a problem. It would be possible to pass unknown to the inverse matchers as a type of received value. That solves the case, but why not to provide a mechanism of type safety for the inverse matchers as well?

For example, here is what @types/jest does: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/3fa56f312fa13d9546ba1745026ee009991bd3c8/types/jest/index.d.ts#L813

Usage example:

expect('abc').toBe<string>('abc');
expect('abc').not.toBe<number>(123);

This approach works for many other matchers too (just look through @types/jest code).

Is it really useful? Is it worth to spend time implementing? These are other questions.

@DetachHead
Copy link
Author

Is it really useful? Is it worth to spend time implementing? These are other questions.

i can't really think of a use case where you'd want to explicitly specify the generics for non-matching types as i can't see how it provides any additional safety. imo in those cases using unknown is fine

@github-actions
Copy link

This issue is stale because it has been open 30 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 Nov 16, 2022
@DetachHead
Copy link
Author

not stale

@mrazauskas
Copy link
Contributor

Similar change was made in @types/jest some time ago and had to be reverted. For discussion see DefinitelyTyped/DefinitelyTyped#62128

@DetachHead
Copy link
Author

to address some of the points mentioned in that discussion:

  • i think many of the issues people have with it can be easily solved by explicitly widening/narrowing the type:
    expect("foo").toBe<number | string>(1)
    // or you could just "as" easily cast it:
    expect("foo").toBe(1 as number | string)
    i'm convinced this is more of an edge case and that people who want to compare non-matching types should just do this. the benefits of having type safety for the other 90% of cases far outweighs it imo.
  • [@types/jest] Infer matcher types based on expect DefinitelyTyped/DefinitelyTyped#47020 changed more than just the toBe method. it also changed toHaveBeenCalledWith and toHaveReturnedWith which caused issues with overloads due to limitations in typescript so i agree that those particular methods should probably stay as is until a better solution can be implemented

however if this really is too big of a breaking change, i would suggest making a new typesafe api while keeping the old one, like what dart is doing

@mrazauskas
Copy link
Contributor

mrazauskas commented Dec 16, 2022

One can use type arguments for extra type safety as well. I think we could agree that there is no consensus on this feature and that is why it is not implemented.

Would you agree that vscode-jest satisfies both sides? I mean, it also provides immediate in-editor response, but does more than typecheck.

EDIT That is right that the DT's PR was implementing more than just .toBe(), but it is also right that there is not need to revert the whole PR.

@DetachHead
Copy link
Author

Would you agree that vscode-jest satisfies both sides?

do you mean how it runs the tests on each change? idk, i often turn that off because it can be resource-intensive to be running tests constantly. i always think it's better to see type errors before having to run any code, which is kinda the point of using typescript in the first place

it also wouldn't pick up cases where for example a function that works properly at runtime could have the incorrect type at compiletime

// foo.d.ts
export declare const doThing: () => number // wrong type
// foo.js
export const doThing = () => 'asdf'
// foo.spec.ts
import { doThing } from './foo'
test('doThing', () => {
    expect(doThing()).toBe('asdf') // no compile error or runtime error
})

@mrazauskas
Copy link
Contributor

Usually I run slow e2e tests separately from unit tests. In this setup vscode-jest could be told to rerun only unit tests.


The other problem you talk about needs a type test. In Jest repo we run type tests via jest-runner-tsd and there are 1000+ assertion at the moment. These prevent type regression very effectively.

I understand that the line between TypeScript and JavaScript is very thin, but look at Jest website: "Jest is a delightful JavaScript Testing Framework". It does not sound like Jest promised to check your types. The point is that Jest should allow everything what JavaScript allows and test only the logic of your code. So seeing no error in your example is correct.


Perhaps a custom transformer is a solution? The TS Compiler API allows custom module resolution. It is pretty simply to make it load something else instead of import from @jest/globals. Add some option to ts-jest to toggle that and you have strict checks. (I talk about ts-jest, because it checks types and it uses the Compiler API already.)

@CreativeTechGuy ping. We had similar discussion in DefinitelyTyped/DefinitelyTyped#63610

@mrazauskas
Copy link
Contributor

mrazauskas commented Dec 17, 2022

Actually, no need to have any option or to wrestle with the Compiler API and no need to involve ts-jest too:

import {expect} from 'jest-strictly-typed-expect';

Could work like this too (I did not try):

import type {StrictlyTypedMatchers} from 'jest-strictly-typed-matchers';

declare module 'expect' {
    interface Matchers<R> extends StrictlyTypedMatchers<R> {}
}

@github-actions
Copy link

This issue is stale because it has been open 30 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 Jan 16, 2023
@DetachHead
Copy link
Author

Not stale

@github-actions github-actions bot removed the Stale label Jan 17, 2023
benjaminjkraft added a commit to benjaminjkraft/jest that referenced this issue Jan 31, 2023
Matchers isn't as typed as some users would like (see jestjs#13334, jestjs#13812).
For users who want to customize it by extending the `Matchers`
interface, it's useful to have access to the type of `actual` (the
argument of `expect`) so you can do, say,
```ts
interface Matchers<R, T> {
    toTypedEqual(expected: T): R
}
```
This commit exposes it. The first-party matchers still have the same
types as before.
@github-actions github-actions bot removed the Stale label Feb 16, 2023
@github-actions
Copy link

This issue is stale because it has been open 30 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 Mar 18, 2023
@DetachHead
Copy link
Author

Not stale

@DetachHead
Copy link
Author

i really think this issue needs to be addressed. jest's assertion library is used by many other testing tools, so this problem comes up in many different places.

Usually I run slow e2e tests separately from unit tests. In this setup vscode-jest could be told to rerun only unit tests.

microsoft/playwright#22371

expect(page.locator('li').count()).toBe(2) //comparing Promise<number> to number

this is just one example where the error could easily be caught at compiletime, but it isn't. playwright isn't just used for e2e tests either (see https://playwright.dev/docs/test-components)

I understand that the line between TypeScript and JavaScript is very thin, but look at Jest website: "Jest is a delightful JavaScript Testing Framework". It does not sound like Jest promised to check your types.

times have changed since that was written. typescript is the standard now, and i can't think of any excuse to not be using it over javascript in the current year. so many "typescript-first" projects use jest's assertion library under the hood, so it seems a little counter-productive to not support it properly.

@KotlinIsland
Copy link

KotlinIsland commented Apr 14, 2023

"Jest is a delightful JavaScript Testing Framework". It does not sound like Jest promised to check your types. The point is that Jest should allow everything what JavaScript allows and test only the logic of your code. So seeing no error in your example is correct.

@mrazauskas I feel that this comment is somewhat misguided, if an end user is using JavaScript, then they will not see any TypeScript type errors, if the user has opted into using TypeScript then they explicitly want to see type errors and expect the language to behave in the most useful way possible (showing type errors where available). To opt out of type correctness in this one example seems to go counter to the concepts of typing and type safety in general.

FYI, Kotlin uses this tight strictness for assertions: https://kotlinlang.org/api/latest/kotlin.test/kotlin.test/assert-equals.html
Dart uses this tight strictness for assertions: dart-lang/matcher#234

@mrazauskas
Copy link
Contributor

Changes in this repo will have no effect on Playwright’s .toBe() typings, because they keep a hard copy here:

https://github.com/microsoft/playwright/blob/da87a0af76d244b977a13702dff86a004890ba99/packages/playwright-test/types/test.d.ts#L3816


After #13848 it should be possible to redeclare matcher typings as strict as you like them. (Not sure if that will work for Playwright.)

@DetachHead
Copy link
Author

Changes in this repo will have no effect on Playwright’s .toBe() typings, because they keep a hard copy here:

i mean it won't have an immediate effect but if they're copying jest's assertion library it's reasonable to assume they will eventually update it with upstream changes. otherwise they wouldn't close issues that suggest changes to it

@github-actions
Copy link

This issue is stale because it has been open 30 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 14, 2023
@DetachHead
Copy link
Author

Not stale

@github-actions github-actions bot removed the Stale label May 14, 2023
@github-actions
Copy link

This issue is stale because it has been open 30 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 Jun 13, 2023
@DetachHead
Copy link
Author

Not stale

@github-actions github-actions bot removed the Stale label Jun 13, 2023
@github-actions
Copy link

This issue is stale because it has been open 30 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 Jul 13, 2023
@DetachHead
Copy link
Author

Not stale

@github-actions github-actions bot removed the Stale label Jul 13, 2023
@github-actions
Copy link

This issue is stale because it has been open 30 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 Aug 12, 2023
@DetachHead
Copy link
Author

Not stale

@github-actions github-actions bot removed the Stale label Aug 12, 2023
@mrazauskas
Copy link
Contributor

Just use typed-jest-expect. "Elegant jest.expect typings for a more civilized age", as they say. Currently 48 weekly downloads.

@github-actions
Copy link

This issue 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 Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants