Unsorted keys on test.deepEquals diff.

[Palid]

I've noticed a thing, that makes understanding what happened a little harder than it should. Whenever you deepEqual objects, the keys returned in a diff are not sorted anyhow, which makes diff hardly readable in your object has multiple entries. Simple naive sorting before passing to deepEquals would solve this feature, though it adds on runtime, especially if sorting would go down each level, unless we'd just sort top-level keys.

Current behavior:

    operator: deepEqual
    expected: |-
      { kittens: 4, also: 5, unsorted: 2, keys: 2, here: 3 }
    actual: |-
      { unsorted: 1, keys: 2, here: 3, kittens: 4, also: 5 }

Expected behavior after this issue:

    operator: deepEqual
    expected: |-
      { also: 5, here: 3, keys: 2, kittens: 4, unsorted: 2 }
    actual: |-
      { also: 5, here: 3, keys: 2, kittens: 4, unsorted: 1 }

Simple POC/problem resolution repository: https://github.com/Palid/tape-diff-poc
See files current-behavior.js for current behavior and better-diff-behavior.js for proposed (though naive) resolution.

[ljharb]

Depending on the engine, object key ordering is unreliable; two different objects might indeed have two different orderings, and the inspection results of that should be faithful to the object themselves.

What I can see being useful is, in addition to actual/expected, adding a "diff" result to the report?

[Palid]

Unfortunately adding 'diff' looks like a breaking change (may, and probably will break most of the reporters) and I'm not sure if that's necessary here.
ECMA specification says that keys ordering is not specified, so some browsers actually implement sorting keys alphabetically (I'm looking at you, IE). Other do not sort anyhow.

IMHO in worst case scenario for this change would be a waste of CPU cycles, in best, and the most used one, which is Node.JS environment, it will actually improve the diffs thrown by different tap reporters.

[ljharb]

If you're using a reporter however, can't it sort the keys for you in order to do the diff?

[Palid]

From my understanding it would require reparsing the objects from generated TAP text, which for bigger structures could be resource intensive.
It'd be also problematic whenever you go few levels deep with your objects, as it's not reported well by TAP generated by Tape.
From what I quickly verified on a few of reporters, neither of them resorts the keys and just diffs as-is. Including this change in Tape would be a huge quality of life change for all of the reporters.
I understand that you want to keep Tape small and fast, so maybe hiding it under a configuration flag is not unreasonable?

[ljharb]

What does node’s assert.deepEqual do here?

[Palid]

Node's assert.deepEqual does not sort keys, though jest's toEqual do.
Node:

assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: { unsorted: 1, keys: 2, here: 3, kittens: 4, also: 5 } deepStrictEqual { kittens: 4, also: 5, unsorted: 2, keys: 2, here: 3 }

Jest:

    expect(received).toEqual(expected)

    Expected value to equal:
      {"also": 5, "here": 3, "keys": 2, "kittens": 4, "unsorted": 2}
    Received:
      {"also": 5, "here": 3, "keys": 2, "kittens": 4, "unsorted": 1}

    Difference:

    - Expected
    + Received

      Object {
        "also": 5,
        "here": 3,
        "keys": 2,
        "kittens": 4,
    -   "unsorted": 2,
    +   "unsorted": 1,
      }

Jest code tested:

  it('should show deep equals diff correctly', () => {
    const a = {
      unsorted: 1,
      keys: 2,
      here: 3,
      kittens: 4,
      also: 5
    };
    const b = {
      kittens: 4,
      also: 5,
      unsorted: 2,
      keys: 2,
      here: 3,
    };
    expect(a).toEqual(b);
  });

I'd really lean towards jest's behavior, as it's much easier to understand and work with.

[ljharb]

That's fair; but this package tries particularly hard to adhere to assert's API.

I looked into this; but since this package uses https://github.com/substack/object-inspect, I think that change would have to be made there - potentially behind an option. It'd be a fairly complex change, as well.

[ljharb]

tape matches node's assert, so I think the change would need to be first made in node before we could make it here.