[jest-circus] Support both done callback and async test simultaneously #10529
Description
🚀 Feature Proposal
Please support writing async tests that can also use the done callback.
This didn't work "properly" before the new jest-circus runner forbid this combination, but it did sorta work enough to be useful. #9129
Ideally, an async (done) => {} test would not complete successfully unless both:
- the
donecallback was called without error AND - the Promise returned by the test resolved
Test would fail immediately if one of:
- the
donecallback is invoked with a truthy (error) value OR - the returned
Promiserejects OR - test timeout reached
When to proceed to after hooks and next test is debatable:
- If
doneerrors, should it wait for the promise to settle or timeout occurs? - If the promise rejects before
doneis called, should it wait fordoneto be called or timeout occurs? - Both of the above?
IMO it should always wait for promise to settle/timeout, but not wait for done if the promise rejects.
Motivation
Currently:
- Testing Promises 👍
- Testing Events/callbacks 👍
- Testing a mix of promises and events/callbacks: 👎
With the done OR Promise setup, how do you set up a test that checks both that a promise resolved AND an event was fired? Seems like this should be simple, but is it?
Consider a connection object , something like:
class Connection extends EventEmitter {
isConnected() {
return !!this._isConnected
}
async connect() {
return new Promise((resolve, reject) => {
// … implementation details go here
// imagine that it also rejects + emits 'error' on an error
setTimeout(() => {
this._isConnected = true
this.emit('connected') // or maybe after the resolve?
resolve()
}, 100)
})
}
}How to test the behaviour of this?
Here's some examples of partial/naive solutions to testing this that I have seen in the wild (or done myself).
For example, this doesn't verify that the promise resolved, or didn't reject before moving onto the next test:
test('this will not check both event + promise v1', (done) => {
connection.once('error', done) // auto-fail test if an error is emitted
connection.once('connected', () => {
expect(connection.isConnected()).toBeTruthy()
done() // but we haven't checked if connect call resolved/rejected
})
connection.connect().catch(done)
})And conversely using an async test, we can't check that the event was fired before the test ended:
test('this will not check both event + promise v2', async () => {
connection.once('error', done) // auto-fail test if an error is emitted
connection.once('connected', () => {
expect(connection.isConnected()).toBeTruthy() // if connected event fires after resolution then this won't be checked
})
await connection.connect()
})One way to set it up that will work, and handle all cases, is to queue the promise and then resolve the promise with done:
test('this will check both event + promise v1', (done) => {
connection.once('error', done) // auto-fail test if an error is emitted
let task
connection.once('connected', () => {
expect(connection.isConnected()).toBeTruthy()
task.then(() => done(), done)
})
task = connection.connect()
})But IIUC with the new jest-circus runner (at least in version 26.4.2), if that expect call fails, then test will wait to time out before moving on to the next test because done is never called because expect threw. This isn't ideal if we want fast tests. So I guess we have to wrap every event handler in a try/catch?
test('this will check both event + promise v1', (done) => {
connection.once('error', done) // auto-fail test if an error is emitted
let task
connection.once('connected', () => {
try {
expect(connection.isConnected()).toBeTruthy()
} catch (err) {
done(err)
return // does return here also make a task rejection trigger an unhandled rejection?
}
task.then(() => done(), done)
})
task = connection.connect()
})Perhaps that's off-topic. update: I've opened a ticket about uncaught exception waiting for timeout here #10530
The done callback style test doesn't give us the convenience of await though.
To set it up with an async test you have to do something like the code below:
test('this will check both event + promise v2', async () => { // auto-fail test if an error is emitted
const task = new Promise((resolve, reject) => {
connection.once('connected', resolve).once('error', reject)
}).then(() => {
expect(connection.isConnected()).toBeTruthy() // won't be executed synchronously with the 'connected' event now
})
await connection.connect()
expect(connection.isConnected()).toBeTruthy()
await task
})However the above does change the task timing of when the 'connected' event's expect call is run, it no longer runs in the same microtask as the event, so to restore this timing you have to do something like:
test('this will check both event + promise v3', async () => {
const task = new Promise((resolve, reject) => {
connection.once('connected', () => {
expect(connection.isConnected()).toBeTruthy()
resolve()
}).once('error', reject)
})
await connection.connect()
expect(connection.isConnected()).toBeTruthy()
await task
})However on failure, this will never call the reject/resolve so the test will also time out. Perhaps we wrap the expect in a try/catch?
test('this will check both event + promise v4', async () => {
const task = new Promise((resolve, reject) => {
connection.once('connected', () => {
try {
expect(connection.isConnected()).toBeTruthy()
resolve()
} catch (err) {
reject(err)
}
}).once('error', reject)
})
await connection.connect()
expect(connection.isConnected()).toBeTruthy()
await task
})Overall this is all a lot of thinking and messing around in order to get robust tests for something that could be simple.
Example
Ideally the Promise + done test would work something like so:
test('desired workflow', async (done) => {
connection.once('error', done) // auto-fail test if an error is emitted
connection.once('connected', () => {
expect(connection.isConnected()).toBeTruthy()
done() // this must be called before timeout
})
await connection.connect() // this must also resolve
expect(connection.isConnected()).toBeTruthy()
})Test would pass once both done and the returned promise resolve, one of them rejects, or the test times out.
And the test runner would do something like this (pseudocode):
let done = () => {} // noop for good measure
const onDone = new Promise((resolve, reject) => {
if (!testFn.length) {
resolve() // just resolve if test takes no `done` argument
return // optional I guess
}
done = (err) => err ? reject(err) : resolve()
})
await Promise.race([
// immediately rejects if either testFn promise rejects, or done passes an error
// otherwise waits for both to resolve before proceeding
Promise.all([
Promise.resolve().then(() => ( // wrap to reject on sync exceptions
testFn(done) // testFn is the test to be executed
),
onDone, // resolves when done is called (if testFn takes a callback)
]),
rejectOnUnhandledOrUncaught(), // convert global unhandled/uncaught event to rejection
getTimeout(testFn), // rejects on test timeout
])Could also consider using Promise.allSettled instead of Promise.all in order to wait for both done + resolve/reject before continuing, but that would forces the test to hit timeout in a case like expect throwing inside an event handler leading to done not being called? Or perhaps, when an unhandled exception/rejection fires, this implicitly calls done, so it doesn't have to wait?
Another option would be to use expect.assertions(n) to signal the test's end, but this is a PITA if you have expect assertions in before/after hooks, as these are included in the overall count for every test affected by those hooks.
edit: Perhaps the correct answer in this case is to simply test events and promises separately, which requires no changes and is arguably cleaner?
test('event is fired', (done) => {
connection.once('error', done)
connection.once('connected', () => {
expect(connection.isConnected()).toBeTruthy() // this failing shouldn't force test to wait for timeout though
done() // this must be called before timeout
})
connection.connect() // ignore resolve, reject will be picked up as unhandled
})
test('promise is resolved', async () => {
await connection.connect()
expect(connection.isConnected()).toBeTruthy()
})
Activity