From cf24bcf63f5f8418d86c38a2a59176b2b21896fb Mon Sep 17 00:00:00 2001 From: Mark Wubben Date: Mon, 3 Apr 2017 13:48:26 +0100 Subject: [PATCH 1/3] Reword error message for when assertions are used after test has finished --- lib/test.js | 6 +++--- test/test.js | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/test.js b/lib/test.js index 9d7f0623a..c33c2fa8e 100644 --- a/lib/test.js +++ b/lib/test.js @@ -154,7 +154,7 @@ class Test { countPassedAssertion() { if (this.finishing) { - this.saveFirstError(new Error('Assertion passed, but test has already ended')); + this.saveFirstError(new Error('Assertion passed, but test has already finished')); } this.assertCount++; @@ -162,7 +162,7 @@ class Test { addPendingAssertion(promise) { if (this.finishing) { - this.saveFirstError(new Error('Assertion passed, but test has already ended')); + this.saveFirstError(new Error('Assertion passed, but test has already finished')); } this.assertCount++; @@ -171,7 +171,7 @@ class Test { addFailedAssertion(error) { if (this.finishing) { - this.saveFirstError(new Error('Assertion failed, but test has already ended')); + this.saveFirstError(new Error('Assertion failed, but test has already finished')); } this.assertCount++; diff --git a/test/test.js b/test/test.js index e8111d6b6..43789ba11 100644 --- a/test/test.js +++ b/test/test.js @@ -591,7 +591,7 @@ test('number of assertions matches t.plan when the test exits, but before all pe result = r; }).run().then(passed => { t.is(passed, false); - t.match(result.reason.message, /Assertion passed, but test has already ended/); + t.match(result.reason.message, /Assertion passed, but test has already finished/); t.is(result.reason.name, 'Error'); t.end(); }); @@ -610,7 +610,7 @@ test('number of assertions matches t.plan when the test exits, but before all pe result = r; }).run().then(passed => { t.is(passed, false); - t.match(result.reason.message, /Assertion failed, but test has already ended/); + t.match(result.reason.message, /Assertion failed, but test has already finished/); t.is(result.reason.name, 'Error'); t.end(); }); From 15e9ae3052556cba4e6c84bea7c1dd3bdea03927 Mon Sep 17 00:00:00 2001 From: Mark Wubben Date: Mon, 3 Apr 2017 13:55:53 +0100 Subject: [PATCH 2/3] Remove unnecessary test to verify throws/notThrows return promises --- test/test.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/test/test.js b/test/test.js index 43789ba11..9eb8b7836 100644 --- a/test/test.js +++ b/test/test.js @@ -1,7 +1,6 @@ 'use strict'; const test = require('tap').test; const delay = require('delay'); -const isPromise = require('is-promise'); const formatValue = require('../lib/format-assert-error').formatValue; const Test = require('../lib/test'); @@ -635,17 +634,6 @@ test('number of assertions doesn\'t match plan when the test exits, but before a t.end(); }); -test('assertions return promises', t => { - ava(a => { - a.plan(2); - t.ok(isPromise(a.throws(Promise.reject(new Error('foo'))))); - t.ok(isPromise(a.notThrows(Promise.resolve(true)))); - }).run().then(passed => { - t.is(passed, true); - t.end(); - }); -}); - test('contextRef', t => { new Test({ contextRef: {context: {foo: 'bar'}}, From 66c664d11838dab3dc08d80d757e6d546d52e702 Mon Sep 17 00:00:00 2001 From: Mark Wubben Date: Mon, 3 Apr 2017 14:09:41 +0100 Subject: [PATCH 3/3] 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 (https://github.com/avajs/ava/issues/1327#issuecomment-291122432). Fixes #1327. --- lib/test.js | 36 +++++------ readme.md | 16 +++++ test/test.js | 174 +++++++++++++++++++-------------------------------- 3 files changed, 98 insertions(+), 128 deletions(-) diff --git a/lib/test.js b/lib/test.js index c33c2fa8e..a9b0fb1d9 100644 --- a/lib/test.js +++ b/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; diff --git a/readme.md b/readme.md index b45688265..f02b1c553 100644 --- a/readme.md +++ b/readme.md @@ -964,10 +964,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`. diff --git a/test/test.js b/test/test.js index 9eb8b7836..812b11feb 100644 --- a/test/test.js +++ b/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 => {