Restrict test modifier chaining

Explicitly specify allowable chains, with some ground rules in mind:

Test chaining rules:

* `serial` must come at the start
* `only` and `skip` must come at the end
* `failing` must come at the end, but can be followed by `only` and `skip`
* `only` and `skip` cannot be chained together
* no repeating

Hook chaining rules:

* `always` comes immediately after "after hooks"
* `skip` must come at the end
* no `only`
* no repeating

Additionally:

* `todo` cannot be chained, except after `serial`
* hooks are not available on `serial`

This commit also removes now unnecessary assertions from TestCollection.

Fixes #1182.

Author: novemberborn

docs/recipes/typescript.md

@@ -89,7 +89,7 @@ test.beforeEach(t => {
 	t.context.foo = 123; // error:  Type '123' is not assignable to type 'string'
 });
 
-test.after.always.failing.cb.serial('very long chains are properly typed', t => {
+test.serial.failing.cb('very long chains are properly typed', t => {
 	t.context.fooo = 'a value'; // error: Property 'fooo' does not exist on type '{ foo: string }'
 });
 

lib/main.js

@@ -95,9 +95,21 @@ globals.setImmediate(() => {
 	});
 });
 
-module.exports = runner.chain;
-
-// TypeScript imports the `default` property for
-// an ES2015 default import (`import test from 'ava'`)
-// See: https://github.com/Microsoft/TypeScript/issues/2242#issuecomment-83694181
-module.exports.default = runner.chain;
+const makeCjsExport = () => {
+	function test() {
+		return runner.chain.apply(null, arguments);
+	}
+	return Object.assign(test, runner.chain);
+};
+
+// Support CommonJS modules by exporting a test function that can be fully
+// chained. Also support ES module loaders by exporting __esModule and a
+// default. Support `import * as ava from 'ava'` use cases by exporting a
+// `test` member. Do all this whilst preventing `test.test.test() or
+// `test.default.test()` chains, though in CommonJS `test.test()` is
+// unavoidable.
+module.exports = Object.assign(makeCjsExport(), {
+	__esModule: true,
+	default: runner.chain,
+	test: runner.chain
+});

lib/runner.js

@@ -2,38 +2,112 @@
 const EventEmitter = require('events');
 const path = require('path');
 const Bluebird = require('bluebird');
-const optionChain = require('option-chain');
 const matcher = require('matcher');
 const snapshotManager = require('./snapshot-manager');
 const TestCollection = require('./test-collection');
 const validateTest = require('./validate-test');
 
-const chainableMethods = {
-	defaults: {
-		type: 'test',
-		serial: false,
-		exclusive: false,
-		skipped: false,
-		todo: false,
-		failing: false,
-		callback: false,
-		always: false
-	},
-	chainableMethods: {
-		test: {},
-		serial: {serial: true},
-		before: {type: 'before'},
-		after: {type: 'after'},
-		skip: {skipped: true},
-		todo: {todo: true},
-		failing: {failing: true},
-		only: {exclusive: true},
-		beforeEach: {type: 'beforeEach'},
-		afterEach: {type: 'afterEach'},
-		cb: {callback: true},
-		always: {always: true}
+const chainRegistry = new WeakMap();
+
+function startChain(name, call, defaults) {
+	const fn = function () {
+		call(Object.assign({}, defaults), Array.from(arguments));
+	};
+	Object.defineProperty(fn, 'name', {value: name});
+	chainRegistry.set(fn, {call, defaults, fullName: name});
+	return fn;
+}
+
+function extendChain(prev, name, flag) {
+	if (!flag) {
+		flag = name;
 	}
-};
+
+	const fn = function () {
+		callWithFlag(prev, flag, Array.from(arguments));
+	};
+	const fullName = `${chainRegistry.get(prev).fullName}.${name}`;
+	Object.defineProperty(fn, 'name', {value: fullName});
+	prev[name] = fn;
+
+	chainRegistry.set(fn, {flag, fullName, prev});
+	return fn;
+}
+
+function callWithFlag(prev, flag, args) {
+	const combinedFlags = {[flag]: true};
+	do {
+		const step = chainRegistry.get(prev);
+		if (step.call) {
+			step.call(Object.assign({}, step.defaults, combinedFlags), args);
+			prev = null;
+		} else {
+			combinedFlags[step.flag] = true;
+			prev = step.prev;
+		}
+	} while (prev);
+}
+
+function createHookChain(hook, isAfterHook) {
+	// Hook chaining rules:
+	// * `always` comes immediately after "after hooks"
+	// * `skip` must come at the end
+	// * no `only`
+	// * no repeating
+	extendChain(hook, 'cb', 'callback');
+	extendChain(hook, 'skip', 'skipped');
+	extendChain(hook.cb, 'skip', 'skipped');
+	if (isAfterHook) {
+		extendChain(hook, 'always');
+		extendChain(hook.always, 'cb', 'callback');
+		extendChain(hook.always, 'skip', 'skipped');
+		extendChain(hook.always.cb, 'skip', 'skipped');
+	}
+	return hook;
+}
+
+function createChain(fn, defaults) {
+	// Test chaining rules:
+	// * `serial` must come at the start
+	// * `only` and `skip` must come at the end
+	// * `failing` must come at the end, but can be followed by `only` and `skip`
+	// * `only` and `skip` cannot be chained together
+	// * no repeating
+	const root = startChain('test', fn, Object.assign({}, defaults, {type: 'test'}));
+	extendChain(root, 'cb', 'callback');
+	extendChain(root, 'failing');
+	extendChain(root, 'only', 'exclusive');
+	extendChain(root, 'serial');
+	extendChain(root, 'skip', 'skipped');
+	extendChain(root.cb, 'failing');
+	extendChain(root.cb, 'only', 'exclusive');
+	extendChain(root.cb, 'skip', 'skipped');
+	extendChain(root.cb.failing, 'only', 'exclusive');
+	extendChain(root.cb.failing, 'skip', 'skipped');
+	extendChain(root.failing, 'only', 'exclusive');
+	extendChain(root.failing, 'skip', 'skipped');
+	extendChain(root.serial, 'cb', 'callback');
+	extendChain(root.serial, 'failing');
+	extendChain(root.serial, 'only', 'exclusive');
+	extendChain(root.serial, 'skip', 'skipped');
+	extendChain(root.serial.cb, 'failing');
+	extendChain(root.serial.cb, 'only', 'exclusive');
+	extendChain(root.serial.cb, 'skip', 'skipped');
+	extendChain(root.serial.cb.failing, 'only', 'exclusive');
+	extendChain(root.serial.cb.failing, 'skip', 'skipped');
+
+	root.after = createHookChain(startChain('test.after', fn, Object.assign({}, defaults, {type: 'after'})), true);
+	root.afterEach = createHookChain(startChain('test.afterEach', fn, Object.assign({}, defaults, {type: 'afterEach'})), true);
+	root.before = createHookChain(startChain('test.before', fn, Object.assign({}, defaults, {type: 'before'})), false);
+	root.beforeEach = createHookChain(startChain('test.beforeEach', fn, Object.assign({}, defaults, {type: 'beforeEach'})), false);
+
+	// Todo tests cannot be chained. Allow todo tests to be flagged as needing to
+	// be serial.
+	root.todo = startChain('test.todo', fn, Object.assign({}, defaults, {type: 'test', todo: true}));
+	root.serial.todo = startChain('test.serial.todo', fn, Object.assign({}, defaults, {serial: true, type: 'test', todo: true}));
+
+	return root;
+}
 
 function wrapFunction(fn, args) {
 	return function (t) {
@@ -63,7 +137,7 @@ class Runner extends EventEmitter {
 			compareTestSnapshot: this.compareTestSnapshot.bind(this)
 		});
 
-		this.chain = optionChain(chainableMethods, (opts, args) => {
+		this.chain = createChain((opts, args) => {
 			let title;
 			let fn;
 			let macroArgIndex;
@@ -100,6 +174,14 @@ class Runner extends EventEmitter {
 			} else {
 				this.addTest(title, opts, fn, args);
 			}
+		}, {
+			serial: false,
+			exclusive: false,
+			skipped: false,
+			todo: false,
+			failing: false,
+			callback: false,
+			always: false
 		});
 	}
 

lib/test-collection.js

@@ -38,10 +38,6 @@ class TestCollection extends EventEmitter {
 		const metadata = test.metadata;
 		const type = metadata.type;
 
-		if (!type) {
-			throw new Error('Test type must be specified');
-		}
-
 		if (test.title === '' || typeof test.title !== 'string') {
 			if (type === 'test') {
 				throw new TypeError('Tests must have a title');
@@ -58,16 +54,8 @@ class TestCollection extends EventEmitter {
 			}
 		}
 
-		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;
 		}

package-lock.json

@@ -124,7 +124,6 @@
 		"ms": "^2.1.1",
 		"multimatch": "^2.1.0",
 		"observable-to-promise": "^0.5.0",
-		"option-chain": "^1.0.0",
 		"package-hash": "^2.0.0",
 		"pkg-conf": "^2.1.0",
 		"plur": "^2.0.0",

package.json

@@ -385,6 +385,8 @@ test.serial('passes serially', t => {
 
 Note that this only applies to tests within a particular test file. AVA will still run multiple tests files at the same time unless you pass the [`--serial` CLI flag](#cli).
 
+You can use the `.serial` modifier with all tests, hooks and even `.todo()`, but it's only available on the `test` function.
+
 ### Running specific tests
 
 During development it can be helpful to only run a few specific tests. This can be accomplished using the `.only` modifier:
@@ -399,6 +401,8 @@ test.only('will be run', t => {
 });
 ```
 
+You can use the `.only` modifier with all tests. It cannot be used with hooks or `.todo()`.
+
 *Note:* The `.only` modifier applies to the test file it's defined in, so if you run multiple test files, tests in other files will still run. If you want to only run the `test.only` test, provide just that test file to AVA.
 
 ### Running tests with matching titles
@@ -485,7 +489,7 @@ test.skip('will not be run', t => {
 });
 ```
 
-You must specify the implementation function.
+You must specify the implementation function. You can use the `.skip` modifier with all tests and hooks, but not with `.todo()`. You can not apply further modifiers to `.skip`.
 
 ### Test placeholders ("todo")
 
@@ -495,6 +499,12 @@ You can use the `.todo` modifier when you're planning to write a test. Like skip
 test.todo('will think about writing this later');
 ```
 
+You can signal that you need to write a serial test:
+
+```js
+test.serial.todo('will think about writing this later');
+```
+
 ### Failing tests
 
 You can use the `.failing` modifier to document issues with your code that need to be fixed. Failing tests are run just like normal ones, but they are expected to fail, and will not break your build when they do. If a test marked as failing actually passes, it will be reported as an error and fail the build with a helpful message instructing you to remove the `.failing` modifier.
@@ -558,7 +568,7 @@ test('title', t => {
 });
 ```
 
-Hooks can be synchronous or asynchronous, just like tests. To make a hook asynchronous return a promise or observable, use an async function, or enable callback mode via `test.cb.before()`, `test.cb.beforeEach()` etc.
+Hooks can be synchronous or asynchronous, just like tests. To make a hook asynchronous return a promise or observable, use an async function, or enable callback mode via `test.before.cb()`, `test.beforeEach.cb()` etc.
 
 ```js
 test.before(async t => {
@@ -569,7 +579,7 @@ test.after(t => {
 	return new Promise(/* ... */);
 });
 
-test.cb.beforeEach(t => {
+test.beforeEach.cb(t => {
 	setTimeout(t.end);
 });
 
@@ -610,19 +620,6 @@ test('context is unicorn', t => {
 
 Context sharing is *not* available to `before` and `after` hooks.
 
-### Chaining test modifiers
-
-You can use the `.serial`, `.only` and `.skip` modifiers in any order, with `test`, `before`, `after`, `beforeEach` and `afterEach`. For example:
-
-```js
-test.before.skip(...);
-test.skip.after(...);
-test.serial.only(...);
-test.only.serial(...);
-```
-
-This means you can temporarily add `.skip` or `.only` at the end of a test or hook definition without having to make any other changes.
-
 ### Test macros
 
 Additional arguments passed to the test declaration will be passed to the test implementation. This is useful for creating reusable test macros.