fixed asymmetrical equality of cyclic objects

[grosto]

Summary

fixes #7555

I have copied the node's implementation for comparing cyclic object. By which, two objects described in the issue are not considered equal.

Test plan

Added a new test case

[SimenB]

Thanks! Mind updating the changelog as well?

[jeysal]

I'm not convinced that not.toEqual() is the more intuitive option, but I can see that the opposite might be hard to implement.
@grosto did you encounter any implementation that properly treats them as equal (without bugs like Ramda)? Or any threads where this case was discussed in detail?

[grosto]

Updated the changelog.
I went through some libraries again to find if any of them consider these objects equal.

This is the only one
https://github.com/jkroso/equals/blob/master/index.js#L53-L58

The implementation is quite easy too. Could be achieved by changing one line in Jest's current equality function. I checked and all tests passed.

I could not find any threads discussing this case. Personally, I think they equal only if the cyclic reference is at the same depth in both objects. I also get that toEqual is more intuitive.

[codecov-io]

Codecov Report

Merging #7730 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #7730      +/-   ##
=========================================
+ Coverage   68.38%   68.4%   +0.02%     
=========================================
  Files         253     253              
  Lines        9723    9726       +3     
  Branches        5       5              
=========================================
+ Hits         6649    6653       +4     
+ Misses       3072    3071       -1     
  Partials        2       2

Impacted Files	Coverage Δ
packages/expect/src/jasmineUtils.js	84.87% <100%> (+1.25%)	⬆️

---

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 b167892...266b0b0. Read the comment docs.

[jeysal]

Should get someone with high reach to Twitter poll this for a highly scientific evaluation of the two options ^^

[jeysal]

@pedrottimark @thymikee @rickhanlonii anyone with a strong opinion about this? 😄

[pedrottimark]

Please forgive the delay to review this important pull request.

I verified that code is adapted from Underscore and in version 1.9.1 _.toEqual returns inconsistent results for the test case in the issue.

To make sure I review according to correct assumptions, corresponding properties:

• are equal if both values are circular references (but see below)
• are not equal if one value is circular and the other value is not circular (the original test case)

It looks like recent versions of node provide a test that behaves correctly:

• util.isDeepStrictEqual in version 9.0.0 and later
• assert.deepStrictEqual in version 9.9.0 and later

Is that from where you adapted the code?

While I dig into the code, let’s add a few more tests:

• .toEqual if both values are circular references (with same circularity)
• .not.toEqual if both values are circular references (with different circularity, see below)

I expected node to consider these equal:

const a = {};
a.x = { y: a };
const b = {};
const bx = {};
b.x = bx;
bx.y = bx;

because a.x.y and b.x.y it detects both as [Circular] according to util.format

{ x: { y: [Circular] } }
{ x: { y: [Circular] } }

but it considers them unequal, so let’s see what the updated code for Jest does in that case.

[grosto]

Thank you for your review.

Yes, I have adapted the code from the latest version of node.

There is one more assumption:

• objects, to which circular references are pointing to, must have the same position in depth too.

I will demonstrate with your example.

const a = {};
a.x = { y: a };
const b = {};
const bx = {};
b.x = bx;
bx.y = bx;

In a circular reference is pointing to a itself, which has a position of 0. In b however, the circular reference is pointing to position bx which is at position 1. This is how modified util.format string would look like, if they would take this into account.

{ x: { y: [CircularTo0] } }
{ x: { y: [CircularTo1] } }

The position is important because if we change [Circular] with reference objects, these two objects would not be equal anymore, because their shapes would differ.

{ x: { y: { x: { y: [Circular] } } } }
{ x: { y: { y: [Circular] } } }

This is my mental model. I expect that if .isEqual returns true for an object, then their string representation must be equal. So, if the strings(the ones including position) of cyclic objects are equal, then they should be considered equal by .isEqual.

These are just my assumptions and I might be missing something. I looked into node's test cases and discovered that they do not have the same assumptions.

They consider this case to be equal.

const a = {};
const b = {};
a.a = a;
b.a = {};
b.a.a = a;

{ a: [Circular] }
{ a: { a: { a: [Circular] } } }

Currently, algorithm in this PR behaves exactly the same way as node's one. I will make adjustments and add extra tests later today or tomorrow.

[jeysal]

This is my mental model. I expect that if .isEqual returns true for an object, then their string representation must be equal.

BTW this is the part where I disagree about the expected behavior. I expect toEqual (unlike toBe) to ignore referential equality, instead looking only at the object shapes.
Node stringifies these objects in a way that does care about referential equality, that's how it decides at what level to stop printing the nested structure.
Therefore it is IMO not okay to define the way toEqual works (shape) based on the way toString works (identity).

(This is such a perfect topic for bikeshedding 👌😅)

[grosto]

Updated tests and algorithm.

[jeysal]

https://twitter.com/_jeysal_/status/1092792746488799233 can get us an overview of what's intuitive for most people, RTed by @cpojer and @rickhanlonii for reach

[satya164]

@jeysal tbh the example code in the tweet will affect the perception without more context. most of the time the object will come from some other function, and how the object was created won't matter. i think the behaviour described in jest's documentation to compare recursively all properties of object instances (also known as "deep" equality). should how it behave (i.e. no referential equality).

[jeysal]

@satya164 Twitter polls will always have some flaws, but I think it's still a good way to find out about intuition. If we discuss in more detail here and get to agree on some super important argument why it should be one way or the other, we can still deviate from the poll result.

[gokulkrishh]

@jeysal Hi, twitter is not the ideal way to find it out but it gives an idea.

I agree with @satya164, I don't care much about the referential equality check but its all property checks.

Eager to find what people are thinking about this 🥇

[jeysal]

Would be cool to see stats on how long people voting for either option looked at the poll. My guess is that most people think "fail" at first because, as @satya164 said, the object structure is built in such a different way, but then sway over to "pass" after thinking about the structure for a while.

[gokulkrishh]

@jeysal Yep. I feel the same way. Lets wait and see :D

[jeysal]

Poll is 51% fail vs 19% pass, indicating that failing the assertion is likely more intuitive, as inaccurate as Twitter polling may be.
Some have pointed out that after some more thinking about it, they started to see why it should fail, as do the people who came here to leave comments.

[grosto]

@jeysal I agree with you that twitter poll can be quite inaccurate.

I am waiting a review for now, but just to clarify your suggestion is following.

expect(a).toEqual(b)
expect(a).not.toStrictEqual(b)

Did I get it right?

[jeysal]

Did I get it right?

I think both toEqual and toStrictEqual should pass.

Either way, we should definitely not introduce more differences between toEqual and toStrictEqual. The only reason why toEqual exists is that a lot of existing tests (including at Facebook) rely on its behavior. If we wrote expect from scratch, toEqual would always behave like toStrictEqual and toStrictEqual wouldn't exist.

[pedrottimark]

@grosto @jeysal

Finally, I followed link to Node.js code and studied its history for a few hours.

Also I noticed a bullet point under Comparison details for assert module:

Recursion stops when both sides differ or both sides encounter a circular reference.

I feel pulled in 2 directions:

• now that util module has isDeepStrictEqual there is a cost of confusion if Jest isn’t consistent
• goal of Node.js implementation is to avoid stack overflow, but not very concerned (that I could understand) with meaning of true versus false return value when it encounters a cycle

By the way, it looks like #7555 is like a test case from Node.js when it had become inconsistent.

I think I misunderstood a quote from #7730 (comment)

objects, to which circular references are pointing to, must have the same position in depth too.

Is that a statement of an intuitive (to us) requirement that node algorithm does not enforce?

And is the change in most recent commit intended to enforce it:

if (bMemoA !== undefined || bMemoB !== undefined) {
  return bMemoA === bMemoB;
}

In several pages of search results, the only relevant work that I could find was for Scheme:
Efficient Nondestructive Equality Checking for Trees and Graphs

Although I keep tripping on examples where isDeepStrictEqual returns true when I expected obviously false I will change gear:

• for examples where I expect it to return true verify that it does, and then see if it returns false after I change data structure according to plausible coding error?
• look at results from concordance package which ava uses: did y’all look at what it does?

[jeysal]

concordance.compare considers the values different.
concordance.diff prints the rather funny diff

  {
    x: {
-     y: [Circular],
+     y: [Circular],
    },
  }

[pedrottimark]

The code called to my mind again before I turn my attention to more realistic test cases.

The concordance package returns unequal for cycle compared to non-cycle:

    let result
    if (lhsCircular.has(lhs)) {
      result = lhsCircular.get(lhs) === rhsCircular.get(rhs)
        ? DEEP_EQUAL
        : UNEQUAL
    } else if (rhsCircular.has(rhs)) {
      result = UNEQUAL
    } else {
      result = lhs.compare(rhs)
    }

That makes intuitive sense to me and the most recently committed change seems equivalent.

Node.js ignores cycle to non-cycle and only compares cycle to cycle:

    const val2MemoA = memos.val1.get(val1);
    if (val2MemoA !== undefined) {
      const val2MemoB = memos.val2.get(val2);
      if (val2MemoB !== undefined) {
        return val2MemoA === val2MemoB;
      }
    }

[pedrottimark]

@grosto Have thought about this more slowly than oil paint drying on a canvas. I like your early intuition in #7555 (comment) with Object.is instead of == operator. Here is a rough draft. Please improve it:

  while (length--) {
    // Linear search. Performance is inversely proportional to the number of
    // unique nested structures.
    // circular is equal to circular at same depth
    // circular is not equal to non-circular
    if (Object.is(aStack[length], a)) {
      return Object.is(bStack[length], b);
    } else if (Object.is(bStack[length], b)) {
      return false;
    }
  }

If testing circular to non-circular in only one direction is the root of the problem, then Node.js code has same problem (at the time of writing this comment) and makes expect package depend on Map type.

To support filtering now and into the future, can you move the new tests into a describe('circular', () => { … }) block, and then cut out repeated phrase in test names?

The expect package is now TypeScript, so you can merge changes from master into your branch, and then resolve any conflicts in jasmineUtils.ts file (your local git understands renaming, but if your editor doesn’t, then close renamed .js files without saving changes).

After so many changes in recent days: yarn clean-all && yarn before yarn test

[grosto]

Sorry for being absent. I was traveling last week.

First of all, thank you for all your input.

Yes, you are right. If we are doing circular to non-circular testing for both arguments anyway, there is no need for copying node's implementation. I agree that just adding another check will be the best solution.

I will update PR in several hours.

[pedrottimark]

Now that I think of it, you are free to rewrite as === strict equality if you prefer. After I wrote the comment, I realized that all three of == or === or Object.is might work the same for object instances, but we will steadily move away from == loose equality so readers don’t stop to think.

A note in case we ever rewrite the array stack as Map for performance reason (correctness comes first). Something that confused me about Node.js code was unbounded incrementing of counter values. The concordance package uses Map with values that correspond to depth in a stack.

Sorry for being absent. I was traveling last week.

I am sorry that issue and PR simmered in my mental slow cooker until all the water boiled away.

This has been amazing start to 2019. People are opening issues about correctness of confusing parts of expect package that I had to put aside because they bounced off my eyes when I did code review in early January to prepare for #7893 and people like you are taking a share of the work!

[pedrottimark]

When I added temporary console.log to see length index at which rough draft of improved loop returns, I noticed that all our examples are at 0 (root object level for received or expected, or both).

Can you adjust one of the existing cases (but not the one that corresponds to the issue) so the circularly referenced objects are one level deeper?

[SimenB]

better typing makes me happy! 🙏

[pedrottimark]

Yes, he committed it better than he found it!

[grosto]

better typing makes me happy! 🙏

Same here 🙌

[pedrottimark]

To make explicit guarantee that the comparison is symmetrical, do we want to double up on all the assertions?

Like the pair in the last test case.

[grosto]

Fair point.
I will add another assertion to the test cases in which cyclic objects have the same shape.

[pedrottimark]

Let’s be explicit both for positive .toEqual or negative .not.toEqual There have been multiple new issues this week with inconsistent asymmetrical negative assertions in expect package.

[pedrottimark]

Cool. I like it.

[pedrottimark]

It looks clear and correct. Deciding what is correct was the hardest work, heh?

[grosto]

It looks clear and correct. Deciding what is correct was the hardest work, heh?

Happy to hear that. "Just" several lines of code but thought me a lot.

[jeysal]

Although I would have liked Jest to be the first to (IMO correctly) consider such structures equal, I can see why just the consistency with Node assert is a good enough reason not to do it. Code LGTM!

[pedrottimark]

@jeysal When we improve the reports when toEqual and toStrictEqual matchers fail, for sure we will want to pay attention to serialization and diff for objects with circular references.

[github-actions]

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.