Skip to content

assert: avoid expensive diff for large values#62798

Open
semimikoh wants to merge 2 commits intonodejs:mainfrom
semimikoh:fix-assert-large-diff
Open

assert: avoid expensive diff for large values#62798
semimikoh wants to merge 2 commits intonodejs:mainfrom
semimikoh:fix-assert-large-diff

Conversation

@semimikoh
Copy link
Copy Markdown
Contributor

@semimikoh semimikoh commented Apr 18, 2026

This limits the input size used for simple assertion diffs.

When an assertion fails for large inspected values, such as
assert.strictEqual(Buffer.alloc(n), [buffer]), AssertionError currently
runs the full line-based Myers diff over the inspected output. For large
Buffers this can make the failure path much slower than linear even though the
values are trivially different.

This change keeps the existing rich diff behavior for smaller values, but skips
the Myers diff for very large inspected outputs in the default simple diff
mode. In that case, the error message includes a truncated representation of
both sides and marks skipped lines instead.

diff: 'full' remains available for callers that explicitly request the full
diff.

Refs: #62792

Test

  • Added coverage in test/parallel/test-assert-deep.js
  • Not run locally: this machine has Apple clang 16.0.0 / Command Line Tools
    16.0, while this tree requires a newer macOS toolchain. Local build fails in
    V8 because std::atomic_ref is unavailable.

@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. labels Apr 18, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 18, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.61%. Comparing base (d080801) to head (8f55a99).
⚠️ Report is 62 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/assert/assertion_error.js 80.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62798      +/-   ##
==========================================
- Coverage   91.55%   89.61%   -1.95%     
==========================================
  Files         355      706     +351     
  Lines      149381   219161   +69780     
  Branches    23364    41997   +18633     
==========================================
+ Hits       136765   196391   +59626     
- Misses      12354    14677    +2323     
- Partials      262     8093    +7831     
Files with missing lines Coverage Δ
lib/internal/assert/assertion_error.js 95.07% <80.00%> (+2.66%) ⬆️

... and 481 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread lib/internal/assert/assertion_error.js Outdated
diffType !== 'full' &&
inspectedSplitActual.length + inspectedSplitExpected.length > kMaxDiffLineCount
) {
message = `\n${colors.green}+${colors.white} ${getTruncatedDiffValue(inspectedSplitActual)}\n` +
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hardcoded colors bypasses the partialDeepStrictEqual coloring convention

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the truncated diff path to follow the partialDeepStrictEqual coloring convention.

Comment thread lib/internal/assert/assertion_error.js Outdated
@@ -213,6 +222,13 @@ function createErrDiff(actual, expected, operator, customMessage, diffType = 'si
message = ArrayPrototypeJoin(inspectedSplitActual, '\n');
}
header = '';
} else if (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can completely hide the real diff when users actually need it.

Copy link
Copy Markdown
Contributor Author

@semimikoh semimikoh Apr 20, 2026

Choose a reason for hiding this comment

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

Narrowed the skip path to large root-level mismatches only. Large values with the same root representation still use the normal line diff, and I added a regression test for that case.

@BHUVANSH855
Copy link
Copy Markdown

I think we can further optimize this by short-circuiting diff generation for clearly incompatible types (e.g. Buffer vs Array).

Right now, even in trivial mismatches, we still go through inspection and diff generation which is expensive for large values.

Would it make sense to add a fast-path before diffing, something like:

  • If types differ fundamentally → skip diff entirely
  • If size exceeds threshold → fallback to truncated diff (current approach)

This could avoid unnecessary work and improve performance beyond just limiting diff size.

@semimikoh semimikoh force-pushed the fix-assert-large-diff branch from f41f6cf to 00c4485 Compare April 20, 2026 07:51
@semimikoh semimikoh force-pushed the fix-assert-large-diff branch from 00c4485 to 8f55a99 Compare April 20, 2026 07:54
@joepie91
Copy link
Copy Markdown
Contributor

@BHUVANSH855 As the original reporter of the issue, that's what I also would've expected the behaviour to be.

Thinking about it more, I'm actually slightly confused as to why it even tries to diff the content in the first place in strictEqual, given that it implements an identity check for objects, not a recursive check of their contents. As a user, I wouldn't expect it to do more than a best-effort visualization of the difference, maybe not even more than a "types mismatch" error (though if cheap enough to compute, a content preview can still be helpful).

With deepStrictEqual it makes more sense to try and visualize the content diff, though in that case it might be difficult to spot structural mismatches (extra or missing wrapper data structure) if it simply shows the two sides in an abbreviated manner; like in the original case, a single [ difference is easy to overlook if there's no diff coloring.

@semimikoh Would it perhaps make more sense to do an "until first mismatch" diff (maybe with a minimum of 5 lines or so) of the two inputs, and then indicating skipped lines for the remainder? That way it could still visualize and colorize the specific differences, highlighting eg. structural differences too, but without running a full diff. IIRC a Myers diff is very fast on long matching sequences, but I'm not sure how it's implemented here and whether it's possible to stop it early for example.

@BHUVANSH855
Copy link
Copy Markdown

@BHUVANSH855 As the original reporter of the issue, that's what I also would've expected the behaviour to be.

Thinking about it more, I'm actually slightly confused as to why it even tries to diff the content in the first place in strictEqual, given that it implements an identity check for objects, not a recursive check of their contents. As a user, I wouldn't expect it to do more than a best-effort visualization of the difference, maybe not even more than a "types mismatch" error (though if cheap enough to compute, a content preview can still be helpful).

With deepStrictEqual it makes more sense to try and visualize the content diff, though in that case it might be difficult to spot structural mismatches (extra or missing wrapper data structure) if it simply shows the two sides in an abbreviated manner; like in the original case, a single [ difference is easy to overlook if there's no diff coloring.

@semimikoh Would it perhaps make more sense to do an "until first mismatch" diff (maybe with a minimum of 5 lines or so) of the two inputs, and then indicating skipped lines for the remainder? That way it could still visualize and colorize the specific differences, highlighting eg. structural differences too, but without running a full diff. IIRC a Myers diff is very fast on long matching sequences, but I'm not sure how it's implemented here and whether it's possible to stop it early for example.

Thanks for the insights! That makes sense regarding visualization expectations.

Building on my earlier thought — instead of completely skipping diff for incompatible types, maybe we could introduce a lightweight fast-path:

  • Detect obvious type mismatch early (e.g., Buffer vs Array)
  • Provide a short, highlighted summary (type + preview)
  • Skip full diff unless explicitly requested (diff: 'full')

This way:

  • We avoid expensive diff computation
  • Still retain useful debugging context
  • Keep behavior consistent with developer expectations

Would this approach align better with the intended behavior?

@semimikoh
Copy link
Copy Markdown
Contributor Author

@BHUVANSH855 @joepie91
Thanks, that makes sense. I can see the case for a bounded "until first mismatch" diff here as well.

It does feel like a somewhat larger behavioral change, though, so I'm not sure whether it makes sense to include it in this PR or handle it separately as a follow-up. For this PR, I was trying to keep the scope to avoiding the pathological full diff for large values while preserving the current behavior as much as possible.

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. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants