Rule proposal: Forbid tests with both a callback and a return statement

[jfmengels]

In Mocha v3, it is not allowed to return a Promise in tests while the test function takes a done callback function.

It would be nice to detect this, so that you have a tighter feedback loop when coding, instead of waiting for your test to run. It could also help with the migration to v3 (I did this yesterday in a relatively big project, and it was a bit time-consuming).

I'm not sure whether we should forbid (top-level) return statements altogether, or only when we can detect that a Promise is returned (with a .then call). I think the former would be fine, as there is little value in returning something in a test when you have a callback, and it would cover a lot more cases where we can't know for sure that a Promise is being returned.

Incorrect

it('should XYZ', function(done) {
  return foo()
    .then(bar)
    .nodeify(done);
});

Correct

it('should XYZ', function(done) {
  foo()
    .then(bar)
    .nodeify(done);
});

it('should XYZ', function() {
  return foo()
    .then(bar);
});

As for the rule name, I'm not too sure yet. no-return-and-callback? Or if we want to make sure the returned statement is a Promise, no-promise-and-callback?

[lo1tuma]

I general I like this proposal. I think the hard part of this would be detect if a promise is returned or not. Do you know how mocha does this check? Does it fail for any non falsy value that is returned or only for objects that have a then method?

Maybe the easiest approach would be to warn on any top-level return (maybe with exceptions for return; return undefined; and return null;). I think looking for the usage of calling a then method within the return statement is probably pretty error-prone. Consider the following examples:

it('foo', function () {
    // chai-as-promised assertions returns promises
    return expect(promiseReturningFunction()).to.be.fulfilled;
});

it('foo', function () {
    return Promise.all([ checkFoo(), checkBar() ]);
});

[jfmengels]

Does it fail for any non falsy value that is returned or only for objects that have a then method?

Only for Promises (so the latter). This is correct behavior for instance:

describe('foo', function() {
  it('should', function(done) {
    function foo() {
      done();
    }
    setTimeout(foo, 500);
    return 3;
  });
});

But returning anything other than a Promise doesn't provide any value, so I'd say it's fine to warn if there is a return statement and a callback.

Maybe the easiest approach would be to warn on any top-level return

Yes, that is what I was thinking too.

maybe with exceptions for return; return undefined; and return null;

Not sure I see the point of having such statements in your tests anyway 😕

I think looking for the usage of calling a then method within the return statement is probably pretty error-prone

Same here. Sometimes you just want to call catch, or custom Bluebird methods for instance.

[lo1tuma]

Not sure I see the point of having such statements in your tests anyway

Maybe it is used to skip further execution of the test, but maybe you are right it is uncommon to do that anyway 😉 .

[jfmengels]

Maybe it is used to skip further execution of the test, but maybe you are right it is uncommon to do that anyway 😉 .

Ah, you mean something like this

it('should xyz', function() {
  const res = foo();
  assert(res);
  if (!process.env.FOO) {
    return;
  }
  assert(res.xyz === 'foo');
});

Would make sense if you're testing stuff conditionally based on the environment, or creating tests in a loop. You're right, I'll make the change later to ignore this when you return a nothing (return;) or a literal value (return null/"foo") or undefined.

[jfmengels]

You're right, I'll make the change later to ignore this when you return a nothing (return;) or a literal value (return null/"foo") or undefined.

That has been done in PR #94.