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

Iterable equality within object #8359

Merged
merged 15 commits into from
Feb 24, 2022

Conversation

d7my11
Copy link
Contributor

@d7my11 d7my11 commented Apr 22, 2019

Summary

Fixes #8280 iterableEquality ignores other properties within inequal objects.

This PR solves an issue in iterableEquality for iterators within objects with other properties.

Test plan

  • Added test for iterable within enequal objects
  • Added test for iterable within equal objects
  iterableEquality
    ✓ returns true when given circular iterators (3ms)
    ✓ returns true when given circular Set
    ✓ returns true when given nested Sets (1ms)
    ✓ returns false when given inequal set within a set
    ✓ returns false when given inequal map within a set (1ms)
    ✓ returns false when given inequal set within a map
    ✓ returns true when given iterator within equal objects (1ms)
    ✓ returns false when given iterator within inequal objects
    ✓ returns false when given iterator within inequal nested objects
    ✓ returns true when given circular Set shape (1ms)
    ✓ returns true when given circular key in Map (3ms)
    ✓ returns true when given nested Maps
    ✓ returns true when given circular key and value in Map
    ✓ returns true when given circular value in Map

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Great work @d7my11, thanks!

packages/expect/src/__tests__/utils.test.js Outdated Show resolved Hide resolved
packages/expect/src/utils.ts Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@jeysal
Copy link
Contributor

jeysal commented Apr 22, 2019

By the way, don't forget to add a changelog entry for this fix :)

@d7my11 d7my11 force-pushed the iterable-equality-within-object branch from 8fc238f to 1e5d3b7 Compare April 22, 2019 10:53
@d7my11
Copy link
Contributor Author

d7my11 commented Apr 22, 2019

@jeysal I used Object.entries which is not supported by node@6.x. should i change that or should i wait for #8346 ?

@jeysal
Copy link
Contributor

jeysal commented Apr 22, 2019

@jeysal I used Object.entries which is not supported by node@6

Oh right, damn Node 6 is old 😁 Yeah even if Node 6 is EOL soon, we can only drop support for it in a major release and we might want some minors / patches first, so I guess it's better to not break it already.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@codecov-io
Copy link

codecov-io commented Apr 22, 2019

Codecov Report

Merging #8359 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #8359      +/-   ##
=========================================
- Coverage    65.1%   65.1%   -0.01%     
=========================================
  Files         278     278              
  Lines       11860   11864       +4     
  Branches     2923    2923              
=========================================
+ Hits         7722    7724       +2     
- Misses       3511    3512       +1     
- Partials      627     628       +1
Impacted Files Coverage Δ
packages/expect/src/utils.ts 95.06% <100%> (-1.15%) ⬇️

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 77c3cee...416f5b3. Read the comment docs.

@@ -251,6 +258,12 @@ export const iterableEquality = (
return false;
}

const aEntries = getObjectEntries(a);
const bEntries = getObjectEntries(b);
if (!equals(aEntries, bEntries)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add [iterableEquality] as optional customTesters argument to equals call in case any of these property values have iterators? All the calls in matchers.ts include it. I am asking, not telling.

This illustrates a limitation that we recently noticed: it is not (yet) possible to for a custom tester to know about sibling testers (for example, toStrictEqual matcher needs more testers than toEqual does, and iterableEquality has no way to provide them in this recursive call).

Copy link
Member

Choose a reason for hiding this comment

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

@pedrottimark is there an extra test case you're thinking of we should add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm so sorry for the very late follow up.
I'm not familiar with the code base but I see the point and the limitation.

This illustrates a limitation that we recently noticed: it is not (yet) possible to for a custom tester to know about sibling testers

IMO customTesters should be kept like this so each custom tester should test only one thing, so my changes in iterableEquality method will be reverted and will include non-enumerable members again in keys method in jasmineUtils.ts
I'm not sure of the consequences of this change but will try it out.

export const getObjectEntries: typeof Object.entries = (
object: any,
): Array<[string, any]> =>
object == null ? [] : Object.keys(object).map(key => [key, object[key]]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of Object.keys do we need to call keys in jasmineUtils.ts to include enumerable symbols (and maybe filter out Symbol.iterator in rare case it might be iterable in user-defined object) so that the comparison of properties is as consistent with ordinary equals call? I am asking, not telling.

This illustrates a limitation we have recently noticed: it is not (yet) possible for custom tester to know additional arguments for property comparison which distinguish toEqual from toStrictEqual matcher. Ignore that problem.

@pedrottimark
Copy link
Contributor

@d7my11 Thank you for clear solution to this problem. The questions that I asked say more about current excess complication in jasmineUtils.ts and utils.ts than about your work.

@SimenB
Copy link
Member

SimenB commented Nov 9, 2019

@d7my11 ping 🙂

@d7my11 d7my11 force-pushed the iterable-equality-within-object branch from e1c6917 to 416f5b3 Compare November 23, 2019 20:40
CHANGELOG.md Show resolved Hide resolved
@jquense
Copy link
Contributor

jquense commented Jun 24, 2020

anything i can do to get this over the line, the current behavior is surprising (and led to lots of bad tests i didn't know about) and hard to work around.

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.

tests are only added, so if CI is happy, I'm happy 🙂

Thanks!

@SimenB SimenB merged commit 536efe8 into jestjs:main Feb 24, 2022
@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 Mar 27, 2022
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.

isEqual() on iterable ignores other properties
7 participants