[jest-circus] Support both done callback and async test simultaneously

[timoxley]

🚀 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. https://github.com/facebook/jest/pull/9129

Ideally, an async (done) => {} test would not complete successfully unless both:

• the done callback was called without error AND
• the Promise returned by the test resolved

Test would fail immediately if one of:

• the done callback is invoked with a truthy (error) value OR
• the returned Promise rejects OR
• test timeout reached

When to proceed to after hooks and next test is debatable:

• If done errors, should it wait for the promise to settle or timeout occurs?
• If the promise rejects before done is called, should it wait for done to 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 https://github.com/facebook/jest/issues/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()
})

[ahnpnl]

+1, this prevents jest-preset-angular to use jest-circus

[SimenB]

I've been snoozing this issue for weeks meaning to get back to it 😛 The OP was very detailed and well thought out, so I wanted to let it simmer for a bit.

---

I don't wanna rollback the change as I think it's a good one - tests are way easier to reason about when there's not multiple async paradigms in use at the same time. We've also been warning for at least a full major version.

To concretely come up with a working test for the example in the OP I'd do something like this

import { once } from 'events';

test('intended workflow', async () => {
  const connectedPromise = once(connection, 'connected');

  await Promise.all([connection.connect(), connectedPromise]);

  expect(connection.isConnected()).toBeTruthy();
});

once comes from node core and handles the error event by rejecting: https://nodejs.org/api/events.html#events_events_once_emitter_name_options. You can probably build that helper yourself if you don't wanna rely on node core modules - it's not too complex (at least if you ignore abort signals etc.).

Would be even better if events were guaranteed to be emitted on next tick, but that's not the way node event emitters work, thus the connectedPromise assignment. If the connected is guaranteed to be emitted asynchronously it's even shorter

import { once } from 'events';

test('intended workflow', async () => {
  await Promise.all([connection.connect(), once(connection, 'connected')]);

  expect(connection.isConnected()).toBeTruthy();
});

All that to say is that I think the change is a good one - APIs mixing callbacks and promises are weird and hard to reason about, and coercing them to follow a single paradigm (by wrapping the APIs or using helpers like I've done in this post) makes the code cleaner and easier to reason about.

---

/cc @jeysal @thymikee @cpojer would love your thoughts on this

[timoxley]

@SimenB

The once API with Promise.all is an ok solution, however I see that's still not perfect since a late rejection will potentially infect other tests with unhandled rejections from the previous test. Really should use Promise.allSettled then some helper to lift status: "rejected" entries into a rejection, e.g. using AggregateError.

But yeah, I reiterate:

Overall this is all a lot of thinking and messing around in order to get robust tests.

Not even really Jest's fault either, JS just doesn't provide a lot of utility for writing robust, non-trivial async workflows. But it'd be good if Jest could help pave a cowpath so it was easier to write clean, robust, async tests.

tests are way easier to reason about when there's not multiple async paradigms in use at the same time

Agreed but unfortunately fairly common to have multiple paradigms in play and may be unavoidable in many codebases. Ideally it wouldn't be so perilous to do so.

[fivethreeo]

Any progress on this? I reaaly wanted to switch to jest-circus for razzle but this is a blocker for that.

[enisdenjo]

Definitely e deal breaker for transitioning from the jest-jasmine2 to jest-circus runner...

[akauppi]

I’m surprised this is being called for. For me, the jest-circus approach of either using done or await was a welcome one, and the few places where I needed to wrap callbacks were nothing to write about.

Writing this not to raise arguments, but to support @SimenB ’s write from Dec 2020.

[BeauBouchard]

+1 can't wait till this feature is restored

[masad-frost]

None of these forms is particularly superior to the others, and you can mix and match them across a codebase or even in a single file. It just depends on which style you feel makes your tests simpler.

From https://jestjs.io/docs/asynchronous should be updated to point this out, while it doesn't say you can mix and match within a single test, it should clearly state that these 2 methods of testing are incompatible.

[masad-frost]

Here's a workaround for now. I don't know if there's any gotcha's, I assume there are some, if it was that simple jest would've already supported it.

  function itAsyncDone(
    name: string,
    cb: (done: jest.DoneCallback) => Promise<void>,
    timeout?: number,
  ) {
    it(
      name,
      (done) => {
        let doneCalled = false;
        const wrappedDone: jest.DoneCallback = (...args) => {
          if (doneCalled) {
            return;
          }

          doneCalled = true;
          done(...args);
        };

        wrappedDone.fail = (err) => {
          if (doneCalled) {
            return;
          }

          doneCalled = true;

          done.fail(err);
        };

        cb(wrappedDone).catch(wrappedDone);
      },
      timeout,
    );
  }

[manuman94]

Hi @masad-frost, thank you so much for your workaround.

We are migrating from Jasmine to Jest lots of projects that use both done callback and async functions so your method is making our lifes easier.

I modified it with two changes:

1. I called the function "it" instead of "itAsyncDone" in order to work with our previous test syntax. Because of this, I changed the "it" call inside the function with "test" as it is the same function in Jest.
2. I changed the done.fail(err) line with done(err), as Jest does not support done.fail syntax anymore. I opened this issue #11780 but it seems it is not supported anymore. With your function I made done.fail() available again.

Thank you so much, greetings.

[ronaldroe]

+1 This is breaking every test where I'm testing thrown errors. The methods I'm running are async, so I have to await them, but if I don't call done in my catch, the test times out. If I don't call done in the try, the test times out if what I'm testing doesn't throw as expected. I'm sure there's another way around it, but the change seems a bit arbitrary.

EDIT TO ADD: For my case, chaining a .catch and testing the error output there worked. It's not prime, but it works, and the tests function as expected.

[AbdelhamidGamal]

Test functions cannot both take a 'done' callback and return something. Either use a 'done' callback, or return a promise.
    Returned value: Promise {}

tried running done inside a .catch() instead of async await , but didn't work also

[molszanski]

My tests now look pretty silly because I can not use await

it("should be able subscribe to container set change", (cb) => {
  // This is silly 
  ;(async () => {
    const cont = getMainMockAppContainer()
    let containerSet = await cont.getContainerSet(["aCont", "bCont", "cCont"]) // await improves readability

    expect(containerSet.bCont.b2).toMatchObject({ a1: {} })
    expect(containerSet.cCont.c2.size).toBe(5)

    cont.subscribeToContinerSet(["aCont", "bCont", "cCont"], (containerSet) => {
      expect(containerSet.cCont.c2.size).toBe(10)
      cb()
    })

    containerSet.cCont.upgradeCContainer()
  })()
})

I would rather do this

it("should be able subscribe to container set change", async (cb) => {
  const cont = getMainMockAppContainer()
  let containerSet = await cont.getContainerSet(["aCont", "bCont", "cCont"])

  expect(containerSet.bCont.b2).toMatchObject({ a1: {} })
  expect(containerSet.cCont.c2.size).toBe(5)

  cont.subscribeToContinerSet(["aCont", "bCont", "cCont"], (containerSet) => {
    expect(containerSet.cCont.c2.size).toBe(10)
    cb()
  })

  containerSet.cCont.upgradeCContainer()
})

[molszanski]

From https://jestjs.io/docs/asynchronous should be updated to point this out, while it doesn't say you can mix and match within a single test, it should clearly state that these 2 methods of testing are incompatible.

Also, this is a breaking change, because it worked until jest 25 or something and I am sure I have hundreds of spec files written that way

[github-actions[bot]]

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

[wrslatz]

I think this issue is still valid and we have interest in it. I'd either like to see this regression fixed or some clearer guidance on why these two async test strategies should not be combined.

[github-actions[bot]]

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

[timoxley]

bump

[masad-frost]

This should probably be closed as there are workarounds, and the team is confident in the decision. Most of the examples from people mentioned in this thread are callbacks that can simply be wrapped in a promise, but here's an example where I think the oversight lies and leads to unergonomic, hard to read, and hard to write tests.

test("something", async (done) => {
  onShouldNeverHappen(() => done(new Error("should never happen")));

  const bar = await foo();

  expect(bar).toBe(1);

  done()
});

Obviously, you can do something like

test("something", async () => {
  const shouldNeverHappenFn = jest.fn();
  onShouldNeverHappen(shouldNeverHappenFn);

  const bar = await foo();

  expect(shouldNeverHappenFn).not.haveBeenCalled()
  expect(bar).toBe(1);
});

But it becomes really repetitive when you have multiple async things happening where you have to check that your callback is not called after each one and/or multiple callbacks that you expect not to be called.

What's worse is that done fails early (afaict using expect in callbacks doesn't work because jest doesn't care about expect calls failing, just expects that throw in the main function, this is another poor deviation from Jasmine, vitest handles this well), and so if shouldNeverHappenFn indicates that await foo() will hang, you will need to wrap every async call with Promise.race and promisify your callback, or you'll run into opaque timeouts.

test("something", async () => {
  const result = await Promise.race([
    foo(),
    new Promise((resolve, reject) => {
      onShouldNeverHappen(() => {
        reject(new Error("Should never happen function was called"));
      });
    })
  ]);

  expect(result).toBe(1);
});

Hopefully I'm missing something obvious here.

[molszanski]

@masad-frost This doesn't work in vitest so well too :/ https://github.com/vitest-dev/vitest/discussions/1699