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: make assertion_error user myers diff algorithm #54862

Merged

Conversation

puskin94
Copy link
Contributor

@puskin94 puskin94 commented Sep 9, 2024

Fixes: #51733

Co-Authored-By: Pietro Marchini [email protected]

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:

  1. 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:

image

New implementation:

image

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:

image

New implementation:

image
  1. 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:

image

New implementation:

image

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);
image
  1. The new implementation is faster than the old one.
  2. Standardized spacings, indendations and new lines in the diff output.

@nodejs-github-bot 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 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
Copy link

codecov bot commented Sep 9, 2024

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
Copy link
Member

targos commented Sep 9, 2024

@nodejs/assert

@puskin94 puskin94 force-pushed the make-assert-errors-use-myers-diff branch from 16a54cb to 537b61f Compare September 9, 2024 16:58
@RedYetiDev RedYetiDev added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 9, 2024
@RedYetiDev
Copy link
Member

The new implementation is faster than the old one.

needs-benchmark-ci

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
@RedYetiDev
Copy link
Member

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

@puskin94
Copy link
Contributor Author

puskin94 commented Sep 9, 2024

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 😄

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

lpinca commented Sep 9, 2024

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

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

puskin94 commented Sep 9, 2024

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

oops, good catch! fixed 😄

@RedYetiDev
Copy link
Member

nit: Commit mysers -> Myers

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

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

@puskin94 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
Copy link
Member

RedYetiDev commented Sep 15, 2024

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.

Copy link
Member

@RedYetiDev RedYetiDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these prototypes needed?

+ one question

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
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 puskin94 force-pushed the make-assert-errors-use-myers-diff branch from 86d1745 to 27c553c Compare September 15, 2024 20:24
@puskin94
Copy link
Contributor Author

puskin94 commented Sep 18, 2024

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

@pmarchini
Copy link
Member

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

@BridgeAR
Copy link
Member

BridgeAR commented Oct 8, 2024

I think a few more persons should chime in to provide an opinion about the colored string comparison. The example above is a bit extreme and we might need a heuristic to limit that to e.g., a percentage of the input that may be different or a fixed number of changes before it falls back to the stacked version.

@puskin94 puskin94 force-pushed the make-assert-errors-use-myers-diff branch from 904644b to 9899708 Compare October 9, 2024 10:53
@puskin94
Copy link
Contributor Author

puskin94 commented Oct 9, 2024

More examples for the string colored Myers diff:

image

And this is a longer example with an example I found online filled with grammatical errors:

image

@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 13, 2024
@BridgeAR
Copy link
Member

I marked this as semver-minor due to the change how strings would be compared. I do believe the contribution can be considered a new feature.

@BridgeAR
Copy link
Member

@MoLow @mcollina @jasnell @atlowChemi @aduh95 PTAL

You recently reviewed other assert PRs, so I thought it's good to give you a ping. A main focus would be the new string formatting. The other changes are just improvements that will limit the overall output for multiple cases.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the lts-watch-v20.x PRs that may need to be released in v20.x label Oct 15, 2024
@RedYetiDev 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2024
@nodejs-github-bot
Copy link
Collaborator

@puskin94 puskin94 force-pushed the make-assert-errors-use-myers-diff branch from 9899708 to ca1b5bc Compare October 15, 2024 17:20
@puskin94 puskin94 force-pushed the make-assert-errors-use-myers-diff branch from ca1b5bc to 508d1fe Compare October 15, 2024 17:39
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@pmarchini pmarchini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@puskin94 puskin94 changed the title assert: make assertion_error user mysers diff algorithm assert: make assertion_error user myers diff algorithm Oct 16, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 17, 2024
@nodejs-github-bot 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 nodejs-github-bot merged commit 4d6d7d6 into nodejs:main Oct 17, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 4d6d7d6

aduh95 pushed a commit that referenced this pull request Oct 19, 2024
Fixes: #51733

Co-Authored-By: Pietro Marchini <[email protected]>
PR-URL: #54862
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
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. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lts-watch-v20.x PRs that may need to be released in v20.x needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unexpected diff on assert.deepStrictEqual
8 participants