Edge case weirdness with new deepEqual setup

[tquetano-r7]

Description

It appears with the new diff mechanism, some very edge-case weirdness is happening. I encountered it a scenario (sorry, this is seriously the least involved example I could reproduce this with) where:

• A specific object is used multiple times
• One of the deeply-nested keys is a string that is dot-separated

js

  const someAction = {payload: 'foo', type: 'BAR'};
  const fakeStuff = [{foo: ['FOO'], name: 'BAR'}];

  const firstItems = [
    someAction,
    someAction,
    {
      payload: {
        fakeStuff,
        'foo.bar': fakeStuff
      },
      type: 'QUZ'
    }
  ];
  const secondItems = [
    {...someAction},
    {...someAction},
    {
      payload: {
        fakeStuff,
        'foo.bar': fakeStuff
      },
      type: 'QUZ'
    }
  ];

  t.deepEqual(firstItems, secondItems);

Errors out:

Rejected promise returned by test. Reason:

TypeError {
  message: 'Cannot read property \'formatDeep\' of undefined',
}

But if you comment out any of the items in the arrays, it passes. Also, if you comment out the 'foo.bar': fakeStuff line from each, it also passes!

Finally ...

firstItems.forEach((item, index) => {
  t.deepEqual(item, secondItems[index]);
});

Also passes. It is literally just the deepEqual of the main objects that doesn't. This is a workaround for now, but this is hinky.

Error Message & Stack Trace

In some cases, the error mentioned above fails with:

Rejected promise returned by test. Reason:

TypeError {
  message: 'Cannot read property \'formatDeep\' of undefined',
}

However in rare cases, i actually get a diff ... where the dot.separated property value is deemed inequal, but it is clearly equal in value. The worst one is when I have a larger array of this issue, where the output I get in the console is:

[
      Object { … },
      Object { … },
      Object { … },
      Object { … },
      Object { … },
  -   Object { … },
      Object { … },
      Object { … },
    ]

Very unhelpful.

Config

Copy the relevant section from package.json:

{
  "ava": {
    "babel": "inherit",
    "concurrency": 5,
    "failFast": true,
    "files": [
      "test/**/*.js"
    ],
    "require": [
      "babel-register",
      "./test/helpers/setup-browser-env.js"
    ],
    "source": [
      "src/**/*.js"
    ],
    "verbose": true
  }
}

Environment

Node.js = 6.10.2
OS = Manjaro Linux
AVA = 0.20.0
npm = 3.10.0
yarn = 0.24.6

[novemberborn]

Wow, nice find. I'll take a look at that!

[novemberborn]

Well this is a tricky one!

Note that though both arrays look the same, there is one major difference. In the first array, items 0 and 1 are exactly the same, whereas in the second they're deeply equal but different objects.

The underlying comparison library describes each value it encounters. Comparisons are done on these descriptor objects. It also detects when object trees contain the same objects (by identity), describing these as pointers. Pointers are expected to be equal.

secondItems[1] is not a pointer, but firstItems[1] is. The arrays would have been equal when secondItems[0] === secondItems[1].

This trips up the payload comparison. The fakeStuff objects, though having the same identity, are assigned different pointers when firstItems and secondItems are described.

When the fakeStuff property is compared it encounters the fakeStuff object for the first time. The object is (of course) equal with itself, so it's formatted without a difference. This registers the pointer for the fakeStuff descriptor as part of firstItems, but neglects to register it for fakeStuff as part of secondItems.

When the foo.bar properties are compared they have pointer values. Because of the secondItems[1] difference these pointers are unequal. The crash occurs because one of the pointers was not registered.

Fixing that leads to another problem: a diff is shown for foo.bar even though the properties are exactly the same! It's secondItems[0] that was expected to be strictly equal to secondItems[1]. I can detect that which could yield this output:

  [
    {
      payload: 'foo',
      type: 'BAR',
    },
-   // Pointer 1
-   {
-     payload: 'foo',
-     type: 'BAR',
-   },
+   // Pointer 2
+   {
+     payload: 'foo',
+     type: 'BAR',
+   },
    {
      payload: {
        fakeStuff: [
          {
            foo: [
              'FOO',
            ],
            name: 'BAR',
          },
        ],

That's pretty confusing! Perhaps it could be improved by including an "address" after the { bit. This doesn't yet solve the foo.bar comparison problem though.

A question for you @tquetano-r7 and also @avajs/core: would you expect t.deepEqual() to care about how objects are reused within the actual and expected values respectively?

If we decide not to care then the pointer stuff can be removed entirely, allowing your test to pass without even having to diff anything. Unfortunately pointers also play a role in circular reference detection so removing it isn't straight-forward either.

[tquetano-r7]

Personally I would not expect deepEqual to care how pointers are used between the compared objects, but they would be considered for how they are used within the same object (for the circular reference reason you describe). That said, the knowledge would only operate insofar as the need to solve circular references and no more ... basically a decycle level of knowledge. Perhaps that is a bit naive, but its what I would expect.

[novemberborn]

Personally I would not expect deepEqual to care how pointers are used between the compared objects, but they would be considered for how they are used within the same object (for the circular reference reason you describe).

Yea, anything else is too dificult.

@tquetano-r7 in your project, could you run npm install concordancejs/concordance#a9e7403857f4093ff562be847d8034f20f551b8a and run your tests? That should install the commit from concordancejs/concordance#33 which should fix your issue.