assert: deprecate assert.fail partially

Using `assert.fail()` with more than one argument is not intuitive
to use and has no benefit over using a message on its own.

Therefore this introduces a runtime deprecation in case it is used
in that way.

PR-URL: #18418
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Author: BridgeAR

doc/api/assert.md

@@ -415,30 +415,64 @@ parameter is an instance of an [`Error`][] then it will be thrown instead of the
 `AssertionError`.
 
 ## assert.fail([message])
+<!-- YAML
+added: v0.1.21
+-->
+* `message` {any} **Default:** `'Failed'`
+
+Throws an `AssertionError` with the provided error message or a default error
+message. If the `message` parameter is an instance of an [`Error`][] then it
+will be thrown instead of the `AssertionError`.
+
+```js
+const assert = require('assert').strict;
+
+assert.fail();
+// AssertionError [ERR_ASSERTION]: Failed
+
+assert.fail('boom');
+// AssertionError [ERR_ASSERTION]: boom
+
+assert.fail(new TypeError('need array'));
+// TypeError: need array
+```
+
+Using `assert.fail()` with more than two arguments is possible but deprecated.
+See below for further details.
+
 ## assert.fail(actual, expected[, message[, operator[, stackStartFunction]]])
 <!-- YAML
 added: v0.1.21
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/REPLACEME
+    description: Calling `assert.fail` with more than one argument is deprecated
+                 and emits a warning.
 -->
 * `actual` {any}
 * `expected` {any}
-* `message` {any} **Default:** `'Failed'`
+* `message` {any}
 * `operator` {string} **Default:** '!='
 * `stackStartFunction` {Function} **Default:** `assert.fail`
 
-Throws an `AssertionError`. If `message` is falsy, the error message is set as
-the values of `actual` and `expected` separated by the provided `operator`. If
-the `message` parameter is an instance of an [`Error`][] then it will be thrown
-instead of the `AssertionError`. If just the two `actual` and `expected`
-arguments are provided, `operator` will default to `'!='`. If `message` is
-provided only it will be used as the error message, the other arguments will be
-stored as properties on the thrown object. If `stackStartFunction` is provided,
-all stack frames above that function will be removed from stacktrace (see
-[`Error.captureStackTrace`]). If no arguments are given, the default message
-`Failed` will be used.
+> Stability: 0 - Deprecated: Use `assert.fail([message])` or other assert
+> functions instead.
+
+If `message` is falsy, the error message is set as the values of `actual` and
+`expected` separated by the provided `operator`. If just the two `actual` and
+`expected` arguments are provided, `operator` will default to `'!='`. If
+`message` is provided as third argument it will be used as the error message and
+the other arguments will be stored as properties on the thrown object. If
+`stackStartFunction` is provided, all stack frames above that function will be
+removed from stacktrace (see [`Error.captureStackTrace`]). If no arguments are
+given, the default message `Failed` will be used.
 
 ```js
 const assert = require('assert').strict;
 
+assert.fail('a', 'b');
+// AssertionError [ERR_ASSERTION]: 'a' != 'b'
+
 assert.fail(1, 2, undefined, '>');
 // AssertionError [ERR_ASSERTION]: 1 > 2
 
@@ -452,21 +486,11 @@ assert.fail(1, 2, new TypeError('need array'));
 // TypeError: need array
 ```
 
-*Note*: In the last two cases `actual`, `expected`, and `operator` have no
+In the last three cases `actual`, `expected`, and `operator` have no
 influence on the error message.
 
-```js
-assert.fail();
-// AssertionError [ERR_ASSERTION]: Failed
-
-assert.fail('boom');
-// AssertionError [ERR_ASSERTION]: boom
-
-assert.fail('a', 'b');
-// AssertionError [ERR_ASSERTION]: 'a' != 'b'
-```
-
 Example use of `stackStartFunction` for truncating the exception's stacktrace:
+
 ```js
 function suppressFrame() {
   assert.fail('a', 'b', undefined, '!==', suppressFrame);

doc/api/deprecations.md

@@ -847,6 +847,15 @@ Type: Documentation-only
 The [`crypto.fips`][] property is deprecated. Please use `crypto.setFips()`
 and `crypto.getFips()` instead.
 
+<a id="DEP0XX"></a>
+### DEP0XXX: Using `assert.fail()` with more than one argument.
+
+Type: Runtime
+
+Using `assert.fail()` with more than one argument has no benefit over writing an
+individual error message. Either use `assert.fail()` with one argument or switch
+to one of the other assert methods.
+
 [`--pending-deprecation`]: cli.html#cli_pending_deprecation
 [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
 [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array

lib/assert.js

@@ -52,6 +52,8 @@ const ERR_DIFF_DEACTIVATED = 0;
 const ERR_DIFF_NOT_EQUAL = 1;
 const ERR_DIFF_EQUAL = 2;
 
+let warned = false;
+
 // The assert module provides functions that throw
 // AssertionError's when particular conditions are not met. The
 // assert module must conform to the following interface.
@@ -80,8 +82,18 @@ function fail(actual, expected, message, operator, stackStartFn) {
   } else if (argsLen === 1) {
     message = actual;
     actual = undefined;
-  } else if (argsLen === 2) {
-    operator = '!=';
+  } else {
+    if (warned === false) {
+      warned = true;
+      process.emitWarning(
+        'assert.fail() with more than one argument is deprecated. ' +
+          'Please use assert.strictEqual() instead or only pass a message.',
+        'DeprecationWarning',
+        'DEP00XXX'
+      );
+    }
+    if (argsLen === 2)
+      operator = '!=';
   }
 
   innerFail({

test/parallel/test-assert-fail-deprecation.js

@@ -0,0 +1,64 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+
+common.expectWarning(
+  'DeprecationWarning',
+  'assert.fail() with more than one argument is deprecated. ' +
+    'Please use assert.strictEqual() instead or only pass a message.'
+);
+
+// Two args only, operator defaults to '!='
+assert.throws(() => {
+  assert.fail('first', 'second');
+}, {
+  code: 'ERR_ASSERTION',
+  name: 'AssertionError [ERR_ASSERTION]',
+  message: '\'first\' != \'second\'',
+  operator: '!=',
+  actual: 'first',
+  expected: 'second'
+});
+
+// Three args
+assert.throws(() => {
+  assert.fail('ignored', 'ignored', 'another custom message');
+}, {
+  code: 'ERR_ASSERTION',
+  name: 'AssertionError [ERR_ASSERTION]',
+  message: 'another custom message',
+  operator: undefined,
+  actual: 'ignored',
+  expected: 'ignored'
+});
+
+// Three args with custom Error
+assert.throws(() => {
+  assert.fail(typeof 1, 'object', new TypeError('another custom message'));
+}, {
+  name: 'TypeError',
+  message: 'another custom message',
+  operator: undefined,
+  actual: undefined,
+  expected: undefined,
+  code: undefined
+});
+
+// No third arg (but a fourth arg)
+assert.throws(() => {
+  assert.fail('first', 'second', undefined, 'operator');
+}, {
+  code: 'ERR_ASSERTION',
+  name: 'AssertionError [ERR_ASSERTION]',
+  message: '\'first\' operator \'second\'',
+  operator: 'operator',
+  actual: 'first',
+  expected: 'second'
+});
+
+// The stackFrameFunction should exclude the foo frame
+assert.throws(
+  function foo() { assert.fail('first', 'second', 'message', '!==', foo); },
+  (err) => !/^\s*at\sfoo\b/m.test(err.stack)
+);

test/parallel/test-assert-fail.js

@@ -1,95 +1,40 @@
 'use strict';
 
-/* eslint-disable prefer-common-expectserror */
-
-const common = require('../common');
+require('../common');
 const assert = require('assert');
 
 // No args
 assert.throws(
   () => { assert.fail(); },
-  common.expectsError({
+  {
     code: 'ERR_ASSERTION',
-    type: assert.AssertionError,
+    name: 'AssertionError [ERR_ASSERTION]',
     message: 'Failed',
     operator: undefined,
     actual: undefined,
     expected: undefined
-  })
+  }
 );
 
 // One arg = message
-common.expectsError(() => {
+assert.throws(() => {
   assert.fail('custom message');
 }, {
   code: 'ERR_ASSERTION',
-  type: assert.AssertionError,
+  name: 'AssertionError [ERR_ASSERTION]',
   message: 'custom message',
   operator: undefined,
   actual: undefined,
   expected: undefined
 });
 
 // One arg = Error
-common.expectsError(() => {
+assert.throws(() => {
   assert.fail(new TypeError('custom message'));
 }, {
-  type: TypeError,
+  name: 'TypeError',
   message: 'custom message',
   operator: undefined,
   actual: undefined,
   expected: undefined
 });
-
-// Two args only, operator defaults to '!='
-common.expectsError(() => {
-  assert.fail('first', 'second');
-}, {
-  code: 'ERR_ASSERTION',
-  type: assert.AssertionError,
-  message: '\'first\' != \'second\'',
-  operator: '!=',
-  actual: 'first',
-  expected: 'second'
-});
-
-// Three args
-common.expectsError(() => {
-  assert.fail('ignored', 'ignored', 'another custom message');
-}, {
-  code: 'ERR_ASSERTION',
-  type: assert.AssertionError,
-  message: 'another custom message',
-  operator: undefined,
-  actual: 'ignored',
-  expected: 'ignored'
-});
-
-// Three args with custom Error
-common.expectsError(() => {
-  assert.fail(typeof 1, 'object', new TypeError('another custom message'));
-}, {
-  type: TypeError,
-  message: 'another custom message',
-  operator: undefined,
-  actual: 'number',
-  expected: 'object'
-});
-
-// No third arg (but a fourth arg)
-common.expectsError(() => {
-  assert.fail('first', 'second', undefined, 'operator');
-}, {
-  code: 'ERR_ASSERTION',
-  type: assert.AssertionError,
-  message: '\'first\' operator \'second\'',
-  operator: 'operator',
-  actual: 'first',
-  expected: 'second'
-});
-
-// The stackFrameFunction should exclude the foo frame
-assert.throws(
-  function foo() { assert.fail('first', 'second', 'message', '!==', foo); },
-  (err) => !/^\s*at\sfoo\b/m.test(err.stack)
-);