rule to enforce returning promise in async tests instead of done

[zowers]

don't know if it is possible to enforce usage of promise returns, probably not.
at least it should be possible to check that done argument is not used and a return something is used instead

[lo1tuma]

Thanks for the issue.

What would be the intent of such a rule? Is it only about consistency or does it prevent some issues with the done callback`?

[zowers]

that would be used as a reminder of a better code made possible by the use of promises for all the project members.

most our code is promise-based except tests, which mostly done callbacks based, which happened due to lack of knowledge that mocha supports promises.

done callbacks are error-prone, it's easy to forget to catch errors and call done(err)

[lo1tuma]

Ok, sounds like it is a stylistic rule.I think we can do this by simply looking for the done argument and warn whenever it is specified. Checking if the return statement is present could be covered by #5.

[jfmengels]

I have been trying to write tests this way, and it works fine for normal cases, but I'm struggling to find a good way to find that a Promise should be rejected (with or without a check on the rejection reason). Any tips on that? I think tips for that should probably appear in the rule documentation.

[zowers]

promise should be rejected? what you mean?
the rule should disallow done callback in it/beforeEach/afterEach, and it doesn't have anything with promise state

[jfmengels]

I mean, how do you write a test where you expect a Promise to be rejected?

it('should reject when no arguments are passed', function(done) {
  lib.foo()
    .then(function() {
      done(new Error('Expected error'));
    })
    .catch(function(error) {
      assert.ok(error);
      done();
    });
});

I have trouble writing tests like these withtout the done callback and without having the test pass when the promise is not rejected.

[lo1tuma]

it('should reject when no arguments are passed', function() {
  return lib.foo()
    .then(function() {
      assert(false);
    })
    .catch(function(error) {
      assert.ok(error);
    });
});

I think that should work.

[zowers]

or using chai-as-promised

it('should reject when no arguments are passed', function() {
  return expect( lib.foo() ).to.eventually.be.rejected;
});

[jfmengels]

@lo1tuma Unfortunately, your solution doesn't work, because the error thrown by the assert.false() statement would be catched by the following catch. You could then make new assertions in the catch to check what kind of error was thrown and make sure it was not caused by the assert.false() (assuming you know what kind of error could be thrown by the tested function).

I actually remembered a solution that I used, which uses the two arguments of then

it('should reject when no arguments are passed', function() {
  return lib.foo()
    .then(function() {
      assert(false);
    }, function(error) {
      assert(error);
    });
});

This works. But I find using the two arguments instead of a catch a bit confusing and harder to read.

And yes, chai-as-promised works if you use chai, but I think this needs to be something that you can do without a special library.

[jfmengels]

Anyway, now that I have found a solution, I'm all for this rule, but the documentation should probably give tips on how to achieve this (normal case and my expected error case).