Fix bugs with mock/spy result tracking of recursive functions.

[UselessPickles]

Summary

Results of mocks/spies were not being recorded properly for recursive functions because the result of the call was pushed onto an array AFTER the function returned (results were in reverse order).

I have reworked the result recording code to push an object onto the "results" array immediately upon calling the mock (with values representing an "incomplete" call), then update that same result object upon returning.

• Results are now recorded in the right order during recursion.
• All the "return" expect matchers have been updated to be aware of incomplete calls.

Test plan

See new unit tests. Edge cases related to recursion are now covered (including testing mock result state and calling expect return matchers from inside of the recursive function).

Breaking Change

The structure of the MockFunctionResult has changed. The property isThrow: boolean has changed to type: 'return' | 'throw' | 'incomplete'.

[codecov-io]

Codecov Report

Merging #6381 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6381      +/-   ##
==========================================
+ Coverage   63.57%   63.59%   +0.01%     
==========================================
  Files         235      235              
  Lines        8992     8996       +4     
  Branches        3        4       +1     
==========================================
+ Hits         5717     5721       +4     
  Misses       3274     3274              
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-mock/src/index.js	87.65% <100%> (+0.15%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c61baa...57deef3. Read the comment docs.

[rickhanlonii]

Great find! Appreciate all the contributions you're making to Jest @UselessPickles!

[UselessPickles]

One of the CI tests failed during yarn install:

[2/4] Fetching packages...
error An unexpected error occurred: "https://registry.yarnpkg.com/hosted-git-info/-/hosted-git-info-2.6.0.tgz: Parse Error".

Any ideas?

[SimenB]

CI seems green?

[thymikee]

I've silently restarted the build :)

[UselessPickles]

Better late than never?

[UselessPickles]

@SimenB @rickhanlonii Ready for review again.

[UselessPickles]

bump

It's been oddly quiet here for a while. Has this PR been forgotten, or is it just low priority?

[UselessPickles]

@SimenB @rickhanlonii poke

[UselessPickles]

@rickhanlonii @SimenB Is there any chance this can be reviewed/merged soon?

[UselessPickles]

@SimenB @rickhanlonii What's the next step on this PR? Do you want me to change the entry in the changelog? I can't really comment on what the entry should be.

[thymikee]

@SimenB mind taking a final look at this one?

[SimenB]

@rickhanlonii next release is a major, mind making the breaking changes we want and land this?

[thymikee]

ping @SimenB @rickhanlonii

[SimenB]

Might work to just revert/remove the last few commits so we're back to the original code submitted in this PR?

[UselessPickles]

Do we just want to remove the "deprecated" isThrow property? I can take care of that tonight. I haven't thought about this in a long time, so I can't remember if there were any other breaking changes that would be nice to do now while we have the chance.

[SimenB]

Yeah, that should be it!

[SimenB]

Perfect, thank you so much! And sorry about the churn

[rubennorte]

Hey @UselessPickles, I just saw this PR after releasing 24.0.0-alpha.6. Would you mind updating the PR (mainly the title) to better reflect the implications of the changes you did here? In this case, the structure of mock objects have changed for end-users and that's not clear by reading this. E.g.:

Before:

expect(callbackOne.mock.results[0].isThrow).toBe(true);

After:

expect(callbackOne.mock.results[0].type).toBe('throw');

I can edit the PR but I prefer if the original author can do it. I'll update the changelog soon so you don't have to worry about that.

[UselessPickles]

I think the PR title accurately describes the purpose of this PR. The PR description clearly explains the breaking change. The entry in the change log clearly indicates that there is a breaking change. I see nothing wrong.

If the goal is to compile a list of breaking changes and a migration guide, then all the necessary information is already there.

I don't know what you would expect from the title to both describe the PR purpose and indicate breaking changes. Feel free to change it.

[rubennorte]

Perhaps I didn't use the better wording here. I think this should've been 2 different PRs: one to change the mock interface and another to fix their behaviour with recursive functions. The most relevant change here is the change in the interface, even though your initial goal with this PR was to fix the issue.

[UselessPickles]

The breaking change came about during the PR review process (at the request of a reviewer). It was not originally a breaking change, and even the design of the breaking change changed over time (at one point retaining deprecated backward compatibility, etc.). I don't think it would have been practical to separate it into two PRs until the very end when the entire change was approved. Hindsight, etc.

Again... I really don't know what you expect from an updated title, so I'm the wrong person to make the change. You can update the title/description however you see fit to clarify the breaking change. I won't be offended :)

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.