-
-
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
[expect] Fix circular references in iterable equality #8160
Conversation
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.
Missing changelog, code LGTM
@mattphillips we should also try how this works at different depths, e.g. const a1 = new Set();
const a2 = new Set();
a1.add(a2);
a2.add(a1);
const b = new Set();
b.add(b);
expect(iterableEquality(a1, b)).toBe(true); That would make it very similar to #7730 |
Yes, please see the recent pull request or the algorithm in |
2402b09
to
b70586f
Compare
return true; | ||
} | ||
|
||
aStack.pop(); |
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.
These pop
s (and the algorithm in general) deserves some clarifying comments
packages/expect/src/utils.ts
Outdated
if ( | ||
nextB.done || | ||
!equals(aValue, nextB.value, [ | ||
(a: any, b: any) => iterableEquality(a, b, aStack, bStack), |
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.
iterableEqualityWithStack
?
Codecov Report
@@ Coverage Diff @@
## master #8160 +/- ##
==========================================
+ Coverage 62.28% 62.35% +0.06%
==========================================
Files 265 265
Lines 10440 10455 +15
Branches 2538 2541 +3
==========================================
+ Hits 6503 6519 +16
Misses 3352 3352
+ Partials 585 584 -1
Continue to review full report at Codecov.
|
@mattphillips You probably have a good high-level overview of how the algorithm works in your head right now, any chance you could write a few lines about that as a comment for |
@mattphillips @jeysal yea, I think we'd all love that. Please do so in a followup :) |
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
Fixes #6830, closes #7526
This uses the same algorithm as
eq
injasmineUtils
Test plan
See unit tests