-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Implemented the .toMatchObject(object)
assertion
#1843
Conversation
Thank you for your pull request. As you may know, we require contributors to sign our Contributor License Agreement, and we don't seem to have you on file and listed as active anymore. In order for us to review and merge your code, please email cla@fb.com with your details so we can update your status. |
Current coverage is 88.95% (diff: 98.27%)@@ master #1843 diff @@
==========================================
Files 39 39
Lines 1409 1467 +58
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 1248 1305 +57
- Misses 161 162 +1
Partials 0 0
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Can this be the default for |
No, I would prefer |
Any updates on the status of this PR? |
@excitement-engineer would you mind rebasing this diff? |
@cpojer I have performed the rebase:) |
Ok thanks! @DmitriiAbramov is going to handle this whenever he gets around to it :) |
Great! Thanks a lot for your help! |
[{a: 'b', t: {z: 'z', x: {r: 'r'}}}, {a: 'b', t: {z: 'z'}}], | ||
[{a: 'b', t: {z: 'z', x: {r: 'r'}}}, {t: {x: {r: 'r'}}}], | ||
[{a: [3, 4, 5], b: 'b'}, {a: [3, 4, 5]}], | ||
[{a: [3, 4, 5], b: 'b'}, {a: [3, 4]}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm a little scared to have the same behavior on arrays.
Properties definitely make sense, but for arrays it can be dangerous if you match some enums or other sets.
for example, this will pass, and it's pretty straightforward:
expect({permissions: ['read', 'write'], ...manyOtherProps}).toMathcObject({permissions: ['read']});
but if the left side is a return of the function, that might be not intuitive and even confusing:
expect(getPermissions()).toMathcObject({permissions: ['read']});
// i expect it to be just 'read', but there's and extra value that i don't even see, and the test is passing
should we match only objects, and use strict equality for arrays? what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you! I have removed this behaviour and have added a check that an equality check is performed for the elements in the array.
So this test will now fail:
expect({permissions: ['read', 'write'], ...manyOtherProps}).toMatchObject({permissions: ['read']});
There is one exception with the equality check mentioned above; you can still perform matchObject checks for key-value objects contained in arrays (i.e. array of objects), so in my update this will pass:
expect([{a: 'a', b: 'b'}, 5]).toMatchObject([{a: 'a'}, 5])
Do you agree with this behaviour?
if (typeof receivedObject !== 'object' || receivedObject === null) { | ||
throw new Error( | ||
matcherHint('[.not].toMatch', 'object', 'expected') + '\n\n' + | ||
`${RECEIVED_COLOR('string')} value must be an object.\n` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you probably copied 'string'
from .toMatch
matcher :)
i'm trying to think of a good message.
Expect object to be an object. Received
or
Expect expected to be an object. Received:
look king of weird.
maybe something like:
<red>object</red> value must be an object. Received:
and
<green>expected</green> value must be an object. Received:
thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, thanks for catching that, haha a bit embarrassing that I didn't see this;)
I implemented your suggestion:
<green>expected</green> value must be an object. Received:
const message = pass | ||
? () => matcherHint('.not.toMatchObject') + | ||
`\n\nExpected value not to match object:\n` + | ||
` ${printExpected(JSON.stringify(expectedObject))}` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't really use JSON.stringify
here.
you should use printExpected
and printReceived
, they won't break on circular references and will handle other special values like undefined
and friends :)
i think what we should also print is a diff between the entire expected
and filtered received
object, with only properties that matter. something like:
expect({a: 1, b: 1, c: 1, d: {e: {f: 555}}}).toMatch({d: {e: {f: 222}}});
Expected object to match:
{d: {e: {f: 222}}}
Received:
{a: 1, b: 1, c: 1, d: {e: {f: 555}}}
Difference:
{
d: {
e: {
+ f: 555,
- f: 222
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip, I have changed the code!
Any tips on how I can implement this diffing? Is it an idea to update the jest-diff
package with a new DiffOption
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have an idea. I will delete all the properties that are contained in the Received object but not contained in the matching object. I can then just use jest-diff
without introducing any changes there! Expect an update with this soon!
# Conflicts: # packages/jest-matchers/src/__tests__/matchers-test.js
@DmitriiAbramov I have made all the changes that you suggested in your code review. Let me know what you think! |
this PR is awesome! thank you so much @excitement-engineer! |
Thanks a lot for you help! No worries about the timing:) |
Guys, just to let you know: https://facebook.github.io/jest/docs/api.html#tomatchobjectobject |
Yea, this was already reported in #2195 :D Thanks! |
* Implemented .toMatchObject() * Fixed error message and array equality in toMatchObject() * Fixed listing errors. * toMatchObject now displays a diff when a test fails
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. |
Summary
Implemented the
toMatchObject(object)
function that checks whether an object matches a subset of the properties of another object. This was discussed in issue #1793.This matcher works for objects with a combination of key/value pairs, arrays and javascript dates: