Mixed Test Completion Styles
ID |
javascript.test_callback_completion_style |
Severity |
high |
Remediation Complexity |
trivial |
Remediation Risk |
low |
Remediation Effort |
low |
Resource |
Reliability |
Language |
JavaScript |
Tags |
async, reliability, testing |
Description
Reports a test or hook callback that declares a done parameter and is async or returns a promise. The two are the framework’s two separate ways of saying "this test is not finished yet", and a callback may use one of them.
A parameterised declaration such as it.each(rows)('…', async (a, b) ⇒ …) is never reported: its callback’s parameters are the row’s columns. Nor is a destructured parameter such as async ({ page }) ⇒ …, which is how fixtures are handed to a test.
"Returns a promise" is read from the shapes the source states outright — an async callback, a returned .then() chain, and a returned Promise.*(…) or new Promise(…).
Rationale
// Bad
it('saves the draft', async (done) => {
await save(draft);
expect(store.draft).toEqual(draft);
done();
});-
The outcome depends on which signal arrives first: the runner settles the test when the returned promise resolves, without waiting for the handshake.
-
Assertions written after
done()— and the errordone(err)was meant to report — are then decided against a test already recorded as passed. -
Frameworks that treat the combination as a defect only do so at run time, and only for the tests that actually reach the line; a callback behind a conditional stays green and stays wrong.
-
The reader cannot tell which of the two mechanisms the test relies on, so later edits break it in ways the diff does not explain.
Remediation
Pick one. Drop the done parameter when the callback is async, or keep done and make the callback synchronous.
// Good
it('saves the draft', async () => {
await save(draft);
expect(store.draft).toEqual(draft);
});
it('saves the draft', (done) => {
store.on('saved', () => {
expect(store.draft).toEqual(draft);
done();
});
});