feat(rules): add no-test-callback rule

[SimenB]

Using a done callback is old-school, all the cool kids should use Promises these days. Especially because of async-await. We might also want to remove done 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

[ranyitz]

This is awesome!

[thymikee]

❤️

[thymikee]

a test with a bit more complex code would be nice to validate fixer

[SimenB]

Feel free to push some examples 🙂 Or write them here, and I can add them

[rubennorte]

@SimenB looks very good! :D

[rubennorte]

@SimenB looks very good! :D

[rubennorte]

@SimenB looks very good! :D

[rubennorte]

@SimenB looks very good! :D

[rubennorte]

@SimenB looks good! :D

[macklinu]

Good idea for a rule! 👍

[macklinu]

I think wrapping this code in a Promise as a fixer is a good first step and a consistent way to wrap code in the "return a Promise" paradigm. Do we also want to document using .resolves, .rejects or returning a Promise / await statement as other suggested alternatives?

[SimenB]

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 🙂

[SimenB]

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? 🙏

[rubennorte]

@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.

[SimenB]

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

[rubennorte]

I just saw that ava has throwsAsync for that :D

[SimenB]

Ah, new in v1, it was just throws before. But yeah, I like that pattern. Doesn't fit into expect, though

[SimenB]

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!

[SimenB]

🎉 This PR is included in version 21.26.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[ron23]

Hey @SimenB Thanks for this rule! It's great and I wanna be part of the cool kids using as/aw.
I have a question about the --fix though: It seems like it's wrapping the done arg with a promise, (basically using the done as a resolver) but I don't think it really needs to.
Why not simply remove the done argument all together and add a return? Is that much more complicated?

This is my original test:

    it('should work', done => {
      mockApi.onGet(`/account/someone`).reply(200, {});
      return Api.fetch(someone).then(result => {
        expect(result).toEqual([]);
        done();
      });
    });

And this is that the fix did:

    it('should work', () => {return new Promise(done => { => {
      mockApi.onGet(`/account/someone`).reply(200, {});
      return Api.fetch(someone).then(result => {
        expect(result).toEqual([]);
        done();
      });
    });

And this is what I want it to be:

    it('should work', () => {
      mockApi.onGet(`/account/someone`).reply(200, {});
      return Api.fetch(someone).then(result => {
        expect(result).toEqual([]);
      });
    });

Sorry for bumping old thread. If you want I can open a new ticket for it.

[G-Rath]

@ron23 this is because of the complexity around doing that, specifically finding the right done:

it('should work', done => {
  mockApi.onGet(`/account/someone`).reply(200, {});

  return new Promise(done => {
    doSomething();
    done();
  }).then(() =>
    Api.fetch(someone).then(result => {
      expect(result).toEqual([]);
      done();
    })
  );
});

In the above, how do you identify the done that should be removed? :)

To be honest, this rule should probably provide a suggestion rather than a fix :/

[SimenB]

Ideally your original example should actually be an error since you're both returning a promise and using a done callback. Mocha actually errors in this case - Jest really should do the same. Using both methods of signalling a test as asynchronous is usually an error, and it's definitely confusing.

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 done call with resolve from the promise executor.

Ideally, you'd just return the promise as you suggest, though, and have no new Promise wrapper 👍

To be honest, this rule should probably provide a suggestion rather than a fix :/

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

[ron23]

Thanks @SimenB and @G-Rath . Good stuff!
I used find & replace and ended up refactoring all of the tests that had that format. I can now safety enable the rule to prevent people from using done!

[vvo]

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!