Implement no-return-from-async rule

[papandreou]

While upgrading a codebase to use async/await I've found it to be a bit confusing when the two Promise styles are mixed so a Promise is returned from an async test:

it('should foo', async function () {
  await bar();
  return expect(somePromise).to.eventually.be.rejectedWith('bla');
});

I've implemented a rule that prohibits mixing these two styles, similar to to the no-return-and-callback. And pretty much all copy/pasted from it because of the similarities :)

[papandreou]

I guess this could be extended to disallow returning from any it, even non-async ones. Then you could enforce a code style where all tests that do async work actually are async functions.

[papandreou]

@lo1tuma, sorry to "at" you, but is this something you would consider adopting?

[lo1tuma]

Hi @papandreou, sorry for the late response. I think this rule makes sense. I haven’t had a closer look at the code yet (I hope I find the time soon), but I’m a bit concerned about the copy-pasted code you mentioned in the PR description. Is there any chance to extract and re-use similarities?

[papandreou]

@lo1tuma, thanks for taking a look!

I'll see about move the copy/pasted helper functions into astUtils.

What do you think about the idea of having a mode where returning from any it, even non-async ones is disallowed?

[lo1tuma]

What do you think about the idea of having a mode where returning from any it, even non-async ones is disallowed?

What would be the rationale behind it? But now that your are saying it I could imagine a generic no-return rule which can be configured to forbid return always or just in combination with callbacks or async functions. That would also solve the duplicated code problem.

[papandreou]

@lo1tuma,

What would be the rationale behind it?

It would be a way to enforce the use of async/await whenever promises are involved in a test, prohibiting eg.

it('should foo', function () {
  return somethingThatReturnsAPromise();
});

... while still allowing a value-less (early) return:

it('should foo', function () {
  if (bar) {
    return;
  }
  // ...
});

... seeing that a promise is the only type of return value from an it that mocha handles.

So it's really mostly a stylistic thing. I came up with the idea when I did a sweep through a test suite that used promise chains, converting it to async/await -- I guess the use case for that idea is probably quite narrow :)

[papandreou]

I've moved the identical helper functions into astUtils now, reducing the duplication quite a lot.

[papandreou]

But now that your are saying it I could imagine a generic no-return rule which can be configured to forbid return always or just in combination with callbacks or async functions. That would also solve the duplicated code problem

My gut feeling is that handling returns in callback-based and async its would be too much to cram into a single rule. I think the "disallow returns of anything but undefined" mode would be more thematically linked to the async case. So I think it should go into no-return-from-async (if we want it at all).

[papandreou]

So I think it should go into no-return-from-async (if we want it at all).

... Right, except that the naming would be off because the rule would then also affect non-async functions 😩. I think I get why you suggested the no-return rule as a compromise :)

[lo1tuma]

Maybe it is the best to make small steps. Let’s start with those two separate rules for now and once we have gained some usage experience we can think about if it would make sense to combine them in a generic no-return rule.

[lo1tuma]

LGTM 👍. Thank you!

[robertrossmann]

Regarding a rule disallowing return completely:

It would be nice, as long as arrow functions with implicit return are allowed. Consider:

// Much nice, such arrow
it('works with a promise', () => expect(getpromise()).to.eventually.not.throw(Error))