Assert: Improve rejects/throws validation handling

Gruntfile.js

@@ -136,6 +136,10 @@ module.exports = function( grunt ) {
 				"test/cli/fixtures/only/module.js",
 				"test/cli/fixtures/only/module-flat.js",
 
+				"test/es2018/async-functions.js",
+				"test/es2018/rejects.js",
+				"test/es2018/throws.js"
+
 				// FIXME: These tests use an ugly hack that re-opens
 				// an already finished test run. This only works reliably
 				// via the HTML Reporter thanks to some delays in the bridge.
@@ -149,8 +153,6 @@ module.exports = function( grunt ) {
 				//
 				// "test/module-skip.js",
 				// "test/module-todo.js",
-				"test/es2018/async-functions.js",
-				"test/es2018/throws.js"
 			]
 		}
 	} );

src/assert.js

@@ -292,24 +292,27 @@ class Assert {
 	}
 
 	[ "throws" ]( block, expected, message ) {
-		let actual,
-			result = false;
+
+		[ expected, message ] = validateExpectedExceptionArgs( expected, message, "throws" );
 
 		const currentTest = ( this instanceof Assert && this.test ) || config.current;
 
-		// 'expected' is optional unless doing string comparison
-		if ( objectType( expected ) === "string" ) {
-			if ( message == null ) {
-				message = expected;
-				expected = null;
-			} else {
-				throw new Error(
-					"throws/raises does not accept a string value for the expected argument.\n" +
-					"Use a non-string object value (e.g. regExp) instead if it's necessary."
-				);
-			}
+		if ( objectType( block ) !== "function" ) {
+			const message = "The value provided to `assert.throws` in " +
+				"\"" + currentTest.testName + "\" was not a function.";
+
+			currentTest.assert.pushResult( {
+				result: false,
+				actual: block,
+				message
+			} );
+
+			return;
 		}
 
+		let actual;
+		let result = false;
+
 		currentTest.ignoreGlobalErrors = true;
 		try {
 			block.call( currentTest.testEnvironment );
@@ -319,48 +322,7 @@ class Assert {
 		currentTest.ignoreGlobalErrors = false;
 
 		if ( actual ) {
-			const expectedType = objectType( expected );
-
-			// We don't want to validate thrown error
-			if ( !expected ) {
-				result = true;
-
-			// Expected is a regexp
-			} else if ( expectedType === "regexp" ) {
-				result = expected.test( errorString( actual ) );
-
-				// Log the string form of the regexp
-				expected = String( expected );
-
-			// Expected is a constructor, maybe an Error constructor.
-			// Note the extra check on its prototype - this is an implicit
-			// requirement of "instanceof", else it will throw a TypeError.
-			} else if ( expectedType === "function" &&
-				expected.prototype !== undefined && actual instanceof expected ) {
-				result = true;
-
-			// Expected is an Error object
-			} else if ( expectedType === "object" ) {
-				result = actual instanceof expected.constructor &&
-					actual.name === expected.name &&
-					actual.message === expected.message;
-
-				// Log the string form of the Error object
-				expected = errorString( expected );
-
-			// Expected is a validation function which returns true if validation passed
-			} else if ( expectedType === "function" ) {
-
-				// protect against accidental semantics which could hard error in the test
-				try {
-					result = expected.call( {}, actual ) === true;
-					expected = null;
-				} catch ( e ) {
-
-					// assign the "expected" to a nice error string to communicate the local failure to the user
-					expected = errorString( e );
-				}
-			}
+			[ result, expected, message ] = validateException( actual, expected, message );
 		}
 
 		currentTest.assert.pushResult( {
@@ -374,28 +336,10 @@ class Assert {
 	}
 
 	rejects( promise, expected, message ) {
-		let result = false;
-
-		const currentTest = ( this instanceof Assert && this.test ) || config.current;
 
-		// 'expected' is optional unless doing string comparison
-		if ( objectType( expected ) === "string" ) {
-			if ( message === undefined ) {
-				message = expected;
-				expected = undefined;
-			} else {
-				message = "assert.rejects does not accept a string value for the expected " +
-					"argument.\nUse a non-string object value (e.g. validator function) instead " +
-					"if necessary.";
+		[ expected, message ] = validateExpectedExceptionArgs( expected, message, "rejects" );
 
-				currentTest.assert.pushResult( {
-					result: false,
-					message: message
-				} );
-
-				return;
-			}
-		}
+		const currentTest = ( this instanceof Assert && this.test ) || config.current;
 
 		const then = promise && promise.then;
 		if ( objectType( then ) !== "function" ) {
@@ -429,47 +373,8 @@ class Assert {
 			},
 
 			function handleRejection( actual ) {
-				const expectedType = objectType( expected );
-
-				// We don't want to validate
-				if ( expected === undefined ) {
-					result = true;
-
-					// Expected is a regexp
-				} else if ( expectedType === "regexp" ) {
-					result = expected.test( errorString( actual ) );
-
-					// Log the string form of the regexp
-					expected = String( expected );
-
-					// Expected is a constructor, maybe an Error constructor
-				} else if ( expectedType === "function" && actual instanceof expected ) {
-					result = true;
-
-					// Expected is an Error object
-				} else if ( expectedType === "object" ) {
-					result = actual instanceof expected.constructor &&
-						actual.name === expected.name &&
-						actual.message === expected.message;
-
-					// Log the string form of the Error object
-					expected = errorString( expected );
-
-					// Expected is a validation function which returns true if validation passed
-				} else {
-					if ( expectedType === "function" ) {
-						result = expected.call( {}, actual ) === true;
-						expected = null;
-
-						// Expected is some other invalid type
-					} else {
-						result = false;
-						message = "invalid expected value provided to `assert.rejects` " +
-							"callback in \"" + currentTest.testName + "\": " +
-							expectedType + ".";
-					}
-				}
-
+				let result;
+				[ result, expected, message ] = validateException( actual, expected, message );
 
 				currentTest.assert.pushResult( {
 					result,
@@ -479,13 +384,97 @@ class Assert {
 					expected,
 					message
 				} );
-
 				done();
 			}
 		);
 	}
 }
 
+function validateExpectedExceptionArgs( expected, message, assertionMethod ) {
+	const expectedType = objectType( expected );
+
+	// 'expected' is optional unless doing string comparison
+	if ( expectedType === "string" ) {
+		if ( message === undefined ) {
+			message = expected;
+			expected = undefined;
+			return [ expected, message ];
+		} else {
+			throw new Error(
+				"assert." + assertionMethod +
+				" does not accept a string value for the expected argument.\n" +
+				"Use a non-string object value (e.g. RegExp or validator function) " +
+				"instead if necessary."
+			);
+		}
+	}
+
+	const valid =
+		!expected || // TODO: be more explicit here
+		expectedType === "regexp" ||
+		expectedType === "function" ||
+		expectedType === "object";
+
+	if ( !valid ) {
+		const message =
+			"Invalid expected value type (" + expectedType + ") " +
+			"provided to assert." + assertionMethod + ".";
+		throw new Error( message );
+	}
+
+	return [ expected, message ];
+}
+
+function validateException( actual, expected, message ) {
+	let result = false;
+	const expectedType = objectType( expected );
+
+	// These branches should be exhaustive, based on validation done in validateExpectedException
+
+	// We don't want to validate
+	if ( !expected ) {
+		result = true;
+
+	// Expected is a regexp
+	} else if ( expectedType === "regexp" ) {
+		result = expected.test( errorString( actual ) );
+
+		// Log the string form of the regexp
+		expected = String( expected );
+
+	// Expected is a constructor, maybe an Error constructor.
+	// Note the extra check on its prototype - this is an implicit
+	// requirement of "instanceof", else it will throw a TypeError.
+	} else if ( expectedType === "function" &&
+		expected.prototype !== undefined && actual instanceof expected ) {
+		result = true;
+
+	// Expected is an Error object
+	} else if ( expectedType === "object" ) {
+		result = actual instanceof expected.constructor &&
+			actual.name === expected.name &&
+			actual.message === expected.message;
+
+		// Log the string form of the Error object
+		expected = errorString( expected );
+
+	// Expected is a validation function which returns true if validation passed
+	} else if ( expectedType === "function" ) {
+
+		// protect against accidental semantics which could hard error in the test
+		try {
+			result = expected.call( {}, actual ) === true;
+			expected = null;
+		} catch ( e ) {
+
+			// assign the "expected" to a nice error string to communicate the local failure to the user
+			expected = errorString( e );
+		}
+	}
+
+	return [ result, expected, message ];
+}
+
 // Provide an alternative to assert.throws(), for environments that consider throws a reserved word
 // Known to us are: Closure Compiler, Narwhal
 // eslint-disable-next-line dot-notation

test/es2018/rejects.js

@@ -0,0 +1,79 @@
+QUnit.module( "assert" );
+
+function CustomError( message ) {
+	this.message = message;
+}
+
+CustomError.prototype.toString = function() {
+	return this.message;
+};
+
+QUnit.test( "rejects", function( assert ) {
+	assert.expect( 1 );
+
+	assert.rejects(
+		Promise.reject( new CustomError( "some error description" ) ),
+		err => {
+			return err instanceof CustomError && /description/.test( err );
+		},
+		"custom validation function"
+	);
+} );
+
+QUnit.test( "rejects with expected class", function( assert ) {
+	assert.expect( 1 );
+
+	class CustomError extends Error {}
+
+	assert.rejects(
+		Promise.reject( new CustomError( "foo" ) ),
+		CustomError,
+		"Expected value is a class extending Error"
+	);
+} );
+
+QUnit.module( "failing assertions", {
+	beforeEach: function( assert ) {
+		const originalPushResult = assert.pushResult;
+		assert.pushResult = function( resultInfo ) {
+
+			// Inverts the result so we can test failing assertions
+			resultInfo.result = !resultInfo.result;
+			originalPushResult( resultInfo );
+		};
+	}
+}, function() {
+	QUnit.test( "rejects", function( assert ) {
+		assert.rejects(
+			Promise.reject(),
+			() => false,
+			"rejects fails when expected function returns false"
+		);
+	} );
+
+	QUnit.module( "inspect expected values", {
+		beforeEach: function( assert ) {
+			const originalPushResult = assert.pushResult;
+			assert.pushResult = function( resultInfo ) {
+
+				// avoid circular asserts and use if/throw to verify
+				if ( resultInfo.expected !== "TypeError: Class constructor CustomError cannot be invoked without 'new'" ) {
+					throw new Error( "Unexpected value: " + resultInfo.expected );
+				}
+
+				// invoke the "outer" pushResult, which still inverts the result for negative testing
+				originalPushResult( resultInfo );
+			};
+		}
+	}, function() {
+		QUnit.test( "does not die when class is expected", function( assert ) {
+			class CustomError extends Error {}
+
+			assert.rejects(
+				Promise.reject( new Error( "foo" ) ),
+				CustomError,
+				"rejects fails gracefully when expected value class does not use 'new'"
+			);
+		} );
+	} );
+} );