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

assert.deepStrictEqual diff confusion #22763

Closed
targos opened this issue Sep 8, 2018 · 8 comments
Closed

assert.deepStrictEqual diff confusion #22763

targos opened this issue Sep 8, 2018 · 8 comments
Assignees
Labels
assert Issues and PRs related to the assert subsystem.

Comments

@targos
Copy link
Member

targos commented Sep 8, 2018

See these examples:

First:

assert.deepStrictEqual([1, 2, 1, 2, 1], [1, 1, 1])
AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  [
    1,
+   2,
+   1,
+   2,
-   1,
    1
  ]

It should actually print:

assert.deepStrictEqual([1, 2, 1, 2, 1], [1, 1, 1])
AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  [
    1,
+   2,
    1,
+   2,
    1
  ]

Second example:

> assert.deepStrictEqual([1, { a: true, b: false }, 1], [2, 1, { b: false, a: true }, 2])
AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  [
+   1,
+   {
+     a: true,
+     b: false
+   },
+   1
-   2,
-   1,
-   {
-     a: true,
-     b: false
-   },
-   2
  ]

Should be:

> assert.deepStrictEqual([1, { a: true, b: false }, 1], [2, 1, { b: false, a: true }, 2])
AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  [
-   2,
    1,
    {
      a: true,
      b: false
    },
+   1
-   2
  ]
Already fixed part
$ ./node -e 'assert.deepStrictEqual({a: 1, b: 2}, {a: 1, b: 2, c: 3})'
assert.js:84
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected inputs to be strictly deep-equal:
+ actual - expected

  {
    a: 1,
+   b: 2
-   b: 2,
-   c: 3
  }

I think I would expect more something like this:

+ actual - expected

  {
    a: 1,
    b: 2,
-   c: 3
  }
@targos targos added the assert Issues and PRs related to the assert subsystem. label Sep 8, 2018
@targos
Copy link
Member Author

targos commented Sep 8, 2018

On a related note, I think it's even more confusing when assert.strictEqual is called. Since it is supposed to compare by reference, it doesn't make sense to me to show a diff, because what the developer has to do to fix the error is not change the properties in the object.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2018

This is already on my ToDo list. I had a look at proper diffing algorithms and I'll add that when I find a bit more time. The output could still be improved a lot. This was a quick and simple implementation that improved the former situation a lot and that's why I went for that first.

I'll have a thought about assert.strictEqual() as well.

@BridgeAR BridgeAR self-assigned this Sep 8, 2018
@BridgeAR BridgeAR mentioned this issue Sep 10, 2018
4 tasks
BridgeAR added a commit to BridgeAR/node that referenced this issue Sep 19, 2018
The order option can be used to sort the inspected values in case
they do not rely on their order as arrays. That way the output is
stable no matter of the object property inspection order.

PR-URL: nodejs#22788
Refs: nodejs#22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this issue Sep 19, 2018
The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.

PR-URL: nodejs#22788
Refs: nodejs#22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit to targos/node that referenced this issue Sep 23, 2018
The order option can be used to sort the inspected values in case
they do not rely on their order as arrays. That way the output is
stable no matter of the object property inspection order.

PR-URL: nodejs#22788
Refs: nodejs#22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this issue Sep 24, 2018
Assertion errors that produce a diff show a diff for identical entries
in case one side of the comparison has more object properties than the
other one. Those lines are now taken into account and will not show up
as diverging lines anymore.

Refs: nodejs#22763
@BridgeAR
Copy link
Member

@targos looking at your comment about strictEqual again: in case an object has the same properties but it's not reference equal, the error is going to be:

AssertionError [ERR_ASSERTION]: Inputs identical but not reference equal:

{
  a: 1,
  b: 2,
  c: 3
}

What do you think would be a good output in case of two diverging objects that are not reference equal? Only showing the actual or expected object or not showing any at all removes important information for the user. So the only thing I can think about right now that might improve the situation would be to show both objects at the same time (which the current situation should actually already solve by having a combined view that reduces the overall output length).

Would a different message instead already help? E.g., 'Expected reference equal inputs but got:'

I would only use that message for objects and not when comparing primitives.

BridgeAR added a commit to BridgeAR/node that referenced this issue Sep 24, 2018
In case reference (un)equal objects fail in assertion, it should be
clear that it is about the reference equality and not about the
object properties. This is fixed by improving the message in such
cases.

Refs: nodejs#22763
targos pushed a commit that referenced this issue Sep 24, 2018
The order option can be used to sort the inspected values in case
they do not rely on their order as arrays. That way the output is
stable no matter of the object property inspection order.

Backport-PR-URL: #23039
PR-URL: #22788
Refs: #22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
oyyd pushed a commit to oyyd/node that referenced this issue Sep 25, 2018
The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.

PR-URL: nodejs#22788
Refs: nodejs#22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this issue Sep 27, 2018
In case reference (un)equal objects fail in assertion, it should be
clear that it is about the reference equality and not about the
object properties. This is fixed by improving the message in such
cases.

Refs: nodejs#22763

PR-URL: nodejs#23056
Refs: nodejs#22763
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this issue Sep 27, 2018
The wording seems clearer when using `values` instead of `inputs`.

PR-URL: nodejs#23056
Refs: nodejs#22763
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this issue Sep 27, 2018
Assertion errors that produce a diff show a diff for identical entries
in case one side of the comparison has more object properties than the
other one. Those lines are now taken into account and will not show up
as diverging lines anymore.

Refs: nodejs#22763

PR-URL: nodejs#23048
Refs: nodejs#22763
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this issue Sep 27, 2018
It became hard to follow what was actually happening in the
algorithm. This adds comments to improve the situation.

PR-URL: nodejs#23048
Refs: nodejs#22763
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos
Copy link
Member Author

targos commented Sep 27, 2018

Thanks for the fixes @BridgeAR !

@targos targos closed this as completed Sep 27, 2018
@BridgeAR
Copy link
Member

@targos you are welcome but for me this is still work in progress. There are still a couple of cases that will not produce a nice diff. It is just not that trivial to solve. This applies especially to more complex nested objects. I am going to work on it either way but we could just this issue open as reference point.

@targos
Copy link
Member Author

targos commented Sep 27, 2018

OK, I can reopen. Can you update the OP or post a message with an example that is still problematic?

@targos targos reopened this Sep 27, 2018
@BridgeAR
Copy link
Member

Done. I just updated your post :)

BridgeAR added a commit to BridgeAR/node that referenced this issue Oct 3, 2018
The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.

PR-URL: nodejs#22788
Refs: nodejs#22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this issue Oct 5, 2018
The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.

Backport-PR-URL: #23226
PR-URL: #22788
Refs: #22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this issue Oct 7, 2018
The output is now a tiny bit improved by sorting object properties
when inspecting the values that are compared with each other. That
reduces the overall diff for identical objects with a different
property insertion order.

Backport-PR-URL: #23226
PR-URL: #22788
Refs: #22763
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos
Copy link
Member Author

targos commented Jun 13, 2020

I'm closing this. The current output is not strictly wrong

@targos targos closed this as completed Jun 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants