Ensure the assertion message is a string

Fixes #1125.

Author: novemberborn

lib/assert.js

@@ -265,18 +265,46 @@ class Assertions {
 			patterns: [pattern]
 		});
 
+		const checkMessage = (assertion, message, powerAssert = false) => {
+			if (typeof message === 'undefined' || typeof message === 'string') {
+				return true;
+			}
+
+			const error = new AssertionError({
+				assertion,
+				improperUsage: true,
+				message: 'The assertion message must be a string',
+				values: [formatWithLabel('Called with:', message)]
+			});
+
+			if (powerAssert) {
+				throw error;
+			}
+
+			fail(error);
+			return false;
+		};
+
 		this.pass = withSkip(() => {
 			pass();
 		});
 
 		this.fail = withSkip(message => {
+			if (!checkMessage('fail', message)) {
+				return;
+			}
+
 			fail(new AssertionError({
 				assertion: 'fail',
 				message: message || 'Test failed via `t.fail()`'
 			}));
 		});
 
 		this.is = withSkip((actual, expected, message) => {
+			if (!checkMessage('is', message)) {
+				return;
+			}
+
 			if (Object.is(actual, expected)) {
 				pass();
 			} else {
@@ -303,6 +331,10 @@ class Assertions {
 		});
 
 		this.not = withSkip((actual, expected, message) => {
+			if (!checkMessage('not', message)) {
+				return;
+			}
+
 			if (Object.is(actual, expected)) {
 				fail(new AssertionError({
 					assertion: 'not',
@@ -316,6 +348,10 @@ class Assertions {
 		});
 
 		this.deepEqual = withSkip((actual, expected, message) => {
+			if (!checkMessage('deepEqual', message)) {
+				return;
+			}
+
 			const result = concordance.compare(actual, expected, concordanceOptions);
 			if (result.pass) {
 				pass();
@@ -332,6 +368,10 @@ class Assertions {
 		});
 
 		this.notDeepEqual = withSkip((actual, expected, message) => {
+			if (!checkMessage('notDeepEqual', message)) {
+				return;
+			}
+
 			const result = concordance.compare(actual, expected, concordanceOptions);
 			if (result.pass) {
 				const actualDescriptor = result.actual || concordance.describe(actual, concordanceOptions);
@@ -351,6 +391,11 @@ class Assertions {
 			// operator, so we can determine the total number of arguments passed
 			// to the function.
 			let [fn, expectations, message] = args;
+
+			if (!checkMessage('throws', message)) {
+				return;
+			}
+
 			if (typeof fn !== 'function') {
 				fail(new AssertionError({
 					assertion: 'throws',
@@ -412,6 +457,11 @@ class Assertions {
 
 		this.throwsAsync = withSkip((...args) => {
 			let [thrower, expectations, message] = args;
+
+			if (!checkMessage('throwsAsync', message)) {
+				return Promise.resolve();
+			}
+
 			if (typeof thrower !== 'function' && !isPromise(thrower)) {
 				fail(new AssertionError({
 					assertion: 'throwsAsync',
@@ -492,6 +542,10 @@ class Assertions {
 		});
 
 		this.notThrows = withSkip((fn, message) => {
+			if (!checkMessage('notThrows', message)) {
+				return;
+			}
+
 			if (typeof fn !== 'function') {
 				fail(new AssertionError({
 					assertion: 'notThrows',
@@ -518,6 +572,10 @@ class Assertions {
 		});
 
 		this.notThrowsAsync = withSkip((nonThrower, message) => {
+			if (!checkMessage('notThrowsAsync', message)) {
+				return Promise.resolve();
+			}
+
 			if (typeof nonThrower !== 'function' && !isPromise(nonThrower)) {
 				fail(new AssertionError({
 					assertion: 'notThrowsAsync',
@@ -574,20 +632,31 @@ class Assertions {
 			return handlePromise(retval, true);
 		});
 
-		this.snapshot = withSkip((expected, optionsOrMessage, message) => {
-			const options = {};
-			if (typeof optionsOrMessage === 'string') {
-				message = optionsOrMessage;
-			} else if (optionsOrMessage) {
-				options.id = optionsOrMessage.id;
+		this.snapshot = withSkip((expected, ...rest) => {
+			let message;
+			let snapshotOptions;
+			if (rest.length > 1) {
+				[snapshotOptions, message] = rest;
+			} else {
+				const [optionsOrMessage] = rest;
+				if (typeof optionsOrMessage === 'object') {
+					snapshotOptions = optionsOrMessage;
+				} else {
+					message = optionsOrMessage;
+				}
 			}
 
-			options.expected = expected;
-			options.message = message;
+			if (!checkMessage('snapshot', message)) {
+				return;
+			}
 
 			let result;
 			try {
-				result = compareWithSnapshot(options);
+				result = compareWithSnapshot({
+					expected,
+					id: snapshotOptions ? snapshotOptions.id : undefined,
+					message
+				});
 			} catch (error) {
 				if (!(error instanceof snapshotManager.SnapshotError)) {
 					throw error;
@@ -625,6 +694,10 @@ class Assertions {
 		});
 
 		this.truthy = withSkip((actual, message) => {
+			if (!checkMessage('truthy', message)) {
+				return;
+			}
+
 			if (actual) {
 				pass();
 			} else {
@@ -638,6 +711,10 @@ class Assertions {
 		});
 
 		this.falsy = withSkip((actual, message) => {
+			if (!checkMessage('falsy', message)) {
+				return;
+			}
+
 			if (actual) {
 				fail(new AssertionError({
 					assertion: 'falsy',
@@ -651,6 +728,10 @@ class Assertions {
 		});
 
 		this.true = withSkip((actual, message) => {
+			if (!checkMessage('true', message)) {
+				return;
+			}
+
 			if (actual === true) {
 				pass();
 			} else {
@@ -663,6 +744,10 @@ class Assertions {
 		});
 
 		this.false = withSkip((actual, message) => {
+			if (!checkMessage('false', message)) {
+				return;
+			}
+
 			if (actual === false) {
 				pass();
 			} else {
@@ -675,6 +760,10 @@ class Assertions {
 		});
 
 		this.regex = withSkip((string, regex, message) => {
+			if (!checkMessage('regex', message)) {
+				return;
+			}
+
 			if (typeof string !== 'string') {
 				fail(new AssertionError({
 					assertion: 'regex',
@@ -711,6 +800,10 @@ class Assertions {
 		});
 
 		this.notRegex = withSkip((string, regex, message) => {
+			if (!checkMessage('notRegex', message)) {
+				return;
+			}
+
 			if (typeof string !== 'string') {
 				fail(new AssertionError({
 					assertion: 'notRegex',
@@ -749,6 +842,8 @@ class Assertions {
 		this.assert = withSkip(withPowerAssert(
 			'assert(value, [message])',
 			(actual, message) => {
+				checkMessage('assert', message, true);
+
 				if (!actual) {
 					throw new AssertionError({
 						assertion: 'assert',

test/assert.js

@@ -201,6 +201,18 @@ test('.fail()', t => {
 		message: 'Test failed via `t.fail()`'
 	});
 
+	failsWith(t, () => {
+		assertions.fail(null);
+	}, {
+		assertion: 'fail',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });
 
@@ -383,6 +395,18 @@ test('.is()', t => {
 		]
 	});
 
+	failsWith(t, () => {
+		assertions.is(0, 0, null);
+	}, {
+		assertion: 'is',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });
 
@@ -421,6 +445,18 @@ test('.not()', t => {
 		values: [{label: 'Value is the same as:', formatted: /foo/}]
 	});
 
+	failsWith(t, () => {
+		assertions.not(0, 1, null);
+	}, {
+		assertion: 'not',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });
 
@@ -682,6 +718,18 @@ test('.deepEqual()', t => {
 		values: [{label: 'Difference:', formatted: /- 'foo'\n\+ 42/}]
 	});
 
+	failsWith(t, () => {
+		assertions.deepEqual({}, {}, null);
+	}, {
+		assertion: 'deepEqual',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });
 
@@ -720,6 +768,18 @@ test('.notDeepEqual()', t => {
 		values: [{label: 'Value is deeply equal:', formatted: /.*\[.*\n.*'a',\n.*'b',/}]
 	});
 
+	failsWith(t, () => {
+		assertions.notDeepEqual({}, [], null);
+	}, {
+		assertion: 'notDeepEqual',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });
 
@@ -921,6 +981,18 @@ test('.throws()', gather(t => {
 			throw new Error('foo');
 		}, undefined);
 	});
+
+	failsWith(t, () => {
+		assertions.throws(() => {}, null, null);
+	}, {
+		assertion: 'throws',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
 }));
 
 test('.throws() returns the thrown error', t => {
@@ -1003,6 +1075,16 @@ test('.throwsAsync()', gather(t => {
 			{label: 'Function returned:', formatted: /undefined/}
 		]
 	});
+
+	eventuallyFailsWith(t, () => assertions.throwsAsync(Promise.resolve(), null, null), {
+		assertion: 'throwsAsync',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
 }));
 
 test('.throwsAsync() returns the rejection reason of promise', t => {
@@ -1218,6 +1300,18 @@ test('.notThrows()', gather(t => {
 		message: 'my message',
 		values: [{label: 'Function threw:', formatted: /foo/}]
 	});
+
+	failsWith(t, () => {
+		assertions.notThrows(() => {}, null);
+	}, {
+		assertion: 'notThrows',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
 }));
 
 test('.notThrowsAsync()', gather(t => {
@@ -1265,6 +1359,16 @@ test('.notThrowsAsync()', gather(t => {
 			{label: 'Function did not return a promise. Use `t.notThrows()` instead:', formatted: /undefined/}
 		]
 	});
+
+	eventuallyFailsWith(t, () => assertions.notThrowsAsync(Promise.resolve(), null), {
+		assertion: 'notThrowsAsync',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
 }));
 
 test('.notThrowsAsync() returns undefined for a fulfilled promise', t => {
@@ -1443,6 +1547,21 @@ test('.snapshot()', t => {
 		}
 	}
 
+	{
+		const assertions = setup('bad message');
+		failsWith(t, () => {
+			assertions.snapshot(null, null, null);
+		}, {
+			assertion: 'snapshot',
+			improperUsage: true,
+			message: 'The assertion message must be a string',
+			values: [{
+				label: 'Called with:',
+				formatted: /null/
+			}]
+		});
+	}
+
 	manager.save();
 	t.end();
 });
@@ -1477,6 +1596,18 @@ test('.truthy()', t => {
 		truthy(true);
 	});
 
+	failsWith(t, () => {
+		assertions.truthy(true, null);
+	}, {
+		assertion: 'truthy',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });
 
@@ -1510,6 +1641,18 @@ test('.falsy()', t => {
 		falsy(false);
 	});
 
+	failsWith(t, () => {
+		assertions.falsy(false, null);
+	}, {
+		assertion: 'falsy',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });
 
@@ -1555,6 +1698,18 @@ test('.true()', t => {
 		trueFn(true);
 	});
 
+	failsWith(t, () => {
+		assertions.true(true, null);
+	}, {
+		assertion: 'true',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });
 
@@ -1600,6 +1755,18 @@ test('.false()', t => {
 		falseFn(false);
 	});
 
+	failsWith(t, () => {
+		assertions.false(false, null);
+	}, {
+		assertion: 'false',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });
 
@@ -1635,6 +1802,18 @@ test('.regex()', t => {
 		]
 	});
 
+	failsWith(t, () => {
+		assertions.regex('foo', /^abc$/, null);
+	}, {
+		assertion: 'regex',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });
 
@@ -1691,6 +1870,18 @@ test('.notRegex()', t => {
 		]
 	});
 
+	failsWith(t, () => {
+		assertions.notRegex('abc', /abc/, null);
+	}, {
+		assertion: 'notRegex',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });
 
@@ -1744,5 +1935,17 @@ test('.assert()', t => {
 		assert(true);
 	});
 
+	failsWith(t, () => {
+		assertions.assert(null, null);
+	}, {
+		assertion: 'assert',
+		improperUsage: true,
+		message: 'The assertion message must be a string',
+		values: [{
+			label: 'Called with:',
+			formatted: /null/
+		}]
+	});
+
 	t.end();
 });