Fail tests that finish with pending assertions

[novemberborn]

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:

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

Or for callback tests:

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.

[sindresorhus]

Can we detect Promise/Observable being passed to t.throws/t.notThrows when using sync test test(t => {}); and disallow it?

[novemberborn]

No, since we don't know a test is synchronous until the function returns.

[sindresorhus]

But can't we throw when the test returns then? We could have t.throws add a flag to the test when used with a promise, then right after the test implementation is run, in the same tick, check the flag. If a test finishes in the same tick and doesn't return a promise, it's synchronous. This is not essential for this PR though. I just assume it will be a common issue.

[sindresorhus]

This is actually already caught by another check (nice!), but would be useful to have a better message for it.

import test from 'ava';

const foo = () => Promise.resolve('foo');

test(t => {
	t.throws(foo());
});

  1 failed

  [anonymous]

  Test finished, but an assertion is still pending

[sindresorhus]

Yay. I'm very happy about this change.

[novemberborn]

This is actually already caught by another check (nice!), but would be useful to have a better message for it.

Right. throws and notThrows are also the only assertions that can be pending. Are you saying we should track which assertion was used and base the error message on that?

[sindresorhus]

@novemberborn We could if it's not too hard. I would also be good with just failing the test saying something like:

We detected t.throws or t.notThrows being used with a promise/observable, but the test is synchronous. Either use an async function or return a promise.

That's at least clearer than:

Test finished, but an assertion is still pending

[novemberborn]

It's not too hard.

We detected t.throws or t.notThrows being used with a promise/observable, but the test is synchronous. Either use an async function or return a promise.

Unfortunately even if a test is asynchronous, that doesn't mean it's waiting for the assertion to complete.

[joflashstudios]

This change broke our test helpers - we were using t.notThrows to declare mocked functions that must be called. Now, instead of getting a specific error message, we get a generic "Test finished, but an assertion is still pending" instead of the error message passed into t.notThrows(promise, ), or the rejection itself.

[novemberborn]

@joflashstudios for a generic solution you could await all t.notThrows() calls.

[joflashstudios]

@novemberborn The problem is that the t.notThrows() calls need to be non-blocking, because they happen before the promise gets resolved.

[novemberborn]

You could keep a reference to the returned promises, and then at the end of the test implementation do return Promise.all(arrayOfReferences). You could even wrap that up in a function so you don't need to change your test code, like:

test('something', interceptNotThrows(t => {
  t.notThrows(somethingAsync())
}))