Skip to content
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

Implement no-return-from-async rule #190

Merged
merged 5 commits into from
Jul 17, 2019

Conversation

papandreou
Copy link
Contributor

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 papandreou changed the title Implement no-return-from-async-test rule Implement no-return-from-async rule Mar 5, 2019
@papandreou papandreou force-pushed the feature/no-return-from-async branch from 7c55c58 to a548690 Compare March 5, 2019 14:26
@papandreou
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@lo1tuma
Copy link
Owner

lo1tuma commented Jul 15, 2019

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
Copy link
Contributor Author

@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
Copy link
Owner

lo1tuma commented Jul 15, 2019

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@papandreou
Copy link
Contributor Author

papandreou commented Jul 16, 2019

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
Copy link
Contributor Author

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
Copy link
Owner

lo1tuma commented Jul 17, 2019

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.

Copy link
Owner

@lo1tuma lo1tuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍. Thank you!

@lo1tuma lo1tuma merged commit 31819a2 into lo1tuma:master Jul 17, 2019
@robertrossmann
Copy link

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))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants