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

[expect] Fix circular references in iterable equality #8160

Merged
merged 6 commits into from Mar 20, 2019

Conversation

mattphillips
Copy link
Contributor

@mattphillips mattphillips commented Mar 19, 2019

Summary

Fixes #6830, closes #7526

This uses the same algorithm as eq in jasmineUtils

Test plan

See unit tests

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.

Missing changelog, code LGTM

@jeysal
Copy link
Contributor

jeysal commented Mar 19, 2019

@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

@pedrottimark
Copy link
Contributor

Yes, please see the recent pull request or the algorithm in pretty-format if it is more appropriate.

return true;
}

aStack.pop();
Copy link
Member

Choose a reason for hiding this comment

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

These pops (and the algorithm in general) deserves some clarifying comments

if (
nextB.done ||
!equals(aValue, nextB.value, [
(a: any, b: any) => iterableEquality(a, b, aStack, bStack),
Copy link
Member

Choose a reason for hiding this comment

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

iterableEqualityWithStack?

@codecov-io
Copy link

codecov-io commented Mar 20, 2019

Codecov Report

Merging #8160 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
packages/expect/src/utils.ts 96.55% <100%> (+1.16%) ⬆️

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 c5fd7aa...2e4611f. Read the comment docs.

@mattphillips
Copy link
Contributor Author

@jeysal see ef47fa2 for:

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);

@jeysal
Copy link
Contributor

jeysal commented Mar 20, 2019

@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 iterableEquality?

@thymikee
Copy link
Collaborator

@mattphillips @jeysal yea, I think we'd all love that. Please do so in a followup :)

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iterables and toHaveBeenCalledWith result in RangeError: Maximum call stack size exceeded
7 participants