New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[no-test-callback] doesn't handle done
being used to fail a test
#223
Comments
Perhaps I'm missing something, or Jest's behavior has changed at some point, but when I run the example test from the documentation: test('some test', done => {
expect(false).toBe(true);
done();
}); Jest catches the error as expected, and doesn't wait for the timeout. If that's correct, I don't see why this rule is needed at all anymore. |
Bad example, should be test('some test', done => {
setTimeout(() => {
expect(false).toBe(true);
done();
}, 10);
}); You'd never use |
I just tested that as well with the same results. The error is immediately caught by Jest without waiting for the timeout. |
@SimenB's snippet above makes sense, however @aryehb's latest comment is accurate: The following snippet does timeout like the documentation of test('some test', done => {
setTimeout(() => {
expect(false).toBe(true);
done();
}, 10000);
}); With the following output: ● some test
: Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Error:
> 10 | test('some test', done => {
| ^
11 | setTimeout(() => {
12 | expect(false).toBe(true);
13 | done();
at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:116:22)
at Object.<anonymous> (test/myTest.test.ts:10:6) Howeveeeer the 2 recommendations of the doc to either use a test('try...catch #1', done => {
try {
setTimeout(() => {
expect(false).toBe(true);
done();
}, 10000);
} catch (e) {
done(e);
}
});
test('try...catch #2', done => {
setTimeout(() => {
try {
expect(false).toBe(true);
done();
} catch (e) {
done(e);
}
}, 10000);
});
test('new Promise', () => { // Marking this as `async` made no difference
return new Promise(done => {
setTimeout(() => {
expect(false).toBe(true);
done();
}, 10000);
});
}); All those examples resulted in the exact same output. |
@SimenB have updated your patch to match the new state of things ( |
@SimenB looking over this, I'm not sure how reasonable it is to support, as there's a whole lot of ways the |
no-test-callback
doesn't handle done
being used to fail a testdone
being used to fail a test
The
done
callback in Jest can be used to fail tests, either bydone.fail()
or passing anything intodone
, like this:done('fail')
. Both of these cases are handled wrong by the rule now - always replacingdone
withresolve
, which only works when it's not supposed to fail.This will make the rule more complicated (since it has to figure out how the callback is used instead of just replacing where
done
comes from). When we get to this I'd also prefer to useresolve
as the name of the argument instead of the current logic (which reuses the name given to thedone
callback).I've added 2 failing tests:
The text was updated successfully, but these errors were encountered: