Improve metadata checks

lib/runner.js

@@ -5,6 +5,7 @@ var Promise = require('bluebird');
 var optionChain = require('option-chain');
 var matcher = require('matcher');
 var TestCollection = require('./test-collection');
+var validateTest = require('./validate-test');
 
 function noop() {}
 
@@ -115,18 +116,13 @@ Runner.prototype._addTest = function (title, opts, fn, args) {
 		opts.exclusive = title !== null && matcher([title], this._match).length === 1;
 	}
 
-	if (opts.todo) {
-		if (typeof fn === 'function') {
-			throw new TypeError('`todo` tests are not allowed to have an implementation. Use `test.skip()` for tests with an implementation.');
-		}
+	var validationError = validateTest(title, fn, opts);
+	if (validationError !== null) {
+		throw new Error(validationError);
+	}
 
+	if (opts.todo) {
 		fn = noop;
-
-		if (typeof title !== 'string') {
-			throw new TypeError('`todo` tests require a title');
-		}
-	} else if (typeof fn !== 'function') {
-		throw new TypeError('Expected an implementation. Use `test.todo()` for tests without an implementation.');
 	}
 
 	this.tests.add({

lib/test-collection.js

@@ -60,16 +60,8 @@ TestCollection.prototype.add = function (test) {
 		}
 	}
 
-	if (metadata.always && type !== 'after' && type !== 'afterEach') {
-		throw new Error('"always" can only be used with after and afterEach hooks');
-	}
-
 	// add a hook
 	if (type !== 'test') {
-		if (metadata.exclusive) {
-			throw new Error('"only" cannot be used with a ' + type + ' hook');
-		}
-
 		this.hooks[type + (metadata.always ? 'Always' : '')].push(test);
 		return;
 	}

lib/validate-test.js

@@ -0,0 +1,48 @@
+'use strict';
+
+function validate(title, fn, metadata) {
+	if (metadata.type !== 'test') {
+		if (metadata.exclusive) {
+			return '`only` is only for tests and cannot be used with hooks';
+		}
+
+		if (metadata.failing) {
+			return '`failing` is only for tests and cannot be used with hooks';
+		}
+
+		if (metadata.todo) {
+			return '`todo` is only for documentation of future tests and cannot be used with hooks';
+		}
+	}
+
+	if (metadata.todo) {
+		if (typeof fn === 'function') {
+			return '`todo` tests are not allowed to have an implementation. Use ' +
+			'`test.skip()` for tests with an implementation.';
+		}
+
+		if (typeof title !== 'string') {
+			return '`todo` tests require a title';
+		}
+
+		if (metadata.skipped || metadata.failing || metadata.exclusive) {
+			return '`todo` tests are just for documentation and cannot be used with skip, only, or failing';
+		}
+	} else if (typeof fn !== 'function') {
+		return 'Expected an implementation. Use `test.todo()` for tests without an implementation.';
+	}
+
+	if (metadata.always) {
+		if (!(metadata.type === 'after' || metadata.type === 'afterEach')) {
+			return '`always` can only be used with `after` and `afterEach`';
+		}
+	}
+
+	if (metadata.skipped && metadata.exclusive) {
+		return '`only` tests cannot be skipped';
+	}
+
+	return null;
+}
+
+module.exports = validate;

test/runner.js

@@ -319,6 +319,36 @@ test('only test', function (t) {
 	});
 });
 
+test('throws if you try to set a hook as exclusive', function (t) {
+	var runner = new Runner();
+
+	t.throws(function () {
+		runner.beforeEach.only('', noop);
+	}, {message: '`only` is only for tests and cannot be used with hooks'});
+
+	t.end();
+});
+
+test('throws if you try to set a before hook as always', function (t) {
+	var runner = new Runner();
+
+	t.throws(function () {
+		runner.before.always('', noop);
+	}, {message: '`always` can only be used with `after` and `afterEach`'});
+
+	t.end();
+});
+
+test('throws if you try to set a test as always', function (t) {
+	var runner = new Runner();
+
+	t.throws(function () {
+		runner.test.always('', noop);
+	}, {message: '`always` can only be used with `after` and `afterEach`'});
+
+	t.end();
+});
+
 test('runOnlyExclusive option test', function (t) {
 	t.plan(1);
 

test/test-collection.js

@@ -98,36 +98,6 @@ test('throws if no type is supplied', function (t) {
 	t.end();
 });
 
-test('throws if you try to set a hook as exclusive', function (t) {
-	var collection = new TestCollection();
-	t.throws(function () {
-		collection.add(mockTest({
-			type: 'beforeEach',
-			exclusive: true
-		}));
-	}, {message: '"only" cannot be used with a beforeEach hook'});
-	t.end();
-});
-
-test('throws if you try to set a before hook as always', function (t) {
-	var collection = new TestCollection();
-	t.throws(function () {
-		collection.add(mockTest({
-			type: 'before',
-			always: true
-		}));
-	}, {message: '"always" can only be used with after and afterEach hooks'});
-	t.end();
-});
-
-test('throws if you try to set a test as always', function (t) {
-	var collection = new TestCollection();
-	t.throws(function () {
-		collection.add(mockTest({always: true}));
-	}, {message: '"always" can only be used with after and afterEach hooks'});
-	t.end();
-});
-
 test('hasExclusive is set when an exclusive test is added', function (t) {
 	var collection = new TestCollection();
 	t.false(collection.hasExclusive);

test/validate-test.js

@@ -0,0 +1,106 @@
+'use strict';
+
+var test = require('tap').test;
+var validate = require('../lib/validate-test');
+
+var noop = function () {};
+
+test('validate accepts basic test', function (t) {
+	t.type(validate('basic test', noop, {type: 'test'}), 'null');
+	t.end();
+});
+
+test('validate rejects tests without implementations except `todo` tests', function (t) {
+	var errorMessage = 'Expected an implementation. Use `test.todo()` for tests without an implementation.';
+
+	t.deepEquals(validate('test', null, {type: 'test'}), errorMessage);
+	t.deepEquals(validate('before', null, {type: 'before'}), errorMessage);
+	t.deepEquals(validate('beforeEach', null, {type: 'beforeEach'}), errorMessage);
+	t.deepEquals(validate('after', null, {type: 'after'}), errorMessage);
+	t.deepEquals(validate('afterEach', null, {type: 'afterEach'}), errorMessage);
+	t.end();
+});
+
+test('validate accepts proper todo', function (t) {
+	t.type(validate('proper todo', null, {todo: true, type: 'test'}), 'null');
+	t.end();
+});
+
+test('validate rejects todo with function', function (t) {
+	var errorMessage = '`todo` tests are not allowed to have an implementation. Use ' +
+	'`test.skip()` for tests with an implementation.';
+
+	t.deepEquals(validate('todo with function', noop, {todo: true, type: 'test'}), errorMessage);
+	t.end();
+});
+
+test('validate rejects todo without title', function (t) {
+	var errorMessage = '`todo` tests require a title';
+
+	t.deepEquals(validate(null, null, {todo: true, type: 'test'}), errorMessage);
+	t.end();
+});
+
+test('validate rejects todo with failing, skipped, or exclusive', function (t) {
+	var errorMessage = '`todo` tests are just for documentation and cannot be used with skip, only, or failing';
+
+	t.deepEquals(validate('failing', null, {todo: true, failing: true, type: 'test'}), errorMessage);
+	t.deepEquals(validate('skipped', null, {todo: true, skipped: true, type: 'test'}), errorMessage);
+	t.deepEquals(validate('exclusive', null, {todo: true, exclusive: true, type: 'test'}), errorMessage);
+	t.end();
+});
+
+test('validate rejects todo when it\'s not a test', function (t) {
+	var errorMessage = '`todo` is only for documentation of future tests and cannot be used with hooks';
+
+	t.deepEquals(validate('before', null, {todo: true, type: 'before'}), errorMessage);
+	t.deepEquals(validate('beforeEach', null, {todo: true, type: 'beforeEach'}), errorMessage);
+	t.deepEquals(validate('after', null, {todo: true, type: 'after'}), errorMessage);
+	t.deepEquals(validate('afterEach', null, {todo: true, type: 'afterEach'}), errorMessage);
+	t.end();
+});
+
+test('validate rejects skipped exclusive', function (t) {
+	var errorMessage = '`only` tests cannot be skipped';
+
+	t.deepEquals(validate('skipped exclusive', noop, {exclusive: true, skipped: true, type: 'test'}), errorMessage);
+	t.end();
+});
+
+test('validate rejects failing on non-tests', function (t) {
+	var errorMessage = '`failing` is only for tests and cannot be used with hooks';
+
+	t.type(validate('before', noop, {failing: true, type: 'test'}), 'null');
+	t.deepEquals(validate('before', noop, {failing: true, type: 'before'}), errorMessage);
+	t.deepEquals(validate('beforeEach', noop, {failing: true, type: 'beforeEach'}), errorMessage);
+	t.deepEquals(validate('after', noop, {failing: true, type: 'after'}), errorMessage);
+	t.deepEquals(validate('afterEach', noop, {failing: true, type: 'afterEach'}), errorMessage);
+	t.end();
+});
+
+test('validate rejects skip on non-tests', function (t) {
+	var errorMessage = '`only` is only for tests and cannot be used with hooks';
+
+	t.type(validate('before', noop, {exclusive: true, type: 'test'}), 'null');
+	t.deepEquals(validate('before', noop, {exclusive: true, type: 'before'}), errorMessage);
+	t.deepEquals(validate('beforeEach', noop, {exclusive: true, type: 'beforeEach'}), errorMessage);
+	t.deepEquals(validate('after', noop, {exclusive: true, type: 'after'}), errorMessage);
+	t.deepEquals(validate('afterEach', noop, {exclusive: true, type: 'afterEach'}), errorMessage);
+	t.end();
+});
+
+test('validate only allows always on `after` and `afterEach`', function (t) {
+	var errorMessage = '`always` can only be used with `after` and `afterEach`';
+
+	t.deepEquals(validate('before', noop, {always: true, type: 'test'}), errorMessage);
+	t.deepEquals(validate('before', noop, {always: true, type: 'before'}), errorMessage);
+	t.deepEquals(validate('beforeEach', noop, {always: true, type: 'beforeEach'}), errorMessage);
+	t.type(validate('after', noop, {always: true, type: 'after'}), 'null');
+	t.type(validate('afterEach', noop, {always: true, type: 'afterEach'}), 'null');
+	t.end();
+});
+
+test('validate accepts skipping failing tests', function (t) {
+	t.deepEquals(validate('before', noop, {failing: true, skipped: true, type: 'test'}), null);
+	t.end();
+});