Skip to content
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

Omit empty calls array in plugin for mock functions #4848

Merged
merged 3 commits into from
Nov 6, 2017

Conversation

pedrottimark
Copy link
Contributor

@pedrottimark pedrottimark commented Nov 6, 2017

Summary

Balance the following constraints /cc @mjesun

Before a mock function has been called, serialize it similar to function, especially when it is property of JavaScript object or React element.

Example adapted from #4829 (comment)

Object {
  "hasMore": [MockFunction],
  "loadMore": [MockFunction],
  "refetchConnection": [MockFunction],
}

Serialize non-empty array so it is impossible to accidentally match with an object #4835 (comment)

EDIT selected different example which has non-default name from a test case:

Object {
  "fn": [MockFunction MyConstructor] {
    "calls": Array [
      Array [
        Object {
          "name": "some fine name",
        },
      ],
    ],
  },
}

This pull request makes incremental improvements to:

Test plan

10 tests including 5 snapshots

@codecov-io
Copy link

codecov-io commented Nov 6, 2017

Codecov Report

Merging #4848 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4848      +/-   ##
==========================================
+ Coverage   59.19%   59.25%   +0.06%     
==========================================
  Files         200      200              
  Lines        6641     6646       +5     
  Branches        3        4       +1     
==========================================
+ Hits         3931     3938       +7     
+ Misses       2710     2708       -2
Impacted Files Coverage Δ
packages/jest-snapshot/src/mock_serializer.js 100% <100%> (+50%) ⬆️

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 071f382...76314fa. Read the comment docs.

exports[`mock with 1 calls and non-default name via new in object 1`] = `
Object {
"fn": [MockFunction MyConstructor]
calls: Array [
Copy link
Member

Choose a reason for hiding this comment

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

This looks pretty weird

@cpojer
Copy link
Member

cpojer commented Nov 6, 2017

I have trouble understanding the grouping in the line that I commented. Shouldn't the calls be wrapped in a { around it like:

[MockConstructor Foo] {
  calls: …
}

?

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

I like where this is going! On mobile, so a bit hard to read the snapshot-diffs. First glance looks great, though!


expect(mockWithName).toMatchSnapshot();
test('maxDepth option', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a mock function with a name to a toMatchSnapshot assertion? I can see it working in this test, but a separate assertion would be awesome!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See MyConstructor in mock with 1 calls and non-default name via new in object 1

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I can see it working, but it would be sweet with a test of just the name, no calls or wrapping objects

});

test('mock with name', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This test is what I referred to in my comment

@@ -7,30 +7,122 @@
*/
'use strict';

const mock = jest.fn();
const prettyFormat = require('pretty-format');
Copy link
Member

Choose a reason for hiding this comment

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

import


afterEach(() => mock.mockReset());
const plugin = require('../mock_serializer');
Copy link
Member

Choose a reason for hiding this comment

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

import

@pedrottimark
Copy link
Contributor Author

@cpojer Yes, your wish is my command: #4848 (comment)

@cpojer cpojer merged commit 6c4401d into jestjs:master Nov 6, 2017
@pedrottimark pedrottimark deleted the mock-function-plugin branch November 6, 2017 19:28
@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.

5 participants