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
feat(rules): add no-test-callback
rule
#179
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome!
Co-Authored-By: SimenB <sbekkhus91@gmail.com>
0b79d64
to
421fa8b
Compare
context.report({ | ||
node: argument, | ||
message: 'Illegal usage of test callback', | ||
fix(fixer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
}, | ||
], | ||
output: | ||
'test("something", async () => {await new Promise(done => {done();})})', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a test with a bit more complex code would be nice to validate fixer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to push some examples 🙂 Or write them here, and I can add them
@SimenB looks very good! :D |
3 similar comments
@SimenB looks very good! :D |
@SimenB looks very good! :D |
@SimenB looks very good! :D |
@SimenB looks good! :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea for a rule! 👍
test('some test', () => { | ||
return new Promise(done => { | ||
expect(false).toBe(true); | ||
done(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thinking is that returning a promise is a safe change, as it will always work (I think?). Messing with the body of the tests are both way harder and more brittle. The current solution also makes no assumptions about assertions.
Regarding resolve
and rejects
, I actually want to remove them from Jest and ask people to use async-await
. That's a whole other can of worms, though 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I now understood that you meant the docs... I thought you talked about the fixer, sorry!
Yeah, we should definitely include some prose about using at least async-wait
over returning the promise (async functions returns promises implicitly, so it's almost always a better idea anyways). Mentioning resolves
and rejects
(especially rejects
as resolves
i better handled by await
) is probably a good idea as well.
Would you mind sending a PR for it? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimenB rejects is very useful to avoid using logic in try/catch blocks (like .toThrow
but for async functions), so I would keep it (and resolves
for consistency) unless we come up with an equally good alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for sure.
What I want is same as ava (which returns the error from t.throws
, but I'm not sure if that makes sense for expect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw that ava has throwsAsync
for that :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, new in v1, it was just throws
before. But yeah, I like that pattern. Doesn't fit into expect
, though
Co-Authored-By: SimenB <sbekkhus91@gmail.com>
I've ran this on the Jest codebase without it crashing, or reporting false positives, BTW. So should be fairly good. I'd love for people to try it out though! |
🎉 This PR is included in version 21.26.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hey @SimenB Thanks for this rule! It's great and I wanna be part of the cool kids using as/aw. This is my original test:
And this is that the fix did:
And this is what I want it to be:
Sorry for bumping old thread. If you want I can open a new ticket for it. |
@ron23 this is because of the complexity around doing that, specifically finding the right
In the above, how do you identify the To be honest, this rule should probably provide a suggestion rather than a fix :/ |
Ideally your original example should actually be an error since you're both returning a promise and using a So if we say you only use the done callback and don't return the promise (which is the intended way): it('should work', done => {
mockApi.onGet(`/account/someone`).reply(200, {});
Api.fetch(someone).then(result => {
expect(result).toEqual([]);
done();
});
}); Then the only (relatively) safe transform we can do is wrapping the whole thing in a promise and replacing the Ideally, you'd just return the promise as you suggest, though, and have no
This rule predates suggestions existing in eslint by a year. But yes, I agree - this is not really a safe fix to apply in all cases |
Hey @SimenB I just got warned about this, then I tested the example you gave that should timeout and it does not times out, it just show the error: https://codesandbox.io/s/jest-example-lcpkg Could you precise maybe in the documentation what's really wrong with this pattern? Like with a use case involving maybe a time where it will really timeout? Thanks! |
Using a
done
callback is old-school, all the cool kids should use Promises these days. Especially because ofasync-await
. We might also want to removedone
from a future version of Jest, and this is a nice start to make people migrate./cc @rubennorte, since we talked about something like this