Skip to content

Conversation

@SimenB
Copy link
Member

@SimenB SimenB commented Nov 3, 2017

Summary
Fixes #4835. /cc @mjesun

Test plan
Changed snapshots

const indentationNext = indentation + config.indent;

return printer(mockObject, config, indentation, depth, refs);
return (
Copy link
Member Author

@SimenB SimenB Nov 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels really convoluted to do it this way. @pedrottimark is this necessary to just change the header of the object?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, a solution for a built-in plugin is to call printObjectProperties for example:

https://github.com/facebook/jest/blob/master/packages/pretty-format/src/plugins/asymmetric_matcher.js#L40-L58

But AsymmetricMatcher will soon change not to do that for the reason that I missed when I first read #4835 (comment)

which will make impossible to accidentally match with an object.

Our currently proposed serialization matches instance constructed from ordinary class:

class MockFunction {
  constructor(calls = [], name = 'jest.fn()') {
    this.calls = calls;
    this.name = name;
  }
}

When I read Mock Functions page to borrow an example for alternative, a question hit me:

Should the serialization also include instances property?

http://facebook.github.io/jest/docs/en/mock-function-api.html#mockfnmockinstances

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally had instances as well, but it didn't really provide anything of value.

3514516

@codecov-io
Copy link

codecov-io commented Nov 3, 2017

Codecov Report

Merging #4836 into master will decrease coverage by 0.07%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4836      +/-   ##
==========================================
- Coverage   59.24%   59.17%   -0.08%     
==========================================
  Files         200      200              
  Lines        6633     6670      +37     
  Branches        4        3       -1     
==========================================
+ Hits         3930     3947      +17     
- Misses       2703     2723      +20
Impacted Files Coverage Δ
packages/jest-snapshot/src/mock_serializer.js 50% <0%> (ø) ⬆️
packages/jest-haste-map/src/worker.js 93.93% <0%> (-1.9%) ⬇️
...ckages/jest-cli/src/reporters/coverage_reporter.js 54.45% <0%> (-1.11%) ⬇️
packages/jest-haste-map/src/index.js 96.8% <0%> (-0.71%) ⬇️
packages/jest-runner/src/test_worker.js 0% <0%> (ø) ⬆️
packages/jest-cli/src/generate_empty_coverage.js 85.71% <0%> (ø) ⬆️
packages/jest-runner/src/index.js 38.29% <0%> (+0.52%) ⬆️
packages/jest-worker/src/index.js 100% <0%> (+15.71%) ⬆️
packages/jest-cli/src/reporters/coverage_worker.js 81.81% <0%> (+31.81%) ⬆️

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 eb70265...f72afcf. Read the comment docs.

@pedrottimark
Copy link
Contributor

To continue thought from inline comment with a question for me to learn: are there any situations when the expected result for assertion about mock function has both calls and instances non-empty, or would that only occur in an incorrect received value?

@pedrottimark
Copy link
Contributor

You mean, because invoking mock function with new is rare?

My mental slow cooker is working on a format which only displays neither, either, or both properties which are non-empty arrays.

A format without calls before mock has been called might solve original problem with mocked props in snapshot of JavaScript object or React element.

But I need to sketch what some realistic diffs would look like when snapshot fails.

@cpojer
Copy link
Member

cpojer commented Nov 4, 2017

Nice!

@cpojer cpojer merged commit 68a2f54 into jestjs:master Nov 4, 2017
@SimenB SimenB deleted the mock-serializer branch November 4, 2017 13:50
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change snapshot header for Jest mocks to MockFunction

5 participants