Conversation
packages/expect/src/utils.js
Outdated
| return false; | ||
| } else { | ||
| const args = []; | ||
| } else if (a instanceof Set) { |
There was a problem hiding this comment.
With this change, this branch will pass if b is a superset of a, which I don't think is intentional.
There was a problem hiding this comment.
I don't see how that follows. b must contain all the elements of a, and be the same size. The size constraint should preclude a superset from matching (and there is a test for that).
There was a problem hiding this comment.
Good point, I didn't realize that we had this check up there.
packages/expect/src/utils.js
Outdated
| if (allFound) { | ||
| return true; | ||
| } | ||
| } else if (a instanceof Map) { |
|
I think the new equality check isn't complete. If possible, can we find a way not to use instanceof for Set and Map and detect them in a different way? |
|
What would you prefer to
|
|
The latter, we may end up with objects from different contexts. |
jestjs#4303 added code to check Set equality in an order-independent way, but applied that logic to all iterable objects. This change limits the scope of the special-case to just Set and Map, and corrects the Map case to test that both keys and values are equal.
nigelzor
force-pushed
the
fix-map-equality
branch
from
20c4b66 to
a8988c6
Compare
Codecov Report
@@ Coverage Diff @@
## master #4404 +/- ##
=======================================
Coverage 56.84% 56.84%
=======================================
Files 186 186
Lines 6298 6298
Branches 3 3
=======================================
Hits 3580 3580
Misses 2717 2717
Partials 1 1Continue to review full report at Codecov.
|
|
Thanks for working on this. |
* fix Map equality test jestjs#4303 added code to check Set equality in an order-independent way, but applied that logic to all iterable objects. This change limits the scope of the special-case to just Set and Map, and corrects the Map case to test that both keys and values are equal. * use isA instead of instanceof
|
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. |
4 participants
Summary
This code should throw, but currently does not:
#4303 added code to check Set equality in an order-independent way, but applied that logic to all iterable objects. This change limits the scope of the special-case to just Set and Map, and corrects the Map case to test that both keys and values are equal. The order-independent checks don't do deep equality on keys, but if not equal fall through to the order-dependent deep check (as before).
Test plan
Added additional tests for the
toEqual()matcher, and ranyarn test. Tests failed before the change, and now pass.