assert: make assertion_error user myers diff algorithm #54862

puskin94 · 2024-09-09T13:28:43Z

Co-Authored-By: Pietro Marchini pietro.marchini94@gmail.com

Why

As discussed in the linked issue, the diff algorithm used to display errors during assertions was a "quick, best-effort implementation." After researching, I found that there aren't many diff algorithms faster than Myers' algorithm, which I’ve implemented in this PR.

How

Following this article on Myers' diff algorithm: https://blog.jcoglan.com/2017/02/12/the-myers-diff-algorithm-part-1/, I implemented the algorithm. Myers' algorithm is relatively simple, based on a matrix of size N * M, where N is the length of the first string and M is the length of the second string. Although it’s primarily used for string comparison, I’ve adapted it here to handle the results of the assert function.

Differences with the old implementation

In addition to changing the diff algorithm, I also updated the way diffs are displayed:

Myers' algorithm excels at finding the minimal changes required to transform one string into another, making it ideal for diffing.

example:

const assert = require('assert');

const actual = {
  a: {
    b: {
      c: 1
    }
  }
};
const expected = {
  a: {
    b: {
      c: 1
    },
    d: {
      c: 1
    }
  }
};

assert.strictEqual(actual, expected);

Old implementation:

New implementation:

Another example:

const assert = require('assert');

const actual = {a: ['AAAA', '12345', '6789', 'BC!!!GHI']};
const expected = {a: ['12345', '6789', 'AAAA', 'BC!!!GHI']};

assert.deepStrictEqual(actual, expected);

Old implementation:

New implementation:

The ^ indicator is now only displayed for string diffs when there is sufficient space in the terminal and colors are not supported, otherwise the new Myers algorithm is used for single strings too:

example:

const assert = require('assert');

const actual = '123456789ABCDEFGHI';
const expected = '1!3!5!7!9!BC!!!GHI';

assert.strictEqual(actual, expected);

Old implementation:

New implementation:

OR

const assert = require('assert');

process.env.NODE_DISABLE_COLORS = '1';

const actual = '123456789ABCDEFGHI';
const expected = '12!!5!7!9!BC!!!GHI';

assert.strictEqual(actual, expected);

The new implementation is faster than the old one.
Standardized spacings, indendations and new lines in the diff output.

codecov · 2024-09-09T15:14:03Z

Codecov Report

Attention: Patch coverage is 88.52459% with 35 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (231d5e4) to head (508d1fe).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/assert/assertion_error.js	86.95%	17 Missing and 1 partial ⚠️
lib/internal/assert/myers_diff.js	89.82%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54862      +/-   ##
==========================================
- Coverage   88.41%   88.40%   -0.02%     
==========================================
  Files         652      653       +1     
  Lines      186889   186952      +63     
  Branches    36059    36075      +16     
==========================================
+ Hits       165239   165272      +33     
- Misses      14901    14926      +25     
- Partials     6749     6754       +5

Files with missing lines	Coverage Δ
lib/internal/assert/myers_diff.js	`89.82% <89.82%> (ø)`
lib/internal/assert/assertion_error.js	`95.14% <86.95%> (-4.86%)`	⬇️

... and 38 files with indirect coverage changes

targos · 2024-09-09T15:25:58Z

@nodejs/assert

RedYetiDev · 2024-09-09T17:03:51Z

The new implementation is faster than the old one.

needs-benchmark-ci

lib/internal/assert/myers_diff.js

RedYetiDev · 2024-09-09T17:07:28Z

IMO in the assert documentation, the Myers algorithm should be mentioned as the algorithm in use?

puskin94 · 2024-09-09T17:23:48Z

IMO in the assert documentation, the Myers algorithm should be mentioned as the algorithm in use?

this is not changing what algorithm assert is using, but the algorithm to show the differences when an assertion fails 😄

lpinca · 2024-09-09T20:23:26Z

There is a typo in commit title: user -> use.

puskin94 · 2024-09-09T20:48:09Z

There is a typo in commit title: user -> use.

oops, good catch! fixed 😄

lib/internal/assert/assertion_error.js

RedYetiDev · 2024-09-14T22:07:18Z

nit: Commit mysers -> Myers

puskin94 · 2024-09-15T08:18:15Z

@RedYetiDev updated the messages about skipped lines and the commit message

RedYetiDev · 2024-09-15T19:58:23Z

Per https://openjs-foundation.slack.com/archives/C019Y2T6STH/p1726429431411589:

Is this a breaking change? It modifies the output of an error, an programs may be relying on that output.

RedYetiDev

Are these prototypes needed?

+ one question

lib/internal/assert/myers_diff.js

puskin94 · 2024-09-18T07:02:09Z

Is there anything left on my side to push this forward?

pmarchini · 2024-09-18T07:54:01Z

Maybe @BridgeAR could be interested in this, considering this comment: #51733 (comment) 😁

nodejs-github-bot · 2024-10-15T16:54:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63124/

Fixes: nodejs#51733 Co-Authored-By: Pietro Marchini <pietro.marchini94@gmail.com>

nodejs-github-bot · 2024-10-15T20:28:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63129/

nodejs-github-bot · 2024-10-16T07:17:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63134/

pmarchini

LGTM

pmarchini · 2024-10-16T12:11:05Z

test/parallel/test-assert-deep.js

@@ -127,7 +134,7 @@ test('date', () => {
      code: 'ERR_ASSERTION',
      message: `${defaultMsgStartFull}\n\n` +
              '+ 2016-01-01T00:00:00.000Z\n- MyDate 2016-01-01T00:00:00.000Z' +
-              " {\n-   '0': '1'\n- }"
+              " {\n-   '0': '1'\n- }\n"


nit: I've noticed that we now have several changes related to the trailing newline, was this introduced intentionally? 😁

@pmarchini yessir, it is point 5 and it got approved by @BridgeAR as well :)

mcollina

lgtm

nodejs-github-bot · 2024-10-17T16:02:56Z

Landed in 4d6d7d6

Fixes: #51733 Co-Authored-By: Pietro Marchini <pietro.marchini94@gmail.com> PR-URL: #54862 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable changes: assert: * (SEMVER-MINOR) make `assertion_error` use Myers diff algorithm (Giovanni Bucci) #54862 buffer: * (SEMVER-MINOR) make `Buffer` work with resizable `ArrayBuffer` (James M Snell) #55377 esm: * mark import attributes and JSON module as stable (Nicolò Ribaudo) #55333 lib: * (SEMVER-MINOR) add `UV_UDP_REUSEPORT` for udp (theanarkh) #55403 net: * (SEMVER-MINOR) add `UV_TCP_REUSEPORT` for tcp (theanarkh) #55408 test_runner: * mark `MockTimers` as stable (Erick Wendel) #55398 PR-URL: TODO

Notable changes: assert: * (SEMVER-MINOR) make `assertion_error` use Myers diff algorithm (Giovanni Bucci) #54862 buffer: * (SEMVER-MINOR) make `Buffer` work with resizable `ArrayBuffer` (James M Snell) #55377 esm: * mark import attributes and JSON module as stable (Nicolò Ribaudo) #55333 lib: * (SEMVER-MINOR) add `UV_UDP_REUSEPORT` for udp (theanarkh) #55403 net: * (SEMVER-MINOR) add `UV_TCP_REUSEPORT` for tcp (theanarkh) #55408 test_runner: * mark `MockTimers` as stable (Erick Wendel) #55398 PR-URL: #55513

Fixes: nodejs#51733 Co-Authored-By: Pietro Marchini <pietro.marchini94@gmail.com> PR-URL: nodejs#54862 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable changes: assert: * (SEMVER-MINOR) make `assertion_error` use Myers diff algorithm (Giovanni Bucci) nodejs#54862 buffer: * (SEMVER-MINOR) make `Buffer` work with resizable `ArrayBuffer` (James M Snell) nodejs#55377 esm: * mark import attributes and JSON module as stable (Nicolò Ribaudo) nodejs#55333 lib: * (SEMVER-MINOR) add `UV_UDP_REUSEPORT` for udp (theanarkh) nodejs#55403 net: * (SEMVER-MINOR) add `UV_TCP_REUSEPORT` for tcp (theanarkh) nodejs#55408 test_runner: * mark `MockTimers` as stable (Erick Wendel) nodejs#55398 PR-URL: nodejs#55513

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Sep 9, 2024

puskin94 force-pushed the make-assert-errors-use-myers-diff branch 2 times, most recently from 13b2f65 to 16a54cb Compare September 9, 2024 13:44

puskin94 force-pushed the make-assert-errors-use-myers-diff branch from 16a54cb to 537b61f Compare September 9, 2024 16:58

RedYetiDev added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 9, 2024

RedYetiDev reviewed Sep 9, 2024

View reviewed changes

lib/internal/assert/myers_diff.js Outdated Show resolved Hide resolved

lib/internal/assert/myers_diff.js Outdated Show resolved Hide resolved

lib/internal/assert/myers_diff.js Outdated Show resolved Hide resolved

puskin94 force-pushed the make-assert-errors-use-myers-diff branch from 537b61f to 0494e00 Compare September 9, 2024 17:35

puskin94 force-pushed the make-assert-errors-use-myers-diff branch from 0494e00 to ddb1cc0 Compare September 9, 2024 20:47

RedYetiDev reviewed Sep 13, 2024

View reviewed changes

lib/internal/assert/assertion_error.js Outdated Show resolved Hide resolved

RedYetiDev mentioned this pull request Sep 14, 2024

Expose createErrDiff for diffing objects #51740

Open

puskin94 force-pushed the make-assert-errors-use-myers-diff branch from ddb1cc0 to 3f36065 Compare September 15, 2024 08:17

puskin94 force-pushed the make-assert-errors-use-myers-diff branch 3 times, most recently from 8a301ae to 86d1745 Compare September 15, 2024 09:25

RedYetiDev reviewed Sep 15, 2024

View reviewed changes

puskin94 force-pushed the make-assert-errors-use-myers-diff branch from 86d1745 to 27c553c Compare September 15, 2024 20:24

mcollina added the lts-watch-v20.x PRs that may need to be released in v20.x label Oct 15, 2024

RedYetiDev added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed review wanted PRs that need reviews. labels Oct 15, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2024

puskin94 force-pushed the make-assert-errors-use-myers-diff branch from 9899708 to ca1b5bc Compare October 15, 2024 17:20

assert: make assertion_error use Myers diff algorithm

508d1fe

Fixes: nodejs#51733 Co-Authored-By: Pietro Marchini <pietro.marchini94@gmail.com>

puskin94 force-pushed the make-assert-errors-use-myers-diff branch from ca1b5bc to 508d1fe Compare October 15, 2024 17:39

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2024

pmarchini approved these changes Oct 16, 2024

View reviewed changes

pmarchini reviewed Oct 16, 2024

View reviewed changes

puskin94 changed the title ~~assert: make assertion_error user mysers diff algorithm~~ assert: make assertion_error user myers diff algorithm Oct 16, 2024

mcollina approved these changes Oct 17, 2024

View reviewed changes

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 17, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 17, 2024

nodejs-github-bot merged commit 4d6d7d6 into nodejs:main Oct 17, 2024
59 checks passed

pmarchini mentioned this pull request Oct 17, 2024

lib: ensure FORCE_COLOR forces color output in non-TTY environments #55404

Merged

puskin94 mentioned this pull request Oct 20, 2024

assert: fix the string length check for printing the simple diff #55474

Merged

aduh95 mentioned this pull request Oct 24, 2024

V23.1.0 proposal #55513

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: make assertion_error user myers diff algorithm #54862

assert: make assertion_error user myers diff algorithm #54862

puskin94 commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

targos commented Sep 9, 2024

RedYetiDev commented Sep 9, 2024

RedYetiDev commented Sep 9, 2024

puskin94 commented Sep 9, 2024

lpinca commented Sep 9, 2024

puskin94 commented Sep 9, 2024

RedYetiDev commented Sep 14, 2024

puskin94 commented Sep 15, 2024

RedYetiDev commented Sep 15, 2024 •

edited

Loading

RedYetiDev left a comment •

edited

Loading

puskin94 commented Sep 18, 2024 •

edited

Loading

pmarchini commented Sep 18, 2024

nodejs-github-bot commented Oct 15, 2024

nodejs-github-bot commented Oct 15, 2024

nodejs-github-bot commented Oct 16, 2024

pmarchini left a comment

pmarchini Oct 16, 2024

puskin94 Oct 16, 2024

mcollina left a comment

nodejs-github-bot commented Oct 17, 2024

assert: make assertion_error user myers diff algorithm #54862

assert: make assertion_error user myers diff algorithm #54862

Conversation

puskin94 commented Sep 9, 2024 • edited Loading

Why

How

Differences with the old implementation

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

targos commented Sep 9, 2024

RedYetiDev commented Sep 9, 2024

RedYetiDev commented Sep 9, 2024

puskin94 commented Sep 9, 2024

lpinca commented Sep 9, 2024

puskin94 commented Sep 9, 2024

RedYetiDev commented Sep 14, 2024

puskin94 commented Sep 15, 2024

RedYetiDev commented Sep 15, 2024 • edited Loading

RedYetiDev left a comment • edited Loading

Choose a reason for hiding this comment

puskin94 commented Sep 18, 2024 • edited Loading

pmarchini commented Sep 18, 2024

nodejs-github-bot commented Oct 15, 2024

nodejs-github-bot commented Oct 15, 2024

nodejs-github-bot commented Oct 16, 2024

pmarchini left a comment

Choose a reason for hiding this comment

pmarchini Oct 16, 2024

Choose a reason for hiding this comment

puskin94 Oct 16, 2024

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 17, 2024

puskin94 commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

RedYetiDev commented Sep 15, 2024 •

edited

Loading

RedYetiDev left a comment •

edited

Loading

puskin94 commented Sep 18, 2024 •

edited

Loading