Skip to content

Commit

Permalink
Fail tests that finish with pending assertions
Browse files Browse the repository at this point in the history 
`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.
  • Loading branch information
@novemberborn
novemberborn committed Apr 3, 2017
1 parent 15e9ae3 commit 66c664d
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 128 deletions.
36 changes: 17 additions & 19 deletions lib/test.js
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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'));
}
}
}

Expand Down Expand Up @@ -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;
Expand All @@ -413,6 +405,12 @@ class Test {

return passed;
}

finishPromised() {
return new Promise(resolve => {
resolve(this.finish());
});
}
}

module.exports = Test;
16 changes: 16 additions & 0 deletions readme.md
View file
Expand Up @@ -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`.
Expand Down
174 changes: 65 additions & 109 deletions test/test.js
View file
Expand Up @@ -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 => {
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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 => {
Expand Down

0 comments on commit 66c664d

Please sign in to comment.