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
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -78,6 +78,7 @@
- `[jest-haste-map]` Remove legacy condition for duplicate module detection ([#7333](https://github.com/facebook/jest/pull/7333))
- `[jest-haste-map]` Fix `require` detection with trailing commas and ignore `import typeof` modules ([#7385](https://github.com/facebook/jest/pull/7385))
- `[jest-cli]` Fix to set prettierPath via config file ([#7412](https://github.com/facebook/jest/pull/7412))
- `[jest-snapshot]` Handle arrays when merging snapshots ([#7089](https://github.com/facebook/jest/pull/7089))

### Chore & Maintenance

Expand Down
72 changes: 67 additions & 5 deletions packages/jest-snapshot/src/__tests__/utils.test.js
Expand Up @@ -195,12 +195,74 @@ test('serialize handles \\r\\n', () => {

describe('DeepMerge', () => {
it('Correctly merges objects with property matchers', () => {
const target = {data: {bar: 'bar', foo: 'foo'}};
const matcher = expect.any(String);
const propertyMatchers = {data: {foo: matcher}};
/* 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.

const target = {
data: {
one: 'one',
two: 'two',
three: [
{
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

{
six: 'six',
seven: 'seven',
},
],
},
};
// Don't use `expect.any(string)` since that will cause a false positive
jeysal marked this conversation as resolved.
Show resolved Hide resolved
// 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.

const propertyMatchers = {
data: {
two: matcher,
three: [
{
four: matcher,
},
],
},
};

const mergedOutput = deepMerge(target, propertyMatchers);
expect(mergedOutput).toStrictEqual({
data: {
one: 'one',
two: matcher,
three: [
{
four: matcher,
five: 'five',
},
{
six: 'six',
seven: 'seven',
},
],
},
});

expect(mergedOutput).toStrictEqual({data: {bar: 'bar', foo: matcher}});
expect(target).toStrictEqual({data: {bar: 'bar', foo: 'foo'}});
// Ensure original target is not modified
expect(target).toStrictEqual({
data: {
one: 'one',
two: 'two',
three: [
{
four: 'four',
five: 'five',
},
{
six: 'six',
seven: 'seven',
},
],
},
});
/* eslint-enable sort-keys */
});
});
19 changes: 19 additions & 0 deletions packages/jest-snapshot/src/utils.js
Expand Up @@ -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.

// Clone target
const mergedOutput = target.slice();

source.forEach((element, index) => {
if (typeof mergedOutput[index] === 'undefined') {
mergedOutput[index] = element;
} else if (typeof element === 'undefined') {
mergedOutput.push(target[index]);
} else {
mergedOutput[index] = deepMerge(target[index], element);
}
});

return mergedOutput;
};

export const deepMerge = (target: any, source: any) => {
const mergedOutput = Object.assign({}, target);
if (isObject(target) && isObject(source)) {
Object.keys(source).forEach(key => {
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])) {
mergedOutput[key] = deepMergeArray(target[key], source[key]);
} else {
Object.assign(mergedOutput, {[key]: source[key]});
}
Expand Down