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

Handle arrays when merging snapshots #7089

Merged
merged 10 commits into from Apr 22, 2019
Merged

Handle arrays when merging snapshots #7089

merged 10 commits into from Apr 22, 2019

Conversation

timtrinidad
Copy link
Contributor

Summary

#6455 fixes properties in an object being left out of the snapshot if they are not specified in propertyMatchers by merging the matchers and the received snapshot.

However, arrays are not being merged so fields are being left out in the snapshot. For example:

const result = {
  data: {
    one: 'one',
    two: 'two',
    three: [
      {
        four: 'four',
        five: 'five',
      },
    ],
  }
}

expect(result).toMatchSnapshot({
  data: {
    two: expect.any(String),
    three: [
      {
        four: expect.any(String),
      },
    ],
  },
});

Expected Snapshot:

exports[`snapshot 1`] = `
Object {
  "data": Object {
    "one": "one",
    "three": Array [
      Object {
        "five": "five", // <-- Missing from "Actual"
        "four": Any<String>,
      },
    ],
    "two": Any<String>,
  },
}
`;

Actual snapshot:

exports[`snapshot 1`] = `
Object {
  "data": Object {
    "one": "one",
    "three": Array [
      Object {
        "four": Any<String>,
      },
    ],
    "two": Any<String>,
  },
}
`;

Test plan

The test case has been updated to include an array to be merged for testing deepMerge.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@SimenB
Copy link
Member

SimenB commented Oct 18, 2018

@rickhanlonii mind reviewing this?

@@ -185,6 +185,15 @@ export const deepMerge = (target: any, source: any) => {
if (isObject(source[key]) && !source[key].$$typeof) {
if (!(key in target)) Object.assign(mergedOutput, {[key]: source[key]});
else mergedOutput[key] = deepMerge(target[key], source[key]);
} else if (Array.isArray(source[key])) {
// Convert arrays to objects, merge, convert back to array
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds expensive. Can we instead make deepMergeArray similar to what we have in deepCyclicCopy.js?
https://github.com/facebook/jest/blob/fb61bfffb803bad0c2ff9ab1798a6c4c31cb14fa/packages/jest-util/src/deepCyclicCopy.js#L83-L105

@rickhanlonii
Copy link
Member

Hey @timtrinidad, sorry for the delay

I think we can close this if we do #7128 instead

cc @thymikee

@timtrinidad
Copy link
Contributor Author

timtrinidad commented Dec 2, 2018

Thanks for looking at this! I think #7128 is still different - the snapshot test in this issue only includes asymmetric matchers.

@rickhanlonii
Copy link
Member

Ah, yes I understand now, thanks @timtrinidad

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Agree with @thymikee that we should use deepMergeArray, and the changelog needs fixed, then this can go 👍

Sorry again for the delay

@@ -195,12 +195,57 @@ test('serialize handles \\r\\n', () => {

describe('DeepMerge', () => {
it('Correctly merges objects with property matchers', () => {
const target = {data: {bar: 'bar', foo: 'foo'}};
/* eslint-disable sort-keys */
// to keep keys in numerical order rather than alphabetical
Copy link
Member

Choose a reason for hiding this comment

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

Are you just adding a missing test or is this a changed behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is adding a missing test for the case of nested matchers.

four: 'four',
five: 'five',
},
// Include an array element not present in the propertyMatchers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an extra array element to ensure behavior between differently-sized arrays works as expected

};
// Don't use `expect.any(string)` since that will cause a false positive
// if deepMerge incorrectly keeps two as 'two' from the target
const matcher = '--matcher--';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was originally const matcher = expect.any(string), but when messing with util.deepMerge() I noticed if I just return the target unmodified, this returned a false positive.

@@ -178,13 +178,32 @@ export const saveSnapshotFile = (
);
};

const deepMergeArray = (target, source) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inspired by the deepmerge documentation.

I looked at replacing util.deepMerge() with that npm package since it's already used indirectly by jest-website, but it looks like we had to implement the array merging logic manually either way.

@SimenB
Copy link
Member

SimenB commented Dec 25, 2018

@rickhanlonii ping :)

@yofriadi
Copy link

Hi guys, when will this get merged, I've been waiting for months, thank you :)

@codecov-io
Copy link

Codecov Report

Merging #7089 into master will increase coverage by <.01%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7089      +/-   ##
==========================================
+ Coverage   62.22%   62.22%   +<.01%     
==========================================
  Files         266      266              
  Lines       10720    10731      +11     
  Branches     2609     2611       +2     
==========================================
+ Hits         6670     6677       +7     
- Misses       3463     3465       +2     
- Partials      587      589       +2
Impacted Files Coverage Δ
packages/jest-snapshot/src/utils.ts 88.31% <63.63%> (-4.12%) ⬇️

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 65b48a7...1dada66. Read the comment docs.

@@ -185,8 +185,6 @@ const deepMergeArray = (target: Array<any>, source: Array<any>) => {
source.forEach((element, index) => {
if (typeof mergedOutput[index] === 'undefined') {
mergedOutput[index] = element;
} else if (typeof element === 'undefined') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In adding tests to cover this line, I realized that this case really shouldn't happen (i.e. source having an array element that's undefined)

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Nice work @timtrinidad!
Sorry this was left open so long, and thanks for your swift responses after Simen brought this up again :)

@SimenB SimenB merged commit 7e1e3f8 into jestjs:master Apr 22, 2019
@scotthovestadt
Copy link
Contributor

scotthovestadt commented May 1, 2019

I have to revert this PR for now. It introduces a simple regression around strings being converted to objects, which I took the time to fix here scotthovestadt@3fd5434 but it also introduces a regression around breaking expect.any(...) matchers within arrays, which looks like a more complicated fix. We're doing a release soon and I don't have enough confidence that any fix I do will be sufficient.

Happy to have this merged in once it doesn't break those use-cases.

Copy link
Contributor

@pedrottimark pedrottimark left a comment

Choose a reason for hiding this comment

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

Nevermind. I was in the wrong place when I did this.

@SimenB
Copy link
Member

SimenB commented May 1, 2019

@timtrinidad the revert included regression tests, so if you feel up for it I'd love a new PR landing this 🙂

timtrinidad added a commit to timtrinidad/jest that referenced this pull request May 1, 2019
@timtrinidad
Copy link
Contributor Author

This updated PR (#8408) checks specifically for arrays of arrays and arrays of scalars. The various tests were also split into cases for readability and DRYness.

@joartola
Copy link

Is this issue fixed? If it is, in what version of jest was fixed?

@timtrinidad
Copy link
Contributor Author

Not yet - PR #8408 is still open.

@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 11, 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.

None yet