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

[lo1tuma]

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

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