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

Use iterableEquality in spy matchers #3651

Merged

Conversation

philipp-spiess
Copy link
Contributor

@philipp-spiess philipp-spiess commented May 25, 2017

Fixes #3574
FIxes #3619

Summary

While working on custom matchers to solve #3574, I found out that the cause for
not seeing this issue in expect().toEqual() comes from the fact that this
matcher passes the iterableEquality to the equals() function.

When I added this to the equal calls for our spy matchers as well, Immutable.js
types were properly suppored.

I'm considering this is a bug since the toBeCalledWith() matchers should
behave the same as the equals() matcher.

Test plan

I added a unit test that uses the already included Immutable.js library.

@philipp-spiess
Copy link
Contributor Author

After a quick test, it seems like this also fixes #3619. Should I add the example as another test?

@thymikee
Copy link
Collaborator

If you can think of any other valid example, you're more than welcome to do so! 🙂

@philipp-spiess philipp-spiess force-pushed the philipp/iterator-equality-in-spy-matchers branch 2 times, most recently from da50ded to 709966f Compare May 26, 2017 12:20
@codecov-io
Copy link

codecov-io commented May 26, 2017

Codecov Report

Merging #3651 into master will increase coverage by 0.01%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3651      +/-   ##
==========================================
+ Coverage   58.08%   58.09%   +0.01%     
==========================================
  Files         195      195              
  Lines        6751     6751              
  Branches        6        6              
==========================================
+ Hits         3921     3922       +1     
+ Misses       2827     2826       -1     
  Partials        3        3
Impacted Files Coverage Δ
packages/jest-matchers/src/matchers.js 99.38% <ø> (+1.07%) ⬆️
packages/jest-matchers/src/spy_matchers.js 95.16% <100%> (ø) ⬆️
packages/jest-matchers/src/utils.js 96.07% <86.66%> (-3.93%) ⬇️
packages/jest-matchers/src/jasmine_utils.js 79.81% <0%> (+0.91%) ⬆️

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 98a6d9d...4a0daf8. Read the comment docs.

@philipp-spiess
Copy link
Contributor Author

@thymikee Perfect! I added tests to ensure that the matchers work with ES6 Maps and Sets. Looks all good to me :)

I'm not 100% sure if this is really a fix for #3574, since Immutable.Map and Immutable.Set don't have a specified iterator order and thus there might be two collection that are equal but their iterators emits the values in a different order. But my change will at least unify this behavior with our equals() matcher which, at least for me, is enough Immutable.js support for now. 😄

@philipp-spiess
Copy link
Contributor Author

@thymikee Is there anything else I can do to help get this merged? :)

@thymikee
Copy link
Collaborator

Just wait for @cpojer to review :)

@philipp-spiess philipp-spiess force-pushed the philipp/iterator-equality-in-spy-matchers branch from 709966f to 07ef0ca Compare June 12, 2017 19:57
@cpojer
Copy link
Member

cpojer commented Jun 27, 2017

Would you mind rebasing this?

While working on custom matchers to solve jestjs#3574, I found out that the cause for
not seeing this issue in `expect().toEqual()` comes from the fact that this
matcher passes the `iterableEquality` to the `equals()` function.

When I added this to the equal calls for our spy matchers as well, Immutable.js
types were properly suppored.

I'm considering this is a bug since the `toBeCalledWith()` matchers should
behave the same as the `equals()` matcher.
@philipp-spiess philipp-spiess force-pushed the philipp/iterator-equality-in-spy-matchers branch from 07ef0ca to 4a0daf8 Compare June 27, 2017 11:44
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@cpojer cpojer merged commit c8a5135 into jestjs:master Jun 27, 2017
@cpojer
Copy link
Member

cpojer commented Jun 27, 2017

Nice! Thank you.

@philipp-spiess philipp-spiess deleted the philipp/iterator-equality-in-spy-matchers branch June 27, 2017 14:17
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* Use iterableEquality in spy matchers

While working on custom matchers to solve jestjs#3574, I found out that the cause for
not seeing this issue in `expect().toEqual()` comes from the fact that this
matcher passes the `iterableEquality` to the `equals()` function.

When I added this to the equal calls for our spy matchers as well, Immutable.js
types were properly suppored.

I'm considering this is a bug since the `toBeCalledWith()` matchers should
behave the same as the `equals()` matcher.

* Add spy matchers tests using ES6 Map and Set
@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 13, 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.

.toHaveBeenCalledWith is using only the type of the arguments Immutable.js equality
5 participants