jest icon indicating copy to clipboard operation
jest copied to clipboard

[Feature]: typesafe assertions

Open DetachHead opened this issue 3 years ago • 18 comments

🚀 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

DetachHead avatar Sep 29 '22 07:09 DetachHead

Fix it. Just open a PR (;

mrazauskas avatar Sep 29 '22 07:09 mrazauskas

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

SimenB avatar Sep 29 '22 08:09 SimenB

Hi, can I try this?

jesrodri avatar Oct 10 '22 19:10 jesrodri

Yeah, go for it!

SimenB avatar Oct 11 '22 01:10 SimenB

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 avatar Oct 14 '22 14:10 jesrodri

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.
    */

jesrodri avatar Oct 14 '22 14:10 jesrodri

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.

mrazauskas avatar Oct 14 '22 16:10 mrazauskas

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

DetachHead avatar Oct 17 '22 23:10 DetachHead

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[bot] avatar Nov 16 '22 23:11 github-actions[bot]

not stale

DetachHead avatar Nov 17 '22 03:11 DetachHead

Similar change was made in @types/jest some time ago and had to be reverted. For discussion see https://github.com/DefinitelyTyped/DefinitelyTyped/discussions/62128

mrazauskas avatar Dec 15 '22 19:12 mrazauskas

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.
  • https://github.com/DefinitelyTyped/DefinitelyTyped/pull/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

DetachHead avatar Dec 16 '22 00:12 DetachHead

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.

mrazauskas avatar Dec 16 '22 06:12 mrazauskas

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
})

DetachHead avatar Dec 17 '22 14:12 DetachHead

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 https://github.com/DefinitelyTyped/DefinitelyTyped/pull/63610

mrazauskas avatar Dec 17 '22 15:12 mrazauskas

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> {}
}

mrazauskas avatar Dec 17 '22 15:12 mrazauskas

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[bot] avatar Jan 16 '23 16:01 github-actions[bot]

Not stale

DetachHead avatar Jan 17 '23 00:01 DetachHead

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[bot] avatar Feb 16 '23 00:02 github-actions[bot]

Not stale

DetachHead avatar Feb 16 '23 03:02 DetachHead

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[bot] avatar Mar 18 '23 03:03 github-actions[bot]

Not stale

DetachHead avatar Mar 18 '23 03:03 DetachHead

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.

https://github.com/microsoft/playwright/issues/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.

DetachHead avatar Apr 14 '23 00:04 DetachHead

"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: https://github.com/dart-lang/matcher/issues/234

KotlinIsland avatar Apr 14 '23 00:04 KotlinIsland

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.)

mrazauskas avatar Apr 14 '23 03:04 mrazauskas

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

DetachHead avatar Apr 14 '23 04:04 DetachHead

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[bot] avatar May 14 '23 05:05 github-actions[bot]

Not stale

DetachHead avatar May 14 '23 06:05 DetachHead

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[bot] avatar Jun 13 '23 07:06 github-actions[bot]

Not stale

DetachHead avatar Jun 13 '23 09:06 DetachHead