Disable @typescript-eslint/unbound-method rule in Jest tests

[Mrtenz]

This disables the @typescript-eslint/unbound-method rule in tests, which is too strict in cases, for example when asserting that a function has been called:

const someObject = {
  foo: function() {
    // ...
  }
};

// Assuming this is mocked somewhere.
expect(someObject.foo).toHaveBeenCalled();

[Gudahtt]

Hmm. A lot of JavaScript developers don't have a great understanding of binding. This rule prevents a common footgun.

The case you mentioned is indeed inconvenient to deal with, but I have discovered that there is a separate rule just for this case: https://github.com/jest-community/eslint-plugin-jest/blob/main/docs/rules/unbound-method.md

We could consider using that instead for tests, so that the scenario you described here isn't flagged as a problem.

[Mrtenz]

Hmm. A lot of JavaScript developers don't have a great understanding of binding. This rule prevents a common footgun.

The case you mentioned is indeed inconvenient to deal with, but I have discovered that there is a separate rule just for this case: https://github.com/jest-community/eslint-plugin-jest/blob/main/docs/rules/unbound-method.md

We could consider using that instead for tests, so that the scenario you described here isn't flagged as a problem.

I wasn't aware of this rule. Makes sense to use this instead!

[Mrtenz]

These changes are just indentation, I haven't changed any actual rules here. The only real changes in this file are on the bottom.

[Mrtenz]

This rule requires type information, so we can't enable it in the general config, which may be used for JavaScript files.

[Mrtenz]

Vitest doesn't have an equivalent rule, so I think we can just disable the rule here?

[mcmire]

Hmm. If we're moving everything to Vitest then doesn't it seem like we would still be ignoring a possible footgun?

[Gudahtt]

Apparently a solution is in the works: vitest-dev/eslint-plugin-vitest#591

Not ideal, but, I'm OK with disabling this in the interim personally.

[mcmire]

@Mrtenz In the example you posted in the PR description:

const someObject = {
  foo: function() {
    // ...
  }
};

// Assuming this is mocked somewhere.
expect(someObject.foo).toHaveBeenCalled();

could the lint violation be avoided by using the following?

const someObject = {
  foo: function() {
    // ...
  }
};

const fooSpy = jest.spyOn(someObject, 'foo');
// or: const fooSpy = vi.spyOn(someObject, 'foo');

expect(fooSpy).toHaveBeenCalled();

If so, are there are other examples where this lint rule would be inconvenient?

[Gudahtt]

Generally looks good! Seeing some test errors though

[Gudahtt]

@mcmire I think you're right, that pattern would avoid this lint violation.

We could document that workaround instead 🤔. But I don't know, it still seems better to allow referencing a function unbound for assertions, as it's a bit shorter and maybe easier to read. The risks of passing an unbound method don't apply in that case anyway.

Thoughts on this workaround v.s. disabling the rule in the case of vitest, where we don't have a more specific rule for this yet?

[Mrtenz]

This is not actually run, but needed because otherwise ESLint will fail because there's no tests.

[Mrtenz]

Thoughts on this workaround v.s. disabling the rule in the case of vitest, where we don't have a more specific rule for this yet?

@Gudahtt Thinking about it, maybe it's better we don't disable it for the Vitest config, since enabling the Vitest-specific rule in the future would be a breaking change, whereas just replacing the TypeScript with the Vitest rule when it's available would not (I think)?

[Gudahtt]

Yes I think you're right, it would be a breaking change if we disabled it now and enabled the vitest rule later.

But replacing the base rule with the Jest rule just loosens the check, so it's non-breaking.

[Gudahtt]

LGTM!