Fail tests that finish with pending assertions

`t.throws()` and `t.notThrows()` can be called with an observable or
promise. This commit forces users to wait for the assertion to complete
before finishing the test. Usually this means the test has to be written
like:

```js
test(async t => {
  await t.throws(Promise.reject(new Error()))
})
```

Or for callback tests:

```js
test.cb(t => {
  t.throws(Promise.reject(new Error())).then(t.end)
})
```

This simplifies internal logic and helps discourage code like in #1327.
Anecdotally users are surprised by the previous behavior where a
synchronous test worked with an asynchronous assertion
(#1327 (comment)).

Fixes #1327.

Author: novemberborn

lib/test.js

@@ -111,7 +111,7 @@ class Test {
 		this.finishDueToAttributedError = null;
 		this.finishDueToInactivity = null;
 		this.finishing = false;
-		this.pendingAssertions = [];
+		this.pendingAssertionCount = 0;
 		this.pendingThrowsAssertion = null;
 		this.planCount = null;
 		this.startedAt = 0;
@@ -166,7 +166,10 @@ class Test {
 		}
 
 		this.assertCount++;
-		this.pendingAssertions.push(promise.catch(err => this.saveFirstError(err)));
+		this.pendingAssertionCount++;
+		promise
+			.catch(err => this.saveFirstError(err))
+			.then(() => this.pendingAssertionCount--);
 	}
 
 	addFailedAssertion(error) {
@@ -208,8 +211,12 @@ class Test {
 	}
 
 	verifyAssertions() {
-		if (this.failWithoutAssertions && !this.assertError && !this.calledEnd && this.planCount === null && this.assertCount === 0) {
-			this.saveFirstError(new Error('Test finished without running any assertions'));
+		if (!this.assertError) {
+			if (this.failWithoutAssertions && !this.calledEnd && this.planCount === null && this.assertCount === 0) {
+				this.saveFirstError(new Error('Test finished without running any assertions'));
+			} else if (this.pendingAssertionCount > 0) {
+				this.saveFirstError(new Error('Test finished, but an assertion is still pending'));
+			}
 		}
 	}
 
@@ -375,21 +382,6 @@ class Test {
 		this.verifyPlan();
 		this.verifyAssertions();
 
-		if (this.assertError || this.pendingAssertions.length === 0) {
-			return this.completeFinish();
-		}
-
-		// Finish after potential errors from pending assertions have been consumed.
-		return Promise.all(this.pendingAssertions).then(() => this.completeFinish());
-	}
-
-	finishPromised() {
-		return new Promise(resolve => {
-			resolve(this.finish());
-		});
-	}
-
-	completeFinish() {
 		this.duration = globals.now() - this.startedAt;
 
 		let reason = this.assertError;
@@ -413,6 +405,12 @@ class Test {
 
 		return passed;
 	}
+
+	finishPromised() {
+		return new Promise(resolve => {
+			resolve(this.finish());
+		});
+	}
 }
 
 module.exports = Test;

readme.md

@@ -965,10 +965,26 @@ test('rejects', async t => {
 });
 ```
 
+When testing a promise you must wait for the assertion to complete:
+
+```js
+test('rejects', async t => {
+	await t.throws(promise);
+});
+```
+
 ### `.notThrows(function|promise, [message])`
 
 Assert that `function` does not throw an error or that `promise` does not reject with an error.
 
+Like the `.throws()` assertion, when testing a promise you must wait for the assertion to complete:
+
+```js
+test('rejects', async t => {
+	await t.notThrows(promise);
+});
+```
+
 ### `.regex(contents, regex, [message])`
 
 Assert that `contents` matches `regex`.

test/test.js

@@ -450,10 +450,12 @@ test('throws and notThrows work with promises', t => {
 	let result;
 	ava(a => {
 		a.plan(2);
-		a.throws(delay.reject(10, new Error('foo')), 'foo');
-		a.notThrows(delay(20).then(() => {
-			asyncCalled = true;
-		}));
+		return Promise.all([
+			a.throws(delay.reject(10, new Error('foo')), 'foo'),
+			a.notThrows(delay(20).then(() => {
+				asyncCalled = true;
+			}))
+		]);
 	}, null, r => {
 		result = r;
 	}).run().then(passed => {
@@ -495,143 +497,97 @@ test('cb test that throws sync', t => {
 	t.end();
 });
 
-test('waits for t.throws to resolve after t.end is called', t => {
+test('multiple resolving and rejecting promises passed to t.throws/t.notThrows', t => {
 	let result;
-	ava.cb(a => {
-		a.plan(1);
-		a.notThrows(delay(10), 'foo');
-		a.end();
+	ava(a => {
+		a.plan(6);
+		const promises = [];
+		for (let i = 0; i < 3; i++) {
+			promises.push(
+				a.throws(delay.reject(10, new Error('foo')), 'foo'),
+				a.notThrows(delay(10), 'foo')
+			);
+		}
+		return Promise.all(promises);
 	}, null, r => {
 		result = r;
 	}).run().then(passed => {
 		t.is(passed, true);
-		t.is(result.result.planCount, 1);
-		t.is(result.result.assertCount, 1);
+		t.is(result.result.planCount, 6);
+		t.is(result.result.assertCount, 6);
 		t.end();
 	});
 });
 
-test('waits for t.throws to reject after t.end is called', t => {
+test('fails if test ends while there are pending assertions', t => {
 	let result;
-	ava.cb(a => {
-		a.plan(1);
-		a.throws(delay.reject(10, new Error('foo')), 'foo');
-		a.end();
+	const passed = ava(a => {
+		a.throws(Promise.reject(new Error()));
 	}, null, r => {
 		result = r;
-	}).run().then(passed => {
-		t.is(passed, true);
-		t.is(result.result.planCount, 1);
-		t.is(result.result.assertCount, 1);
-		t.end();
-	});
-});
+	}).run();
 
-test('waits for t.throws to resolve after the promise returned from the test resolves', t => {
-	let result;
-	ava(a => {
-		a.plan(1);
-		a.notThrows(delay(10), 'foo');
-		return Promise.resolve();
-	}, null, r => {
-		result = r;
-	}).run().then(passed => {
-		t.is(passed, true);
-		t.is(result.result.planCount, 1);
-		t.is(result.result.assertCount, 1);
-		t.end();
-	});
+	t.is(passed, false);
+	t.is(result.reason.name, 'Error');
+	t.match(result.reason.message, /Test finished, but an assertion is still pending/);
+	t.end();
 });
 
-test('waits for t.throws to reject after the promise returned from the test resolves', t => {
+test('fails if callback test ends while there are pending assertions', t => {
 	let result;
-	ava(a => {
-		a.plan(1);
-		a.throws(delay.reject(10, new Error('foo')), 'foo');
-		return Promise.resolve();
+	const passed = ava.cb(a => {
+		a.throws(Promise.reject(new Error()));
+		a.end();
 	}, null, r => {
 		result = r;
-	}).run().then(passed => {
-		t.is(passed, true);
-		t.is(result.result.planCount, 1);
-		t.is(result.result.assertCount, 1);
-		t.end();
-	});
-});
+	}).run();
 
-test('multiple resolving and rejecting promises passed to t.throws/t.notThrows', t => {
-	let result;
-	ava(a => {
-		a.plan(6);
-		for (let i = 0; i < 3; i++) {
-			a.throws(delay.reject(10, new Error('foo')), 'foo');
-			a.notThrows(delay(10), 'foo');
-		}
-	}, null, r => {
-		result = r;
-	}).run().then(passed => {
-		t.is(passed, true);
-		t.is(result.result.planCount, 6);
-		t.is(result.result.assertCount, 6);
-		t.end();
-	});
+	t.is(passed, false);
+	t.is(result.reason.name, 'Error');
+	t.match(result.reason.message, /Test finished, but an assertion is still pending/);
+	t.end();
 });
 
-test('number of assertions matches t.plan when the test exits, but before all pending assertions resolve another is added', t => {
+test('fails if async test ends while there are pending assertions', t => {
 	let result;
 	ava(a => {
-		a.plan(2);
-		a.throws(delay.reject(10, new Error('foo')), 'foo');
-		a.notThrows(delay(10), 'foo');
-		setTimeout(() => {
-			a.pass();
-		}, 5);
+		a.throws(Promise.reject(new Error()));
+		return Promise.resolve();
 	}, null, r => {
 		result = r;
 	}).run().then(passed => {
 		t.is(passed, false);
-		t.match(result.reason.message, /Assertion passed, but test has already finished/);
 		t.is(result.reason.name, 'Error');
+		t.match(result.reason.message, /Test finished, but an assertion is still pending/);
 		t.end();
 	});
 });
 
-test('number of assertions matches t.plan when the test exits, but before all pending assertions resolve, a failing assertion is added', t => {
-	let result;
+// This behavior is incorrect, but feedback cannot be provided to the user due to
+// https://github.com/avajs/ava/issues/1330
+test('no crash when adding assertions after the test has ended', t => {
+	t.plan(3);
+
 	ava(a => {
-		a.plan(2);
-		a.throws(delay.reject(10, new Error('foo')), 'foo');
-		a.notThrows(delay(10), 'foo');
-		setTimeout(() => {
-			a.fail();
-		}, 5);
-	}, null, r => {
-		result = r;
-	}).run().then(passed => {
-		t.is(passed, false);
-		t.match(result.reason.message, /Assertion failed, but test has already finished/);
-		t.is(result.reason.name, 'Error');
-		t.end();
-	});
-});
+		a.pass();
+		setImmediate(() => {
+			t.doesNotThrow(() => a.pass());
+		});
+	}).run();
 
-test('number of assertions doesn\'t match plan when the test exits, but before all promises resolve another is added', t => {
-	let result;
-	const passed = ava(a => {
-		a.plan(3);
-		a.throws(delay.reject(10, new Error('foo')), 'foo');
-		a.notThrows(delay(10), 'foo');
-		setTimeout(() => {
-			a.throws(Promise.reject(new Error('foo')), 'foo');
-		}, 5);
-	}, null, r => {
-		result = r;
+	ava(a => {
+		a.pass();
+		setImmediate(() => {
+			t.doesNotThrow(() => a.fail());
+		});
 	}).run();
 
-	t.is(passed, false);
-	t.is(result.reason.assertion, 'plan');
-	t.is(result.reason.operator, '===');
-	t.end();
+	ava(a => {
+		a.pass();
+		setImmediate(() => {
+			t.doesNotThrow(() => a.notThrows(Promise.resolve()));
+		});
+	}).run();
 });
 
 test('contextRef', t => {
@@ -725,8 +681,8 @@ test('failing tests must not return a fulfilled promise', t => {
 test('failing tests pass when returning a rejected promise', t => {
 	ava.failing(a => {
 		a.plan(1);
-		a.notThrows(delay(10), 'foo');
-		return Promise.reject();
+		return a.notThrows(delay(10), 'foo')
+			.then(() => Promise.reject());
 	}).run().then(passed => {
 		t.is(passed, true);
 		t.end();
@@ -735,7 +691,7 @@ test('failing tests pass when returning a rejected promise', t => {
 
 test('failing tests pass with `t.throws(nonThrowingPromise)`', t => {
 	ava.failing(a => {
-		a.throws(Promise.resolve(10));
+		return a.throws(Promise.resolve(10));
 	}).run().then(passed => {
 		t.is(passed, true);
 		t.end();
@@ -745,7 +701,7 @@ test('failing tests pass with `t.throws(nonThrowingPromise)`', t => {
 test('failing tests fail with `t.notThrows(throws)`', t => {
 	let result;
 	ava.failing(a => {
-		a.notThrows(Promise.resolve('foo'));
+		return a.notThrows(Promise.resolve('foo'));
 	}, null, r => {
 		result = r;
 	}).run().then(passed => {