unexpected diff on assert.deepStrictEqual

[dev-ardi]

Version

v20.5.0 (tried on node 16 and 21)

Platform

([ronment]::OSVersion.VersionString) x64

What steps will reproduce the bug?

require("assert").deepStrictEqual(
  {
    common: {},
    key1: "",
    creator: "123",
  },
  {
    creator: "123",
  },
);

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

Only comon and key1 exist so

  {
+   common: {},
+   key1: ''
  }

What do you see instead?

  {
+   common: {},
+   creator: '123',
+   key1: ''
-   creator: '123'
  }

Additional information

No response

[alepefe]

It happened to me as well.

[BridgeAR]

The current implementation for the diff is a quick best effort implementation and we have to change it to Myers algorithm or similar to properly detect these changes without a bad runtime.

[dev-ardi]

Would a fix to this get added to node 20?

[github-actions]

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

[dev-ardi]

bump

[puskin94]

After spending A LOT of time looking around and trying things, I don't think implementing any standard diff algorithm is going to be perfect for us. Why? Because we have a couple of edgecases which would deviate from any diff algorithm, ending up into something custom anyway:

1. https://github.com/nodejs/node/blob/main/lib/internal/assert/assertion_error.js#L235
2. https://github.com/nodejs/node/blob/main/lib/internal/assert/assertion_error.js#L155

I actually ended up implementing the myerDiff algorithm myself in the assertion_error and (with some caveat) is working fine:

There is a couple of problems tho:

1. right at the beginning of the createErrDiff function, we do this:

  const actualInspected = inspectValue(actual);
  const actualLines = StringPrototypeSplit(actualInspected, '\n');
  const expectedLines = StringPrototypeSplit(inspectValue(expected), '\n');

which converts the input:

{
    common: {},
    key1: "",
    creator: "123",
  }

  {
    creator: "123",
  },

to

[ '{', '  common: {},', "  creator: '123',", "  key1: ''", '}' ]
[ '{', "  creator: '123'", '}' ]

and, if you pay enough attention, in actual (the first array), creator: '123', has a trailing comma, which is not present in the second array, that's why the diff with the already implemented diff looks the way it looks, hence the creation of this issue.

The naive solution to this problem is to do something like this:

    // Remove trailing comma from each line
  if (typeof actual === 'object' && actual !== null) {
    for (let i = 0; i < actualLines.length; i++) {
      actualLines[i] = actualLines[i].replace(/,$/, '');
    }
  }
  if (typeof expected === 'object' && expected !== null) {
    for (let i = 0; i < expectedLines.length; i++) {
      expectedLines[i] = expectedLines[i].replace(/,$/, '');
    }
  }

which is NOT ideal. The problem I am pretty sure should be resolved in the inspect function, but I am not smart enough to both fix the issue and not break anything else in the codebase.

2. in the current implementation there is a gazillion of exceptions just for proper formatting the output: object too nested? hide part of it. Too many consecutive unchanged lines? crop some of them. The terminal is too narrow? ellipse some lines.

I am not sure the whole effort of implementing all these things into the new algorithm is worth it.

To fix the issue OP reported, we need to tweak this check: https://github.com/nodejs/node/blob/main/lib/internal/assert/assertion_error.js#L235

right now only works if expected has the trailing comma, it does not check if the actual has it.

I could make that change and it should solve the issue, without adapting the algorithm I built to cover all the edge cases.

What do you think?

[puskin94]

actually, I worked a lot on it today and for such an example:

const {deepStrictEqual} = require('assert');

let actual = {
  common: {},
  key1: "",
  test: {},
  creator: "123",
};

let expected = {
  creator: "123",
  test: {},
};

for (let i = 0 ; i < 55; i++) {
  actual.test[i] = i;
  expected.test[i] = i;
}

deepStrictEqual(actual, expected);

this is what I get back with the new algorithm:

it behaves a little bit differently than before, but it is working nice so far. I still have many broken tests I need to fix, but it is a working solution.

The main problem I am having is how to tackle point 1 of the issues I mentioned before; if you guys have any idea or suggestion, you know how to find me 😄