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.

Sign up for GitHub

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

Jump to bottom

fix: toMatchObject works with super getters #10381

Merged
merged 3 commits into from
Aug 22, 2020
Merged

fix: toMatchObject works with super getters #10381

merged 3 commits into from
Aug 22, 2020

Conversation

souldzin
Copy link
Contributor

@souldzin souldzin commented Aug 7, 2020

Summary

@sstern6 and I worked on this PR to fix #10268, where toMatchObject would not work for getters defined in a super class. It does this by using the in operator as opposed to the previous hasOwnProperty checks.

Test plan

The newly added spec fails on master

# This fails on master

git co 10268-fix-to-match-object-with-getters -- packages/expect/src/__tests__/matchers.test.js
yarn jest packages/expect/src/__tests__/matchers.test.js

And these specs pass on this branch, implying a 🐛 fix 😄

souldzin
souldzin commented Aug 7, 2020
View reviewed changes
packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap
<d>expect(</><r>received</><d>).</>not<d>.</>toMatchObject<d>(</><g>expected</><d>)</>

Expected: not <g>{"a": undefined, "b": "b", "c": "c"}</>
Received: <r>{}</>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

question: You'll notice that we don't include these non-enumerable keys in the error reporting. I have some ideas on how we can improve this, but fixing this will be quite involved. Is this acceptable for now? Can we handle improving this in a follow-up? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

a follow-up on this would be great, but we definitely don't have to handle it at the same time 👍

@souldzin
Copy link
Contributor Author

souldzin commented Aug 7, 2020

@sstern6 could you take a look at this PR we paired on? I still need to add the CHANGELOG 👀

@souldzin souldzin force-pushed the 10268-fix-to-match-object-with-getters branch from 9572967 to 1b3ddba Compare August 7, 2020 21:47
@sstern6
Copy link

sstern6 commented Aug 9, 2020

LGTM!

@souldzin souldzin force-pushed the 10268-fix-to-match-object-with-getters branch from 1b3ddba to 3a88c29 Compare August 9, 2020 15:27
@souldzin
Copy link
Contributor Author

souldzin commented Aug 9, 2020

Thanks @sstern6! I just rebased with upstream/master to see if that would fix the strange pipeline failure.

@jeysal do you have availability to review this PR? Thanks! 🙇

@souldzin
fix(expect): toMatchObject with inherited getters
a8e7e67 
- It also works with `defineProperty`
- There's still a bit of work to be done on
  improving the reporting of this
@souldzin
Recurse proto chain in hasPropertyInObject
3467338 
This way we can catch the actual props of the object
without catching those in Object.prototype
@souldzin
Replace hasOwnProperty with hasPropertyInObject
1dde15b
@souldzin souldzin force-pushed the 10268-fix-to-match-object-with-getters branch from 3a88c29 to 1dde15b Compare August 21, 2020 03:54
SimenB
SimenB approved these changes Aug 21, 2020
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great!

@SimenB
Copy link
Member

SimenB commented Aug 21, 2020

@jeysal @thymikee @pedrottimark would any of you be able to look over this?

@jeysal
Copy link
Contributor

jeysal commented Aug 21, 2020

Not super familiar with this code, but (mostly based on tests) LGTM

@SimenB SimenB merged commit 5c6480b into jestjs:master Aug 22, 2020
@SimenB
Copy link
Member

SimenB commented Aug 22, 2020

Oh sorry @sstern6, I forgot to add the co-author tag to the commit 😞

@SimenB
Copy link
Member

SimenB commented Aug 22, 2020

https://github.com/facebook/jest/releases/tag/v26.4.2

@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.
Reviewers

@SimenB SimenB SimenB approved these changes

@jeysal jeysal Awaiting requested review from jeysal

@thymikee thymikee Awaiting requested review from thymikee

@pedrottimark pedrottimark Awaiting requested review from pedrottimark

@mattphillips mattphillips Awaiting requested review from mattphillips

@cpojer cpojer Awaiting requested review from cpojer

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

toMatchObject doesn't work with class getters
5 participants
@souldzin @sstern6 @SimenB @jeysal @facebook-github-bot