fixup!

Author: BridgeAR

doc/api/assert.md

@@ -1759,8 +1759,10 @@ changes:
 * `actual` {any}
 * `expected` {any}
 * `message` {string|Error|Function} Postfix `printf`-like arguments in case
-  it's used as format string. If a function is used, the function is called. If
-  that function call fails, the default message is generated instead.
+  it's used as format string.
+  If message is a function, it is called in case of a comparison failure. The
+  function receives the `actual` and `expected` arguments and has to return a
+  string that is going to be used as error message.
   `printf`-like format strings and functions are beneficial for performance
   reasons in case arguments are passed through. In addition, it allows nice
   formatting with ease.
@@ -1800,7 +1802,7 @@ assert.strictEqual(1, '1', new TypeError('Inputs are not identical'));
 
 assert.strictEqual(apples, oranges, () => {
   // Do 'heavy' computations
-  return 'My error string'
+  return 'My error string';
 });
 // AssertionError [ERR_ASSERTION]: apples 1 !== oranges 2
 ```

lib/assert.js

@@ -26,6 +26,7 @@ const {
   ArrayPrototypePush,
   ArrayPrototypeSlice,
   Error,
+  ErrorPrototypeToString,
   NumberIsNaN,
   ObjectAssign,
   ObjectIs,
@@ -88,17 +89,37 @@ const NO_EXCEPTION_SENTINEL = {};
 
 function innerFail(obj) {
   if (obj.message.length === 0) {
-    obj.message = undefined
+    obj.message = undefined;
   } else if (typeof obj.message[0] === 'string') {
     if (obj.message.length > 1) {
       obj.message = format(...obj.message);
     } else {
       obj.message = obj.message[0];
     }
   } else if (isError(obj.message[0])) {
+    if (obj.message.length > 1) {
+      throw new ERR_AMBIGUOUS_ARGUMENT(
+        'message',
+        `The error message was passed as error object "${ErrorPrototypeToString(obj.message[0])}" has trailing arguments that would be ignored.`,
+      );
+    }
     throw obj.message[0];
   } else if (typeof obj.message[0] === 'function') {
-    obj.message = obj.message[0]();
+    if (obj.message.length > 1) {
+      throw new ERR_AMBIGUOUS_ARGUMENT(
+        'message',
+        `The error message with function "${obj.message[0].name || 'anonymous'}" has trailing arguments that would be ignored.`,
+      );
+    }
+    try {
+      obj.message = obj.message[0](obj.actual, obj.expected);
+      if (typeof obj.message !== 'string') {
+        obj.message = undefined;
+      }
+    } catch {
+      // Ignore and use default message instead
+      obj.message = undefined;
+    }
   } else {
     throw new ERR_INVALID_ARG_TYPE(
       'message',
@@ -107,7 +128,11 @@ function innerFail(obj) {
     );
   }
 
-  throw new AssertionError(obj);
+  const error = new AssertionError(obj);
+  if (obj.generatedMessage !== undefined) {
+    error.generatedMessage = obj.generatedMessage;
+  }
+  throw error;
 }
 
 /**
@@ -349,7 +374,7 @@ assert.notStrictEqual = function notStrictEqual(actual, expected, ...message) {
 assert.partialDeepStrictEqual = function partialDeepStrictEqual(
   actual,
   expected,
-  ...message,
+  ...message
 ) {
   if (arguments.length < 2) {
     throw new ERR_MISSING_ARGS('actual', 'expected');
@@ -404,7 +429,7 @@ function compareExceptionKey(actual, expected, key, message, keys, fn) {
     innerFail({
       actual,
       expected,
-      message,
+      message: [message],
       operator: fn.name,
       stackStartFn: fn,
     });
@@ -603,7 +628,7 @@ function expectsError(stackStartFn, actual, error, message) {
       actual: undefined,
       expected: error,
       operator: stackStartFn.name,
-      message: `Missing expected ${fnType}${details}`,
+      message: [`Missing expected ${fnType}${details}`],
       stackStartFn,
     });
   }
@@ -651,8 +676,8 @@ function expectsNoError(stackStartFn, actual, error, message) {
       actual,
       expected: error,
       operator: stackStartFn.name,
-      message: `Got unwanted ${fnType}${details}\n` +
-               `Actual message: "${actual?.message}"`,
+      message: [`Got unwanted ${fnType}${details}\n` +
+               `Actual message: "${actual?.message}"`],
       stackStartFn,
     });
   }
@@ -768,29 +793,25 @@ function internalMatch(string, regexp, message, fn) {
   const match = fn === assert.match;
   if (typeof string !== 'string' ||
       RegExpPrototypeExec(regexp, string) !== null !== match) {
-    if (message instanceof Error) {
-      throw message;
-    }
 
-    const generatedMessage = !message;
+    const generatedMessage = message.length === 0;
 
     // 'The input was expected to not match the regular expression ' +
-    message ||= (typeof string !== 'string' ?
+    message[0] ||= (typeof string !== 'string' ?
       'The "string" argument must be of type string. Received type ' +
         `${typeof string} (${inspect(string)})` :
       (match ?
         'The input did not match the regular expression ' :
         'The input was expected to not match the regular expression ') +
           `${inspect(regexp)}. Input:\n\n${inspect(string)}\n`);
-    const err = new AssertionError({
+    innerFail({
       actual: string,
       expected: regexp,
       message,
       operator: fn.name,
       stackStartFn: fn,
+      generatedMessage: generatedMessage,
     });
-    err.generatedMessage = generatedMessage;
-    throw err;
   }
 }
 

test/parallel/test-assert.js

@@ -1595,5 +1595,148 @@ test('assert/strict exists', () => {
   assert.strictEqual(require('assert/strict'), assert.strict);
 });
 
+test('Printf-like format strings as error message', () => {
+  assert.throws(
+    () => assert.equal(1, 2, 'The answer to all questions is %i', 42),
+    /The answer to all questions is 42/
+  );
+
+  assert.throws(
+    () => assert.strictEqual(1, 2, 'The answer to all questions is %i', 42),
+    /The answer to all questions is 42/
+  );
+
+  assert.throws(
+    () => assert.notEqual(1, 1, 'The answer to all questions is %i', 42),
+    /The answer to all questions is 42/
+  );
+
+  assert.throws(
+    () => assert.notStrictEqual(1, 1, 'The answer to all questions is %i', 42),
+    /The answer to all questions is 42/
+  );
+
+  assert.throws(
+    () => assert.deepEqual(1, 2, 'The answer to all questions is %i', 42),
+    /The answer to all questions is 42/
+  );
+
+  assert.throws(
+    () => assert.notDeepEqual(1, 1, 'The answer to all questions is %i', 42),
+    /The answer to all questions is 42/
+  );
+
+  assert.throws(
+    () => assert.deepStrictEqual(1, 2, 'The answer to all questions is %i', 42),
+    /The answer to all questions is 42/
+  );
+
+  assert.throws(
+    () => assert.notDeepStrictEqual(1, 1, 'The answer to all questions is %i', 42),
+    /The answer to all questions is 42/
+  );
+
+  assert.throws(
+    () => assert.partialDeepStrictEqual(1, 2, 'The answer to all questions is %i', 42),
+    /The answer to all questions is 42/
+  );
+
+  assert.throws(
+    () => assert.match('foo', /bar/, 'The answer to all questions is %i', 42),
+    /The answer to all questions is 42/
+  );
+
+  assert.throws(
+    () => assert.doesNotMatch('foo', /foo/, 'The answer to all questions is %i', 42),
+    /The answer to all questions is 42/
+  );
+});
+
+test('Functions as error message', () => {
+  function errorMessage(actual, expected) {
+    return `Nice message including ${actual} and ${expected}`;
+  }
+  assert.throws(
+    () => assert.equal(1, 2, errorMessage),
+    { message: 'Nice message including 1 and 2' }
+  );
+
+  assert.throws(
+    () => assert.strictEqual(1, 2, errorMessage),
+    // TODO(BridgeAR): Align input arguments with generated message for all
+    // methods.
+    // TODO(BridgeAR): Check how to handle this for custom messages. Do
+    // we need this? Should it be skipped for methods like these?
+    { message: 'Nice message including 1 and 2\n\n1 !== 2\n' }
+  );
+
+  assert.throws(
+    () => assert.notEqual(1, 1, errorMessage),
+    { message: 'Nice message including 1 and 1' }
+  );
+
+  assert.throws(
+    () => assert.notStrictEqual(1, 1, errorMessage),
+    { message: 'Nice message including 1 and 1' }
+  );
+
+  assert.throws(
+    () => assert.deepEqual(1, 2, errorMessage),
+    { message: 'Nice message including 1 and 2' }
+  );
+
+  assert.throws(
+    () => assert.notDeepEqual(1, 1, errorMessage),
+    { message: 'Nice message including 1 and 1' }
+  );
+
+  assert.throws(
+    () => assert.deepStrictEqual(1, 2, errorMessage),
+    { message: 'Nice message including 1 and 2\n\n1 !== 2\n' }
+  );
+
+  assert.throws(
+    () => assert.notDeepStrictEqual(1, 1, errorMessage),
+    { message: 'Nice message including 1 and 1' }
+  );
+
+  assert.throws(
+    () => assert.partialDeepStrictEqual(1, 2, errorMessage),
+    { message: 'Nice message including 1 and 2\n\n1 !== 2\n' }
+  );
+
+  assert.throws(
+    () => assert.match('foo', /bar/, errorMessage),
+    { message: 'Nice message including foo and /bar/' }
+  );
+
+  assert.throws(
+    () => assert.doesNotMatch('foo', /foo/, errorMessage),
+    { message: 'Nice message including foo and /foo/' }
+  );
+});
+
+test('Ambiguous error messages fail', () => {
+  function errorMessage(actual, expected) {
+    return `Nice message including ${actual} and ${expected}`;
+  }
+
+  assert.throws(
+    () => assert.doesNotMatch('foo', /foo/, errorMessage, 'foobar'),
+    {
+      code: 'ERR_AMBIGUOUS_ARGUMENT',
+      message: /errorMessage/
+    }
+  );
+
+  assert.throws(
+    () => assert.doesNotMatch('foo', /foo/, new Error('baz'), 'foobar'),
+    {
+      code: 'ERR_AMBIGUOUS_ARGUMENT',
+      message: /baz/
+    }
+  );
+});
+
 /* eslint-enable no-restricted-syntax */
 /* eslint-enable no-restricted-properties */