How to avoid false positives now?

[bitsoflogic]

Back on #621, the --async-only feature was created to force developers to use done as a work-around to mocha not knowing anything about the assertion libraries. It provided a way for devs to ensure their assertions were hit.

It looks like this functionality is now lost when returning promises (See #1490, #1636), which hurts when using Babel's awesome async-to-generator plugin. With the --async-only flag on, false positives are possible again as you're no longer forced to use done with it enabled.

// Babel example
it('will produce a false positive', async () => {
  // no assertions or code required for a passing test here.
});

Thoughts?

[ScottFreeCode]

--async-only was never about assertions as assertions; you can write a useless test with done just as easily as with promises (or async functions):

// passes even with --async-only
it("function with no assertions", function(done) {
  setTimeout(done, 1000)
})

All that done did (and --async-only enforced by requiring done) was to prevent asynchronous test code from being missed merely because it's asynchronous:

// ! oops, Mocha thinks this test passed once the function returns
it("invalid asynchronous test", function() {
  setTimeout(function() { // (more realistically, could be an asynchronous API with result passed to callback)
    assert(whatever) // ! Mocha doesn't know to wait for the callback
  }, 1000)
})

// ! ok, Mocha now knows to wait for `done` to be called
it("valid asynchronous test", function(done) {
  setTimeout(function() { // (more realistically, could be an asynchronous API with result passed to callback)
    assert(whatever)
    done() // ! Mocha knows the test is complete at this point
  }, 1000)
})

But promises accomplish the same thing because Mocha waits for the promise to resolve or reject:

it("valid asynchronous test", function() {
  return new Promise(function(resolve) {
    setTimeout(function() { // (more realistically, the promise might come from a promise API)
      assert(whatever) // if this throws, the promise becomes rejected
      resolve()
    }, 1000)
  })
})

And async functions are just sugar for promise chains:

it("hypothetical promise-based test", function() {
  return somePromiseAPI("what data do we want")
    .then(function(data) {
      assert(data)
    })
})
it("equivalent async-function test", async function() {
  var data = await somePromiseAPI("what data do we want")
  assert(data)
})

[bitsoflogic]

It's true that you can certainly write useless tests with done. However it ensures the test is truly done. Having a Promise resolve isn't necessarily an indication that the test has completed. Since it's easy to forget to include done (user error), it's possible to have false positives where a simple "You said you'd always define done and didn't" error message would get you on track.

In this example, the assertion is only being tested if the route.del properly calls it:

it('can create a false positive', async function() {
  var response = {
    send: function(value) {
      value.should.equal(400);
    }
  };

  var request = await _getRequestObject();
  route.del(request, response, function() {});
});

With done we can be confident that it reached the end of the test or timed out:

it('can create a false positive', async function(done) {
  var response = {
    send: function(value) {
      value.should.equal(400);
      done();
    }
  };

  var request = await _getRequestObject();
  route.del(request, response, function() {});
});

The problem is that async returns a Promise that does not indicate the end of a test, and --async-only no longer enforces that done was defined. This was a design change from the 2.x and earlier series. I do understand the problem the change addresses. I guess I'm just looking for the same safety net aheckmann was back in #621. I've been using his PR ever since I started with mocha a few years back for the same reason it was requested in the first place.

[ScottFreeCode]

So if I'm understanding this right, you're testing a mix of promises and callbacks, and need to finish the test in the callback rather than the promise, and async is implicitly returning a promise. Is that accurate? (I believe we had this come up somewhere else recently, I'll see if I can track it down.)

[ScottFreeCode]

The other issue was #2494; it's unfortunate niether of these opens with the real problem situation of testing callbacks with async tests that always return a promise.

One basic approach would be similar to the potential solution I suggested there: don't use async (or return promises) if you need to test a callback. That being said, that advice would be rendered unnecessary if #2509 does end up making Mocha wait for both the callback and the promise in these cases. Then it would just be a question of, how can done for the callback be enforced where it's needed when promises are also leading to lots of async?

I doubt that Mocha's going to have any reliable, cross-environment way to detect async functions (unless there's a standards-mandated way of identifying them that I'm not yet aware of), so we probably won't be able to do anything special for that. And the general case that --async-only guards against is normally solved by promises just as well as by done (or better, since it can be simpler in many cases): the promise lets Mocha wait for promise-based tests to complete (in ordinary promise-based tests where there isn't a callback waiting too); so we can't just make it start forcing done on tests that are legitimately using promises to complete the test more simply.

Maybe we just need a new, distinct flag to require done for tests that need callbacks even if they return promises, with clear documentation to the effect that --async-only is what you want instead unless you specifically are testing callbacks and want to use async tests to handle promises within the test too...

[bitsoflogic]

Yeah, I'd be fine with something like a --require-done flag being added. I do a lot of express/restify callback style work and really love the simplicity async-await provides for working with database setup and the like.

I do think #2509 is a separate issue in that it deals with explicitly allowing both to exist.

[boneskull]

--async-only no longer enforces that done was defined

I'm not sure why --async-only doesn't barf if you don't use done, regardless of what the test returns. I don't recall an intentional change to that behavior, so this might be a regression.

Labeling unconfirmed because it's not clear to me if this is a bug (though I'm suspecting it is).

[stale]

I am a bot that watches issues for inactivity.
This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue.
Thanks for contributing to Mocha!

[boneskull]

imo this is a subset of whatever the solution will be in #2509.

so, duplicate of #2509