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

[unbound-method] Allow custom assert function names #1033

Closed
SchroederSteffen opened this issue Jan 22, 2022 · 6 comments
Closed

[unbound-method] Allow custom assert function names #1033

SchroederSteffen opened this issue Jan 22, 2022 · 6 comments

Comments

@SchroederSteffen
Copy link

SchroederSteffen commented Jan 22, 2022

Thanks for the plugin! We're happily using it in multiple TypeScript projects.

Though, we recently came across the issue that the unbound-method rule also complains if we're passing a method into a custom expect function like this one:

  const expectCallbackToReturn = (
    method: Class['method'],
    expectedReturnValue: string
  ) => {
    const mock = (method as jest.Mock).mock;
    const [callback] = mock.calls[0] as Parameters<typeof method>;
    expect(callback()).toBe(expectedReturnValue);
  };

[...]

    expectCallbackToReturn(
      // Error: Avoid referencing unbound methods
      mockInstance.method,
      'expected-value'
    );

We wrote this custom expect function to reuse it across a test file, but writing a custom matcher seems like overhead because we need it only in a single file.

A possible solution could be if we could specify custom assertFunctionNames like for expect-expect. Then, we would simply allow expect*.

@SchroederSteffen SchroederSteffen changed the title [unbound-method] Allow custom expect function names [unbound-method] Allow custom assert function names Jan 22, 2022
@G-Rath
Copy link
Collaborator

G-Rath commented Jul 14, 2022

I'm really on the fence about this one because this is actually dependent on more than just the name of the function, and I'm pretty sure the example you've given could be better structured using the mock types, e.g. something like this should work:

  const expectCallbackToReturn = (
    mock: jest.Mocked<Class['method']>,
    expectedReturnValue: string
  ) => {
    const [callback] = mock.calls[0];
    expect(callback()).toBe(expectedReturnValue);
  };

[...]

    expectCallbackToReturn(
      mockInstance.method.mock,
      'expected-value'
    );

We want to be careful with this rule because the more we customize it the harder it becomes to maintain with the base rule, and I'm not sure if there's enough value here vs the alternatives to be worth it.

@SchroederSteffen
Copy link
Author

SchroederSteffen commented Jul 15, 2022

Thanks for the response. I understand the concern.
Do you agree that it should be possible to write convenience assert/expect function to avoid code duplication? If there would be a working pattern for this, we wouldn't need to adjust the rule.

Unfortunately, I couldn't get your suggestion to work, because the property calls doesn't exist on type jest.Mocked<Class['method']>, at least on my machine.
Though, I was able to adjust the function as follows:

  const expectCallbackToReturn(
    mockedFunction: jest.MockedFunction<Class['method']>,
    expectedReturnValue: string
  ) => {
    const [callback] = mockedFunction.mock.calls[0];
    expect(callback()).toBe(expectedReturnValue);
  };

Though, this doesn't solve the jest/unbound-method finding.
In your example, which type should mockInstance have to properly access .method.mock? I tried with jest.Mocked<Class>, but the resulting type jest.MockContext<> doesn't seem right and fits neither jest.Mocked<> nor jest.MockedFunction<> as the function parameter.

@G-Rath
Copy link
Collaborator

G-Rath commented Jul 15, 2022

Here you go:

import { jest } from '@jest/globals';
import * as JestMock from 'jest-mock';

class MyClass {
  myMethod(callback: () => string) {
    // ...
  }
}
  
const expectCallbackToReturn = (
  mock: JestMock.MockInstance<MyClass['myMethod']>['mock'],
  expectedReturnValue: string
) => {
  const [callback] = mock.calls[0];

  expect(callback()).toBe(expectedReturnValue);
};

const mockedMyClass = jest.mocked(MyClass, true);

expectCallbackToReturn(
  mockedMyClass.prototype.myMethod.mock,
  'expected-value'
);

or using the types from @types/jest:

class MyClass {
  myMethod(callback: () => string) {
    // ...
  }
}

const expectCallbackToReturn = (
  mock: jest.MockedFunction<MyClass['myMethod']>['mock'],
  expectedReturnValue: string
) => {
  const [callback] = mock.calls[0];

  expect(callback()).toBe(expectedReturnValue);
};

const mockedMyClass = jest.mocked(MyClass, true);

expectCallbackToReturn(
  mockedMyClass.prototype.myMethod.mock,
  'expected-value'
);

@G-Rath
Copy link
Collaborator

G-Rath commented Aug 27, 2022

I'm going to close this since I think the better option is to restructure the types per my comments and examples above, so there's not a strong reason to extend the rule further.

@G-Rath G-Rath closed this as not planned Won't fix, can't repro, duplicate, stale Aug 27, 2022
@SchroederSteffen
Copy link
Author

SchroederSteffen commented Mar 21, 2023

Coming back reeaally late here... Many thanks for your help! ❤️
I had this on my list since last year and was only able to look into it again today.

Finally, I used the following approach based on your suggestions, only slightly modified:

const expectCallbackToReturn = (
  mock: jest.MockedFunction<MyClass['myMethod']>['mock'],
  expectedReturnValue: string
) => {
  const [callback] = mock.calls[0];
  expect(callback()).toBe(expectedReturnValue);
};

const mockedMyClass: jest.Mocked<MyClass> = { myMethod: jest.fn() };

expectCallbackToReturn(
  mockedMyClass.myMethod.mock,
  'expected-value'
);

The reason why I didn't use jest.mocked() and its jest.MockedObject[Deep]<> type is that I would have to access the .prototype every time I work with the mock as follows, also for assertions outside the helper method:
expect(mockedMyClass.prototype.myMethod).toHaveBeenCalledWith() instead of just expect(mockedMyClass.myMethod).toHaveBeenCalledWith().

This felt quite cumbersome and weird compared to all the other mocks.

(Also, the call signature would have to be jest.mocked(MyClass, { shallow: true }); instead of jest.mocked(MyClass, true);, right?
But for me it actually only worked without shallow as jest.mocked(MyClass); because otherwise the myMethod wouldn't have the .mock property.)


Anyways, many thanks again! I agree that using proper types is the better option.👍

@G-Rath
Copy link
Collaborator

G-Rath commented Mar 21, 2023

Glad I could help!

(Also, the call signature would have to be jest.mocked(MyClass, { shallow: true }); instead of jest.mocked(MyClass, true);, right?
But for me it actually only worked without shallow as jest.mocked(MyClass); because otherwise the myMethod wouldn't have the .mock property.)

Yeah that was the function signature used over at ts-jest (with the second param meaning "deep" rather than "shallow") - at the time I knew that the ts-jest helper had been brought into Jest, but I'd not actually used it so didn't realise they changed the signature slightly :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants