Correctly merge property matchers with the rest of the snapshot in `toMatchSnapshot`. #6528

swcisel · 2018-06-23T08:12:57Z

Summary

Correct an issue where snapshots that were tested against property matchers would not be merged with the rest of the object being tested.

Closes #6581
Closes #6684
Closes #6455

Test plan

All tests pass.

swcisel · 2018-06-23T08:16:59Z

I believe this fixes the issue mentioned in #6455. This is my first PR on this project, so I'll happily take any advice or guidance from the community.

mattphillips · 2018-06-23T11:33:22Z

packages/jest-snapshot/src/index.js

+    Object.keys(source).forEach(key => {
+      if (isObject(source[key]) && !source[key].$$typeof) {
+        if (!(key in target))
+          Object.assign(output, {[key]: source[key]});


I'm not sure where output is defined? Should this not be:

Object.assign(mergedOutput, {[key]: source[key]});

Thanks for catching that, it was late when I submitted this and I overlooked it.

mattphillips · 2018-06-23T11:34:12Z

packages/jest-snapshot/src/index.js

+        if (!(key in target))
+          Object.assign(output, {[key]: source[key]});
+        else
+          output[key] = deepMerge(target[key], source[key]);


Same as above

mattphillips · 2018-06-23T11:34:20Z

packages/jest-snapshot/src/index.js

+        else
+          output[key] = deepMerge(target[key], source[key]);
+      } else {
+        Object.assign(output, {[key]: source[key]});


Same as above

mattphillips

It might make sense to move the deepMerge function out of src/index.js and into src/utils and then add some tests for the logic in: https://github.com/facebook/jest/blob/master/packages/jest-snapshot/src/__tests__/utils.test.js

EDIT: It might be useful to add a new integration test for this too: https://github.com/facebook/jest/blob/master/e2e/__tests__/to_match_snapshot.test.js#L225

@SimenB @thymikee @rickhanlonii do we already have a deep merge function defined anywhere else in the codebase that can be reused?

SimenB

Thanks for tackling this!

This is missing tests, both for the snapshots and the merge function itself.

Tests for snapshots can be added here: https://github.com/facebook/jest/blob/404da0f53bd9c6204823e4ce96c1b6742e7470e3/e2e/__tests__/to_match_snapshot.test.js

If you're unable to find a deep merge module which handles the edge cases, it should receive some unit tests as well.

SimenB · 2018-06-23T11:51:45Z

CHANGELOG.md

@@ -14,6 +14,7 @@
 - `[expect]` `toEqual` no longer tries to compare non-enumerable symbolic properties, to be consistent with non-symbolic properties. ([#6398](https://github.com/facebook/jest/pull/6398))
 - `[jest-util]` `console.timeEnd` now properly log elapsed time in milliseconds. ([#6456](https://github.com/facebook/jest/pull/6456))
 - `[jest-mock]` Fix `MockNativeMethods` access in react-native `jest.mock()` ([#6505](https://github.com/facebook/jest/pull/6505))
+- `[jest-snapshot]` Correctly merge property matchers with the rest of the snapshot in `toMatchSnapshot`. ([#6455](https://github.com/facebook/jest/issues/6455))


this should refer to this PR, not the issue

SimenB · 2018-06-23T11:54:56Z

packages/jest-snapshot/src/index.js

+  return (item && typeof item === 'object' && !Array.isArray(item));
+}
+
+function deepMerge(target, source) {


I'd very much prefer if we could find some deep merge on npm. If that's not possible, this should be moved to a util file and receive quite extensive unit tests 🙂

That's a very good point. It doesn't make a lot of sense to clutter this file up with a general-purpose merge function. I'll take a look at this tonight.

The issue with all of the deep merge libs I tried is that they don't copy the prototype so they break the asymmetric matchers -- the strategy here is the solution I was going to go with as well

SimenB · 2018-06-23T11:59:19Z

@SimenB @thymikee @rickhanlonii do we already have a deep merge function defined anywhere else in the codebase that can be reused?

Nope, we only have shallow merge (and that uses Object.assign)

correct typo variable name in deepMerge

swcisel · 2018-06-24T04:01:56Z

I've moved the deepMerge function to jest-snapshot's utils, and added a simple test. Before I add more thorough tests, would anyone mind weighing in on whether I'm headed in the right direction? I also did some digging into existing deep merge solutions, but I think keeping it local is our best bet.

SimenB · 2018-07-11T09:46:28Z

I think this is a good starting point, yeah 🙂

…property_matcher-2

rickhanlonii

Looks great, I added integration tests, updated the unit test, and added a fix for #6581

Here's the new error I added:

rickhanlonii · 2018-07-12T14:49:18Z

packages/jest-snapshot/src/__tests__/utils.test.js

+    const mergedOutput = deepMerge(target, propertyMatchers);
+
+    expect(mergedOutput).toStrictEqual({data: {bar: 'bar', foo: matcher}});
+    expect(target).toStrictEqual({data: {bar: 'bar', foo: 'foo'}});


@swcisel I updated your tests here, does this make sense?

Looks good to me. thanks!

rickhanlonii · 2018-07-12T14:49:43Z

packages/jest-snapshot/src/index.js

+    throw new Error(
+      'Property matchers must be an object.\n\nTo provide a snapshot test name without property matchers, use: toMatchSnapshot("name")',
+    );
+  }


Added the checks here to close #6581

codecov-io · 2018-07-12T15:14:46Z

Codecov Report

Merging #6528 into master will increase coverage by 0.02%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master    #6528      +/-   ##
==========================================
+ Coverage   63.55%   63.57%   +0.02%     
==========================================
  Files         235      235              
  Lines        9030     9044      +14     
  Branches        3        3              
==========================================
+ Hits         5739     5750      +11     
- Misses       3290     3293       +3     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-snapshot/src/utils.js	`100% <100%> (ø)`	⬆️
packages/jest-snapshot/src/index.js	`54.05% <20%> (-1.67%)`	⬇️

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 0591f22...e4fbf98. Read the comment docs.

rickhanlonii · 2018-07-16T14:21:34Z

@SimenB what do you think now?

github-actions · 2021-05-12T10:07:25Z

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.

deep merge property matchers in snapshot objects

a88a7c0

facebook-github-bot added the cla signed label Jun 23, 2018

mattphillips reviewed Jun 23, 2018

View reviewed changes

mattphillips suggested changes Jun 23, 2018

View reviewed changes

SimenB requested changes Jun 23, 2018

View reviewed changes

swcisel added 2 commits June 23, 2018 08:09

Update index.js

994e09e

correct typo variable name in deepMerge

Update CHANGELOG.md

cbf8f6b

swcisel changed the title ~~deep merge property matchers in snapshot objects~~ Correctly merge property matchers with the rest of the snapshot in toMatchSnapshot. Jun 23, 2018

moved deepMerge to jest-snapshot utils.

7ac7f4d

SimenB requested a review from rickhanlonii July 11, 2018 09:45

SimenB mentioned this pull request Jul 12, 2018

Snapshot property matchers mutate passed-in object #6684

Closed

rickhanlonii added 5 commits July 12, 2018 09:34

Merge remote-tracking branch 'origin/master' into full_snapshot_with_…

a3b32ed

…property_matcher-2

Update changelog

74796b0

Update deepmerge tests

d17e7f6

Add errors for invalid property matchers

1b8dd70

Update changelog

deb016c

rickhanlonii reviewed Jul 12, 2018

View reviewed changes

rickhanlonii mentioned this pull request Jul 12, 2018

Confusing error on missing toMatchSnapshot() argument #6581

Closed

SimenB mentioned this pull request Jul 13, 2018

Add snapshot property matchers #6210

Merged

SimenB approved these changes Jul 16, 2018

View reviewed changes

Merge branch 'master' into full_snapshot_with_property_matcher

7032a35

InExtremaRes mentioned this pull request Jul 28, 2018

Are literal values deliberately allowed as property matchers in toMatchSnapshot()? #6774

Closed

thymikee added 2 commits August 8, 2018 22:13

Merge branch 'master' into full_snapshot_with_property_matcher

e4fbf98

Merge branch 'master' into full_snapshot_with_property_matcher

3902a22

thymikee approved these changes Aug 9, 2018

View reviewed changes

thymikee merged commit 3c3f9f2 into jestjs:master Aug 9, 2018

This was referenced Oct 1, 2018

Update jest in /modules/api from 22.4.3 to 23.6.0 project-accelerate/accelerate#245

Open

Update jest in /modules/frontend from 22.4.3 to 23.6.0 project-accelerate/accelerate#272

Open

dependencies bot mentioned this pull request Nov 1, 2018

Update jest in / from 22.1.4 to 23.6.0 leonardofaria/leozera-ui#250

Closed

pedrottimark mentioned this pull request Mar 12, 2019

Snapshots for strings are sometimes being transformed into objects/arrays #8059

Closed

snyk-bot mentioned this pull request Mar 17, 2020

[Snyk] Upgrade expect from 23.0.0 to 23.6.0 yowainwright/react-workshop#8

Merged

dependabot bot mentioned this pull request Mar 13, 2021

Bump babel-jest from 18.0.0 to 23.6.0 CartoonIsArt/eevee#133

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly merge property matchers with the rest of the snapshot in `toMatchSnapshot`. #6528

Correctly merge property matchers with the rest of the snapshot in `toMatchSnapshot`. #6528

swcisel commented Jun 23, 2018 •

edited by rickhanlonii

Loading

swcisel commented Jun 23, 2018

mattphillips Jun 23, 2018

swcisel Jun 23, 2018

mattphillips Jun 23, 2018

mattphillips Jun 23, 2018

mattphillips left a comment •

edited

Loading

SimenB left a comment

SimenB Jun 23, 2018

SimenB Jun 23, 2018

swcisel Jun 23, 2018

rickhanlonii Jul 12, 2018

SimenB commented Jun 23, 2018

swcisel commented Jun 24, 2018

SimenB commented Jul 11, 2018

rickhanlonii left a comment

rickhanlonii Jul 12, 2018

swcisel Jul 12, 2018

rickhanlonii Jul 12, 2018

codecov-io commented Jul 12, 2018 •

edited

Loading

rickhanlonii commented Jul 16, 2018

github-actions bot commented May 12, 2021

Correctly merge property matchers with the rest of the snapshot in toMatchSnapshot. #6528

Correctly merge property matchers with the rest of the snapshot in toMatchSnapshot. #6528

Conversation

swcisel commented Jun 23, 2018 • edited by rickhanlonii Loading

Summary

Test plan

swcisel commented Jun 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattphillips left a comment • edited Loading

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented Jun 23, 2018

swcisel commented Jun 24, 2018

SimenB commented Jul 11, 2018

rickhanlonii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 12, 2018 • edited Loading

Codecov Report

rickhanlonii commented Jul 16, 2018

github-actions bot commented May 12, 2021

Correctly merge property matchers with the rest of the snapshot in `toMatchSnapshot`. #6528

Correctly merge property matchers with the rest of the snapshot in `toMatchSnapshot`. #6528

swcisel commented Jun 23, 2018 •

edited by rickhanlonii

Loading

mattphillips left a comment •

edited

Loading

codecov-io commented Jul 12, 2018 •

edited

Loading