Iterable equality within object #8359

d7my11 · 2019-04-22T09:41:40Z

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

jeysal

Great work @d7my11, thanks!

packages/expect/src/__tests__/utils.test.js

packages/expect/src/utils.ts

facebook-github-bot · 2019-04-22T10:05:42Z

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 · 2019-04-22T10:14:07Z

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

…y-within-object

d7my11 · 2019-04-22T11:02:49Z

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

jeysal · 2019-04-22T11:09:45Z

@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.

packages/expect/src/utils.ts

facebook-github-bot · 2019-04-22T12:05:31Z

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 · 2019-04-22T12:52:16Z

Codecov Report

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

@@            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.

pedrottimark · 2019-04-22T14:10:30Z

packages/expect/src/utils.ts

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

+  const aEntries = getObjectEntries(a);
+  const bEntries = getObjectEntries(b);
+  if (!equals(aEntries, bEntries)) {


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

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

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.

pedrottimark · 2019-04-22T14:20:56Z

packages/expect/src/utils.ts

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


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 · 2019-04-22T14:29:29Z

@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.

…ity-within-object

SimenB · 2019-11-09T16:40:47Z

@d7my11 ping 🙂

…ity-within-object

CHANGELOG.md

jquense · 2020-06-24T18:55:51Z

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.

SimenB

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

Thanks!

github-actions · 2022-03-27T00:07:27Z

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.

d7my11 added 4 commits April 22, 2019 00:04

iterableEquality when given iterator within inequal object

9bc6a08

remove unnecessary object check

86312c9

check equality again only if object has keys

7e13ba4

ignore iterators while checking other properties

8ec8ab5

jeysal reviewed Apr 22, 2019

View reviewed changes

packages/expect/src/__tests__/utils.test.js Outdated Show resolved Hide resolved

packages/expect/src/utils.ts Outdated Show resolved Hide resolved

jeysal requested review from mattphillips and pedrottimark April 22, 2019 10:01

remove unnecessary emptyObject check

53408bd

d7my11 added 2 commits April 22, 2019 12:51

update CHANGELOG.md

87c4e62

Merge branch 'master' of github.com:d7my11/jest into iterable-equalit…

1e5d3b7

…y-within-object

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

facebook-github-bot added the cla signed label Apr 22, 2019

d7my11 added 2 commits April 22, 2019 13:41

add getObjectEntries instead of Object.entries

84f8bda

import in ascending order

8068054

SimenB reviewed Apr 22, 2019

View reviewed changes

packages/expect/src/utils.ts Outdated Show resolved Hide resolved

better type for getObjectEntries - fix linting issue

3cd2b4e

pedrottimark reviewed Apr 22, 2019

View reviewed changes

Merge branch 'master' of github.com:facebook/jest into iterable-equal…

c4d40b9

…ity-within-object

d7my11 added 2 commits November 23, 2019 21:23

Merge branch 'master' of github.com:facebook/jest into iterable-equal…

2c3a862

…ity-within-object

replace getObjectEntries with Object.entries

416f5b3

d7my11 force-pushed the iterable-equality-within-object branch from e1c6917 to 416f5b3 Compare November 23, 2019 20:40

SimenB reviewed Nov 23, 2019

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

SimenB added 2 commits February 24, 2022 21:03

Merge branch 'main' into iterable-equality-within-object

e50e95e

move changelog

5a46db1

SimenB approved these changes Feb 24, 2022

View reviewed changes

SimenB merged commit 536efe8 into jestjs:main Feb 24, 2022

github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iterable equality within object #8359

Iterable equality within object #8359

d7my11 commented Apr 22, 2019

jeysal left a comment

facebook-github-bot commented Apr 22, 2019

jeysal commented Apr 22, 2019

d7my11 commented Apr 22, 2019 •

edited

Loading

jeysal commented Apr 22, 2019

facebook-github-bot commented Apr 22, 2019

codecov-io commented Apr 22, 2019 •

edited

Loading

pedrottimark Apr 22, 2019

SimenB Nov 23, 2019

d7my11 Nov 24, 2019

pedrottimark Apr 22, 2019

pedrottimark commented Apr 22, 2019

SimenB commented Nov 9, 2019

jquense commented Jun 24, 2020

SimenB left a comment

github-actions bot commented Mar 27, 2022

Iterable equality within object #8359

Iterable equality within object #8359

Conversation

d7my11 commented Apr 22, 2019

Summary

Test plan

jeysal left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 22, 2019

jeysal commented Apr 22, 2019

d7my11 commented Apr 22, 2019 • edited Loading

jeysal commented Apr 22, 2019

facebook-github-bot commented Apr 22, 2019

codecov-io commented Apr 22, 2019 • edited Loading

Codecov Report

pedrottimark Apr 22, 2019

Choose a reason for hiding this comment

SimenB Nov 23, 2019

Choose a reason for hiding this comment

d7my11 Nov 24, 2019

Choose a reason for hiding this comment

pedrottimark Apr 22, 2019

Choose a reason for hiding this comment

pedrottimark commented Apr 22, 2019

SimenB commented Nov 9, 2019

jquense commented Jun 24, 2020

SimenB left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 27, 2022

d7my11 commented Apr 22, 2019 •

edited

Loading

codecov-io commented Apr 22, 2019 •

edited

Loading