[prefer-mock-return-shorthand]: option to only error on static values

[lukpsaxo]

Thanks for adding prefer-mock-return-shorthand - I tried enabling it autofix on our codebase and it simplified alot of test code.

However we have too many cases where it broke tests because a return from mockImplementation was dynamic - I'd rather not have eslint disable across these ~40 cases.

I was thinking it would be good if there was an option to only error on static values e.g. true/"test"/{static: true} and maybe an exception for Promise.resolve/Promise.reject.

[G-Rath]

Can you provide some code examples?

[lukpsaxo]

sure

so something like this:

const eventHandler = new EventHandler();

jest.spyOn(MockedClass, 'subscribe').mockImplementation((fn) => eventHandler.subscribe(fn));

so taking a class but mocking out some part of it to a basic event handler.

or something like this:

let currentX = 0;
jest.spyOn(X, getCount).mockImplementation(() => currentX);

// stuff happens

currentX++;

// more stuff happens

The examples I think are all - a local variable that is mutated or calling through to a different function that returns a value but has a side effect.

It would be fair enough to say you don't think these are good examples of good design, but unfortunately our codebase has enough of these in large enough examples that re-architecting is not worth it to make it more testable.

So upto you if you want to support these types of scenarios. Thanks for looking at this issue.

[G-Rath]

The first one should be addressable by adding a body to the function, and doing a return of the event handler separately if that is needed.

The second one is something we should be able to detect (i.e. based on the let)

I sort of purposely decided to ignore this when implementing as I didn't think it'd be super common and wanted some real world feedback first 🙂

[github-actions]

🎉 This issue has been resolved in version 29.11.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[lukpsaxo]

Thanks!

I'm trying out the new release and its better - I've decided just to disable the remaining cases and switch the rule on as 99% now works fine. Unfortunately I have some examples like this:

            jest.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementation(
                () => {
                    return fromPartial({
                        width,
                        height: 60,
                    });
                },
            );

so here width is a variable.

I tried making it into a function with a body but it still errors. I need to add a 2nd statement to make it think its a valid mockImplementation.

A 2nd example:

            getSpy = jest
                .fn()
                .mockName('get')
                // eslint-disable-next-line jest/prefer-mock-return-shorthand
                .mockImplementation(() => {
                    return new Promise((resolve) => {
                        getSuccessCallbacks.push((data: any) => {
                            resolve({ response: { Data: data } });
                        });
                    });
                });

        dispatchSpy.mockImplementation(() => getHistory().push('/b'));

    function mockSubscription(timeTillOrphanedObj: { time: number }) {
        const subscription = {
            timeTillOrphaned: jest.fn(),
        };
        subscription.timeTillOrphaned.mockImplementation(() => timeTillOrphanedObj.time);
        return subscription;
    }

and theres a couple more that are very similiar to the above.

Lastly I got 2 weird failure - one was caused by mocking a redux store, so the test depended on a new object being returned each time - so the mockReturnValue returned always the same instance and another one to do with promises that is probably to niche to go into here. But for those I think its valid to just add a eslint disable line.

[G-Rath]

@lukpsaxo have done some work to improve this so your first example should be picked up now.

Your other examples are a lot harder to detect, though I think we could technically do some of them based on function calls (for the third example) and looking at the variable type a bit more (for the fourth example).

I think these are in the same camp as what you've mentioned in your last paragraph, which is that really it's technically not correct for us to say you can use the shorthand method if you're returning the value of a function call or an object, because both of those could be mutated.

While being safe is good, I'm sort of counting on that to try and ensure feedback for the rule because I feel people are more likely to make issues for "rule broke my code" than "rule could fix this case" as the latter requires people to effectively do the thing the rule is for, and then be motivated to make an issue... 😅

I wouldn't be surprised if in future we either have an option for changing this behaviour, or just making it the default entirely - in the meantime though huge thanks for you dogfooding this for me! It sounds like you've got access to a large familiar test suite to apply this to, so I'm very interested in hearing about your experiences and findings with applying our rules so keep them coming if there's more!

[github-actions]

🎉 This issue has been resolved in version 29.12.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀