From 2d9e87695e16d80ecffc8eab50e9fe1dfa7005f5 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Sat, 9 Dec 2017 19:38:20 -0200
Subject: [PATCH] assert: improve error messages

From now on all error messages produced by `assert` in strict mode
will produce a error diff.

PR-URL: https://github.com/nodejs/node/pull/17615
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
---
 doc/api/assert.md            |  33 ++++++++
 lib/assert.js                |  16 +++-
 lib/internal/errors.js       | 150 ++++++++++++++++++++++++++++++++++-
 test/parallel/test-assert.js | 143 ++++++++++++++++++++++++++++++++-
 4 files changed, 332 insertions(+), 10 deletions(-)

diff --git a/doc/api/assert.md b/doc/api/assert.md
index 9a37d5f26c1..7d07fbbf0c1 100644
--- a/doc/api/assert.md
+++ b/doc/api/assert.md
@@ -17,6 +17,9 @@ For more information about the used equality comparisons see
 <!-- YAML
 added: REPLACEME
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/REPLACEME
+    description: Added error diffs to the strict mode
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/17002
     description: Added strict mode to the assert module.
@@ -26,12 +29,42 @@ When using the `strict mode`, any `assert` function will use the equality used i
 the strict function mode. So [`assert.deepEqual()`][] will, for example, work the
 same as [`assert.deepStrictEqual()`][].
 
+On top of that, error messages which involve objects produce an error diff
+instead of displaying both objects. That is not the case for the legacy mode.
+
 It can be accessed using:
 
 ```js
 const assert = require('assert').strict;
 ```
 
+Example error diff (the `expected`, `actual`, and `Lines skipped` will be on a
+single row):
+
+```js
+const assert = require('assert').strict;
+
+assert.deepEqual([[[1, 2, 3]], 4, 5], [[[1, 2, '3']], 4, 5]);
+```
+
+```diff
+AssertionError [ERR_ASSERTION]: Input A expected to deepStrictEqual input B:
++ expected
+- actual
+... Lines skipped
+
+  [
+    [
+...
+      2,
+-     3
++     '3'
+    ],
+...
+    5
+  ]
+```
+
 ## Legacy mode
 
 > Stability: 0 - Deprecated: Use strict mode instead.
diff --git a/lib/assert.js b/lib/assert.js
index db8ae35e07c..9931ce9c222 100644
--- a/lib/assert.js
+++ b/lib/assert.js
@@ -48,6 +48,10 @@ const meta = [
 
 const escapeFn = (str) => meta[str.charCodeAt(0)];
 
+const ERR_DIFF_DEACTIVATED = 0;
+const ERR_DIFF_NOT_EQUAL = 1;
+const ERR_DIFF_EQUAL = 2;
+
 // The assert module provides functions that throw
 // AssertionError's when particular conditions are not met. The
 // assert module must conform to the following interface.
@@ -283,7 +287,8 @@ assert.deepStrictEqual = function deepStrictEqual(actual, expected, message) {
       expected,
       message,
       operator: 'deepStrictEqual',
-      stackStartFn: deepStrictEqual
+      stackStartFn: deepStrictEqual,
+      errorDiff: this === strict ? ERR_DIFF_EQUAL : ERR_DIFF_DEACTIVATED
     });
   }
 };
@@ -296,7 +301,8 @@ function notDeepStrictEqual(actual, expected, message) {
       expected,
       message,
       operator: 'notDeepStrictEqual',
-      stackStartFn: notDeepStrictEqual
+      stackStartFn: notDeepStrictEqual,
+      errorDiff: this === strict ? ERR_DIFF_NOT_EQUAL : ERR_DIFF_DEACTIVATED
     });
   }
 }
@@ -308,7 +314,8 @@ assert.strictEqual = function strictEqual(actual, expected, message) {
       expected,
       message,
       operator: 'strictEqual',
-      stackStartFn: strictEqual
+      stackStartFn: strictEqual,
+      errorDiff: this === strict ? ERR_DIFF_EQUAL : ERR_DIFF_DEACTIVATED
     });
   }
 };
@@ -320,7 +327,8 @@ assert.notStrictEqual = function notStrictEqual(actual, expected, message) {
       expected,
       message,
       operator: 'notStrictEqual',
-      stackStartFn: notStrictEqual
+      stackStartFn: notStrictEqual,
+      errorDiff: this === strict ? ERR_DIFF_NOT_EQUAL : ERR_DIFF_DEACTIVATED
     });
   }
 };
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 6e6852f380d..6bbb5d3548f 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -132,22 +132,164 @@ class SystemError extends makeNodeError(Error) {
   }
 }
 
+function createErrDiff(actual, expected, operator) {
+  var other = '';
+  var res = '';
+  var lastPos = 0;
+  var end = '';
+  var skipped = false;
+  const actualLines = util
+    .inspect(actual, { compact: false }).split('\n');
+  const expectedLines = util
+    .inspect(expected, { compact: false }).split('\n');
+  const msg = `Input A expected to ${operator} input B:\n` +
+        '\u001b[32m+ expected\u001b[39m \u001b[31m- actual\u001b[39m';
+  const skippedMsg = ' ... Lines skipped';
+
+  // Remove all ending lines that match (this optimizes the output for
+  // readability by reducing the number of total changed lines).
+  var a = actualLines[actualLines.length - 1];
+  var b = expectedLines[expectedLines.length - 1];
+  var i = 0;
+  while (a === b) {
+    if (i++ < 2) {
+      end = `\n  ${a}${end}`;
+    } else {
+      other = a;
+    }
+    actualLines.pop();
+    expectedLines.pop();
+    a = actualLines[actualLines.length - 1];
+    b = expectedLines[expectedLines.length - 1];
+  }
+  if (i > 3) {
+    end = `\n...${end}`;
+    skipped = true;
+  }
+  if (other !== '') {
+    end = `\n  ${other}${end}`;
+    other = '';
+  }
+
+  const maxLines = Math.max(actualLines.length, expectedLines.length);
+  var printedLines = 0;
+  for (i = 0; i < maxLines; i++) {
+    // Only extra expected lines exist
+    const cur = i - lastPos;
+    if (actualLines.length < i + 1) {
+      if (cur > 1 && i > 2) {
+        if (cur > 4) {
+          res += '\n...';
+          skipped = true;
+        } else if (cur > 3) {
+          res += `\n  ${expectedLines[i - 2]}`;
+          printedLines++;
+        }
+        res += `\n  ${expectedLines[i - 1]}`;
+        printedLines++;
+      }
+      lastPos = i;
+      other += `\n\u001b[32m+\u001b[39m ${expectedLines[i]}`;
+      printedLines++;
+    // Only extra actual lines exist
+    } else if (expectedLines.length < i + 1) {
+      if (cur > 1 && i > 2) {
+        if (cur > 4) {
+          res += '\n...';
+          skipped = true;
+        } else if (cur > 3) {
+          res += `\n  ${actualLines[i - 2]}`;
+          printedLines++;
+        }
+        res += `\n  ${actualLines[i - 1]}`;
+        printedLines++;
+      }
+      lastPos = i;
+      res += `\n\u001b[31m-\u001b[39m ${actualLines[i]}`;
+      printedLines++;
+    // Lines diverge
+    } else if (actualLines[i] !== expectedLines[i]) {
+      if (cur > 1 && i > 2) {
+        if (cur > 4) {
+          res += '\n...';
+          skipped = true;
+        } else if (cur > 3) {
+          res += `\n  ${actualLines[i - 2]}`;
+          printedLines++;
+        }
+        res += `\n  ${actualLines[i - 1]}`;
+        printedLines++;
+      }
+      lastPos = i;
+      res += `\n\u001b[31m-\u001b[39m ${actualLines[i]}`;
+      other += `\n\u001b[32m+\u001b[39m ${expectedLines[i]}`;
+      printedLines += 2;
+    // Lines are identical
+    } else {
+      res += other;
+      other = '';
+      if (cur === 1 || i === 0) {
+        res += `\n  ${actualLines[i]}`;
+        printedLines++;
+      }
+    }
+    // Inspected object to big (Show ~20 rows max)
+    if (printedLines > 20 && i < maxLines - 2) {
+      return `${msg}${skippedMsg}\n${res}\n...${other}\n...`;
+    }
+  }
+  return `${msg}${skipped ? skippedMsg : ''}\n${res}${other}${end}`;
+}
+
 class AssertionError extends Error {
   constructor(options) {
     if (typeof options !== 'object' || options === null) {
       throw new exports.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'Object');
     }
-    var { actual, expected, message, operator, stackStartFn } = options;
+    var {
+      actual,
+      expected,
+      message,
+      operator,
+      stackStartFn,
+      errorDiff = 0
+    } = options;
+
     if (message != null) {
       super(message);
     } else {
+      if (util === null) util = require('util');
+
       if (actual && actual.stack && actual instanceof Error)
         actual = `${actual.name}: ${actual.message}`;
       if (expected && expected.stack && expected instanceof Error)
         expected = `${expected.name}: ${expected.message}`;
-      if (util === null) util = require('util');
-      super(`${util.inspect(actual).slice(0, 128)} ` +
-        `${operator} ${util.inspect(expected).slice(0, 128)}`);
+
+      if (errorDiff === 0) {
+        let res = util.inspect(actual);
+        let other = util.inspect(expected);
+        if (res.length > 128)
+          res = `${res.slice(0, 125)}...`;
+        if (other.length > 128)
+          other = `${other.slice(0, 125)}...`;
+        super(`${res} ${operator} ${other}`);
+      } else if (errorDiff === 1) {
+        // In case the objects are equal but the operator requires unequal, show
+        // the first object and say A equals B
+        const res = util
+          .inspect(actual, { compact: false }).split('\n');
+
+        if (res.length > 20) {
+          res[19] = '...';
+          while (res.length > 20) {
+            res.pop();
+          }
+        }
+        // Only print a single object.
+        super(`Identical input passed to ${operator}:\n${res.join('\n')}`);
+      } else {
+        super(createErrDiff(actual, expected, operator));
+      }
     }
 
     this.generatedMessage = !message;
diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
index ccfe276c80a..e7ee71667de 100644
--- a/test/parallel/test-assert.js
+++ b/test/parallel/test-assert.js
@@ -711,7 +711,8 @@ assert.throws(() => {
   assert.strictEqual('A'.repeat(1000), '');
 }, common.expectsError({
   code: 'ERR_ASSERTION',
-  message: new RegExp(`^'${'A'.repeat(127)} strictEqual ''$`) }));
+  message: /^'A{124}\.\.\. strictEqual ''$/
+}));
 
 {
   // bad args to AssertionError constructor should throw TypeError
@@ -752,7 +753,6 @@ common.expectsError(
   assert.equal(assert.notEqual, assert.notStrictEqual);
   assert.equal(assert.notDeepEqual, assert.notDeepStrictEqual);
   assert.equal(Object.keys(assert).length, Object.keys(a).length);
-  /* eslint-enable no-restricted-properties */
   assert(7);
   common.expectsError(
     () => assert(),
@@ -786,6 +786,145 @@ common.expectsError(
     }
   );
   Error.stackTraceLimit = tmpLimit;
+
+  // Test error diffs
+  const start = 'Input A expected to deepStrictEqual input B:';
+  const actExp = '\u001b[32m+ expected\u001b[39m \u001b[31m- actual\u001b[39m';
+  const plus = '\u001b[32m+\u001b[39m';
+  const minus = '\u001b[31m-\u001b[39m';
+  let message = [
+    start,
+    `${actExp} ... Lines skipped`,
+    '',
+    '  [',
+    '    [',
+    '...',
+    '        2,',
+    `${minus}       3`,
+    `${plus}       '3'`,
+    '      ]',
+    '...',
+    '    5',
+    '  ]'].join('\n');
+  assert.throws(
+    () => assert.deepEqual([[[1, 2, 3]], 4, 5], [[[1, 2, '3']], 4, 5]),
+    { message });
+
+  message = [
+    start,
+    `${actExp} ... Lines skipped`,
+    '',
+    '  [',
+    '    1,',
+    '...',
+    '    0,',
+    `${plus}   1,`,
+    '    1,',
+    '...',
+    '    1',
+    '  ]'
+  ].join('\n');
+  assert.throws(
+    () => assert.deepEqual(
+      [1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1],
+      [1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1]),
+    { message });
+
+  message = [
+    start,
+    `${actExp} ... Lines skipped`,
+    '',
+    '  [',
+    '    1,',
+    '...',
+    '    0,',
+    `${minus}   1,`,
+    '    1,',
+    '...',
+    '    1',
+    '  ]'
+  ].join('\n');
+  assert.throws(
+    () => assert.deepEqual(
+      [1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1],
+      [1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1]),
+    { message });
+
+  message = [
+    start,
+    actExp,
+    '',
+    '  [',
+    '    1,',
+    `${minus}   2,`,
+    `${plus}   1,`,
+    '    1,',
+    '    1,',
+    '    0,',
+    `${minus}   1,`,
+    '    1',
+    '  ]'
+  ].join('\n');
+  assert.throws(
+    () => assert.deepEqual(
+      [1, 2, 1, 1, 0, 1, 1],
+      [1, 1, 1, 1, 0, 1]),
+    { message });
+
+  message = [
+    start,
+    actExp,
+    '',
+    `${minus} [`,
+    `${minus}   1,`,
+    `${minus}   2,`,
+    `${minus}   1`,
+    `${minus} ]`,
+    `${plus} undefined`,
+  ].join('\n');
+  assert.throws(
+    () => assert.deepEqual([1, 2, 1]),
+    { message });
+
+  message = [
+    start,
+    actExp,
+    '',
+    '  [',
+    `${minus}   1,`,
+    '    2,',
+    '    1',
+    '  ]'
+  ].join('\n');
+  assert.throws(
+    () => assert.deepEqual([1, 2, 1], [2, 1]),
+    { message });
+
+  message = `${start}\n` +
+    `${actExp} ... Lines skipped\n` +
+    '\n' +
+    '  [\n' +
+    `${minus}   1,\n`.repeat(10) +
+    '...\n' +
+    `${plus}   2,\n`.repeat(10) +
+    '...';
+  assert.throws(
+    () => assert.deepEqual(Array(12).fill(1), Array(12).fill(2)),
+    { message });
+
+  // notDeepEqual tests
+  message = 'Identical input passed to notDeepStrictEqual:\n[\n  1\n]';
+  assert.throws(
+    () => assert.notDeepEqual([1], [1]),
+    { message });
+
+  message = 'Identical input passed to notDeepStrictEqual:' +
+        `\n[${'\n  1,'.repeat(18)}\n...`;
+  const data = Array(21).fill(1);
+  assert.throws(
+    () => assert.notDeepEqual(data, data),
+    { message });
+  /* eslint-enable no-restricted-properties */
 }
 
 common.expectsError(