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

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

Merged
merged 12 commits into from
Aug 9, 2018
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

this should refer to this PR, not the issue


### Chore & Maintenance

Expand Down
23 changes: 22 additions & 1 deletion packages/jest-snapshot/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,27 @@ const cleanup = (hasteFS: HasteFS, update: SnapshotUpdateState) => {
};
};

function isObject(item) {
return (item && typeof item === 'object' && !Array.isArray(item));
}

function deepMerge(target, source) {
Copy link
Member

Choose a reason for hiding this comment

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

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 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

let 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(output, {[key]: source[key]});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

else
output[key] = deepMerge(target[key], source[key]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

} else {
Object.assign(output, {[key]: source[key]});
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

}
});
}
return mergedOutput;
}

const toMatchSnapshot = function(
received: any,
propertyMatchers?: any,
Expand Down Expand Up @@ -98,7 +119,7 @@ const toMatchSnapshot = function(
report,
};
} else {
Object.assign(received, propertyMatchers);
received = deepMerge(received, propertyMatchers);
}
}

Expand Down