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

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

Merged
merged 30 commits into from Oct 31, 2018

Conversation

Projects
None yet
7 participants
@UselessPickles
Copy link
Contributor

UselessPickles commented Jun 2, 2018

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

This comment has been minimized.

Copy link

codecov-io commented Jun 2, 2018

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

Copy link
Member

rickhanlonii commented Jun 2, 2018

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

@UselessPickles

This comment has been minimized.

Copy link
Contributor Author

UselessPickles commented Jun 2, 2018

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

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jun 2, 2018

CI seems green?

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Jun 2, 2018

I've silently restarted the build :)

Show resolved Hide resolved CHANGELOG.md Outdated
@@ -96,7 +100,7 @@
- `[jest-diff]` Support returning diff from oneline strings ([#6221](https://github.com/facebook/jest/pull/6221))
- `[expect]` Improve return matchers ([#6172](https://github.com/facebook/jest/pull/6172))
- `[jest-cli]` Overhaul watch plugin hooks names ([#6249](https://github.com/facebook/jest/pull/6249))
- `[jest-mock]` Include tracked call results in serialized mock ([#6244](https://github.com/facebook/jest/pull/6244))
- `[jest-mock]` [**BREAKING**] Include tracked call results in serialized mock ([#6244](https://github.com/facebook/jest/pull/6244))

This comment has been minimized.

@UselessPickles

UselessPickles Jun 5, 2018

Author Contributor

Better late than never?

@UselessPickles

This comment has been minimized.

Copy link
Contributor Author

UselessPickles commented Jun 5, 2018

@SimenB @rickhanlonii Ready for review again.

@UselessPickles

This comment has been minimized.

Copy link
Contributor Author

UselessPickles commented Jun 14, 2018

bump

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

@UselessPickles

This comment has been minimized.

Copy link
Contributor Author

UselessPickles commented Jun 20, 2018

@UselessPickles

This comment has been minimized.

Copy link
Contributor Author

UselessPickles commented Jun 29, 2018

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

@rickhanlonii

This comment has been minimized.

Copy link
Member

rickhanlonii commented Jul 12, 2018

@UselessPickles yeah I'm ok with the change to mock_serializer as long as this goes in a minor version and not a patch, we've done snapshot updates for minor versions in the past

If you update the changelog, should it be something like [snapshot update] instead of [breaking]?

@UselessPickles

This comment has been minimized.

Copy link
Contributor Author

UselessPickles commented Aug 6, 2018

@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

This comment has been minimized.

Copy link
Collaborator

thymikee commented Aug 7, 2018

@SimenB mind taking a final look at this one?

@SimenB

SimenB approved these changes Aug 15, 2018

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Sep 29, 2018

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

@SimenB SimenB added this to the Jest 24 milestone Oct 16, 2018

@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Oct 30, 2018

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Oct 30, 2018

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

@UselessPickles

This comment has been minimized.

Copy link
Contributor Author

UselessPickles commented Oct 30, 2018

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

This comment has been minimized.

Copy link
Collaborator

SimenB commented Oct 30, 2018

Yeah, that should be it!

@SimenB

SimenB approved these changes Oct 31, 2018

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Oct 31, 2018

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

@SimenB SimenB merged commit bb9efac into facebook:master Oct 31, 2018

9 of 10 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@UselessPickles UselessPickles deleted the UselessPickles:recursive_mock_results branch Oct 31, 2018

@rubennorte

This comment has been minimized.

Copy link
Contributor

rubennorte commented Nov 9, 2018

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

This comment has been minimized.

Copy link
Contributor Author

UselessPickles commented Nov 9, 2018

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

This comment has been minimized.

Copy link
Contributor

rubennorte commented Nov 9, 2018

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

This comment has been minimized.

Copy link
Contributor Author

UselessPickles commented Nov 9, 2018

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 :)

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