assert: improve assert.throws

Throw a TypeError in case a error message is provided in the second argument and a third argument is present as well. This is clearly a mistake and should not be done. Backport-PR-URL: #23223 PR-URL: #17585 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

Author: BridgeAR

doc/api/assert.md

@@ -691,17 +691,42 @@ assert.throws(
 
 Note that `error` can not be a string. If a string is provided as the second
 argument, then `error` is assumed to be omitted and the string will be used for
-`message` instead. This can lead to easy-to-miss mistakes:
+`message` instead. This can lead to easy-to-miss mistakes. Please read the
+example below carefully if using a string as the second argument gets
+considered:
 
 <!-- eslint-disable no-restricted-syntax -->
 ```js
-// THIS IS A MISTAKE! DO NOT DO THIS!
-assert.throws(myFunction, 'missing foo', 'did not throw with expected message');
-
-// Do this instead.
-assert.throws(myFunction,/missing foo/, 'did not throw with expected message');
+functionthrowingFirst(){
+thrownewError('First');
+}
+functionthrowingSecond(){
+thrownewError('Second');
+}
+functionnotThrowing(){}
+
+// The second argument is a string and the input function threw an Error.
+// In that case both cases do not throw as neither is going to try to
+// match for the error message thrown by the input function!
+assert.throws(throwingFirst, 'Second');
+assert.throws(throwingSecond, 'Second');
+
+// The string is only used (as message) in case the function does not throw:
+assert.throws(notThrowing, 'Second');
+// AssertionError [ERR_ASSERTION]: Missing expected exception: Second
+
+// If it was intended to match for the error message do this instead:
+assert.throws(throwingSecond,/Second$/);
+// Does not throw because the error messages match.
+assert.throws(throwingFirst,/Second$/);
+// Throws a error:
+// Error: First
+// at throwingFirst (repl:2:9)
 ```
 
+Due to the confusing notation, it is recommended not to use a string as the
+second argument. This might lead to difficult-to-spot errors.
+
 ## Caveats
 
 For the following cases, consider using ES2015 [`Object.is()`][],

lib/assert.js

@@ -675,68 +675,73 @@ function expectedException(actual, expected){
 returnexpected.call({},actual)===true;
 }
 
-functiontryBlock(block){
+functiongetActual(block){
+if(typeofblock!=='function'){
+thrownewerrors.TypeError('ERR_INVALID_ARG_TYPE','block','Function',
+block);
+}
 try{
 block();
 }catch(e){
 returne;
 }
 }
 
-functioninnerThrows(shouldThrow,block,expected,message){
-vardetails='';
+// Expected to throw an error.
+assert.throws=functionthrows(block,error,message){
+constactual=getActual(block);
 
-if(typeofblock!=='function'){
-thrownewerrors.TypeError('ERR_INVALID_ARG_TYPE','block','function',
-block);
-}
+if(typeoferror==='string'){
+if(arguments.length===3)
+thrownewerrors.TypeError('ERR_INVALID_ARG_TYPE',
+'error',
+['Function','RegExp'],
+error);
 
-if(typeofexpected==='string'){
-message=expected;
-expected=null;
+message=error;
+error=null;
 }
 
-constactual=tryBlock(block);
-
-if(shouldThrow===true){
-if(actual===undefined){
-if(expected&&expected.name){
-details+=` (${expected.name})`;
-}
-details+=message ? `: ${message}` : '.';
-innerFail({
- actual,
- expected,
-operator: 'throws',
-message: `Missing expected exception${details}`,
-stackStartFn: assert.throws
-});
-}
-if(expected&&expectedException(actual,expected)===false){
-throwactual;
-}
-}elseif(actual!==undefined){
-if(!expected||expectedException(actual,expected)){
-details=message ? `: ${message}` : '.';
-innerFail({
- actual,
- expected,
-operator: 'doesNotThrow',
-message: `Got unwanted exception${details}`,
-stackStartFn: assert.doesNotThrow
-});
+if(actual===undefined){
+letdetails='';
+if(error&&error.name){
+details+=` (${error.name})`;
 }
+details+=message ? `: ${message}` : '.';
+innerFail({
+ actual,
+expected: error,
+operator: 'throws',
+message: `Missing expected exception${details}`,
+stackStartFn: throws
+});
+}
+if(error&&expectedException(actual,error)===false){
 throwactual;
 }
-}
-
-// Expected to throw an error.
-assert.throws=functionthrows(block,error,message){
-innerThrows(true,block,error,message);
 };
 
 assert.doesNotThrow=functiondoesNotThrow(block,error,message){
-innerThrows(false,block,error,message);
+constactual=getActual(block);
+if(actual===undefined)
+return;
+
+if(typeoferror==='string'){
+message=error;
+error=null;
+}
+
+if(!error||expectedException(actual,error)){
+constdetails=message ? `: ${message}` : '.';
+innerFail({
+ actual,
+expected: error,
+operator: 'doesNotThrow',
+message: `Got unwanted exception${details}\n${actual.message}`,
+stackStartFn: doesNotThrow
+});
+}
+throwactual;
 };
 
 assert.ifError=functionifError(err){if(err)throwerr;};

test/parallel/test-assert.js

@@ -640,7 +640,7 @@ try{
 common.expectsError({
 code: 'ERR_INVALID_ARG_TYPE',
 type: TypeError,
-message: 'The "block" argument must be of type function. Received '+
+message: 'The "block" argument must be of type Function. Received '+
 `type ${typeName(block)}`
 })(e);
 }
@@ -731,3 +731,14 @@ common.expectsError(
 message: 'null == true'
 }
 );
+
+common.expectsError(
+// eslint-disable-next-line no-restricted-syntax
+()=>assert.throws(()=>{},'Error message','message'),
+{
+code: 'ERR_INVALID_ARG_TYPE',
+type: TypeError,
+message: 'The "error" argument must be one of type Function or RegExp. '+
+'Received type string'
+}
+);