jest-community / eslint-plugin-jest

ESLint plugin for Jest
MIT License
1.12k stars 228 forks source link

[new rule] no-untyped-spies #1598

Closed MillerSvt closed 1 week ago

MillerSvt commented 1 month ago

Incorrect code:

let dispatchSpy: jest.SpyInstance;
let barSpy: jest.SpyInstance;

beforeEach(() => {
  const mockStore$ = TestBed.inject(MockStore);
  const someService = TestBed.inject(SomeService);

  dispatchSpy = jest.spyOn(mockStore$, `dispatch`);
  barSpy = jest.spyOn(someService.foo, 'bar');
});

Correct code:

let dispatchSpy: jest.Spied<MockStore['dispatch']>;
let barSpy: jest.Spied<SomeService['foo']['bar']>;

beforeEach(() => {
  const mockStore$ = TestBed.inject(MockStore);
  const someService = TestBed.inject(SomeService);

  dispatchSpy = jest.spyOn(mockStore$, `dispatch`);
  barSpy = jest.spyOn(someService.foo, 'bar');
});
let dispatchSpy: jest.SpyInstance<void, [action: Action]>;

Support autofix: yes (?)

Autofix can only be applied if we have accurately identified the type of value passed to the spyOn function. Otherwise, we can simply display an error message.

An example of an algorithm for autofixing:

let barSpy: jest.SpyInstance;

const someService = TestBed.inject(SomeService);

barSpy = jest.spyOn(someService.foo, 'bar');
  1. Determine the type of someService constant (SomeService).
  2. Find the chain of nested keys (foo → bar).
  3. Add a generic type to jest.Spied, using <SomeService['foo']['bar']>.

An example that we cannot fix automatically:

let barSpy: jest.SpyInstance;

const someService = TestBed.inject(SomeService);
const key: string = 'foo';

barSpy = jest.spyOn(someService[key], 'bar');

I can do this. Awaiting for feedback.

G-Rath commented 1 month ago

I'm really on the fence about this because it feels like something other tools should catch - typically in these cases, if TypeScript isn't erroring then its either safe (enough) or it's turning into an any in which case @typescript-eslint has a bunch of rules to catch this.

If you still think there's use, I wonder if it might be better for Jest to have something like a StrictSpyInstance?

MillerSvt commented 1 month ago

in which case @typescript-eslint has a bunch of rules to catch this.

As far as I know, @typescript-eslint can only catch any that I have written myself. For example:

jest.SpyInstance<any, any>;

However, in jest.SpyInstance, if no generic arguments are passed, they automatically become any.

interface SpyInstance<T = any, Y extends any[] = any, C = any> extends MockInstance<T, Y, C> {}

So it's completely legal to use jest.SpyInstance without specifying any generic type, and it won't trigger any warnings. However, it can lead to some issues related to disabled type-checking:

class Service {
  getUser(): User {
    ...
  }
}

class Component {
  user = getUser();
  firstName = user.firstName;
}

declare const service: Service;

let getUserSpy: jest.SpyInstance;

beforeEach(() => {
  getUserSpy.mockReturnValue(false); // no warnings
});

So, it would be helpful for us to create a rule that can prevent this behavior and automatically fix it if possible.


I wonder if it might be better for Jest to have something like a StrictSpyInstance?

I don't think so. I find SpyInstance to be quite difficult and not very useful. It requires at least two arguments, with the return type of the function and the arguments of those functions.

let getUserSpy: jest.SpyInstance<User, [userId: string]>;

It can be difficult if the method has a lot of arguments or a dynamically calculated return type. The Spied type can automatically turn an entire method or class into a SpyInstance:

     /**
     * Constructs the type of a spied class.
     */
    type SpiedClass<T extends abstract new(...args: any) => any> = SpyInstance<
        InstanceType<T>,
        ConstructorParameters<T>,
        T extends abstract new(...args: any) => infer C ? C : never
    >;

    /**
     * Constructs the type of a spied function.
     */
    type SpiedFunction<T extends (...args: any) => any> = SpyInstance<
        ReturnType<T>,
        ArgsType<T>,
        T extends (this: infer C, ...args: any) => any ? C : never
    >;

    type Spied<T extends (abstract new(...args: any) => any) | ((...args: any) => any)> = T extends abstract new(
        ...args: any
    ) => any ? SpiedClass<T>
        : T extends (...args: any) => any ? SpiedFunction<T>
        : never;

And it is enough to pass just the method to generic argument.

let getUserSpy: jest.Spied<User['getUser']>;
G-Rath commented 1 month ago

As far as I know, @typescript-eslint can only catch any that I have written myself. For example:

I assume you're thinking of the "no any" rule, but there's a whole suite of no-unsafe-* rules dedicated to yelling at you anytime you so much as look at something of type any

I don't think so. I find SpyInstance to be quite difficult and not very useful

it doesn't have to be SpyInstance if that's not the right fit - my point is about if it would be more useful for Jest to be supplying a type that does require the generics explicitly

MillerSvt commented 1 month ago

my point is about if it would be more useful for Jest to be supplying a type that does require the generics explicitly

This types already exists: jest.Spied and jest.Mocked

MillerSvt commented 1 month ago

a whole suite of no-unsafe-* rules dedicated to yelling at you anytime you so much as look at something of type any

None of these rules cover passing something to a function with any argument. playground

G-Rath commented 1 month ago

So then it sounds like you can use https://typescript-eslint.io/rules/ban-types/ to ban those looser types?

SimenB commented 1 month ago

Sounds like what you want is a jest.spied(t) which returns a jest.Spied<typeof t>, analogous to jest.mocked(t)?

Jest itself doesn't ship SpyInstance, only Spied (which was copied into @types/jest after Jest shipped it, https://github.com/DefinitelyTyped/DefinitelyTyped/pull/62781). So instead of ban-types (which should work for your use case I think?) you can also do import { jest } from '@jest/globals' and not have access to the unsafe type to begin with.

MillerSvt commented 2 weeks ago

Sounds like what you want is a jest.spied(t)

It seems difficult to create spied function. Jest can spy on getters and setters, not just methods. If we don't store the SpyInstance in a variable, I think it's impossible to access it later.

Jest itself doesn't ship SpyInstance, only Spied

Maybe so, but in reality, you can access it without any issues. The SpyInstance is not marked as internal and is not hidden from us.

So instead of ban-types (which should work for your use case I think?) you can also do import { jest } from '@jest/globals' and not have access to the unsafe type to begin with.

It's not useful to import jest for each test.

SimenB commented 2 weeks ago

Then ban-types is what you want

MillerSvt commented 1 week ago

Then ban-types is what you want

ok, I'll try, thank you!