be more friendly about async "overspecified" errors; closes #2407

[boneskull]

This is what I got for #2407. It seems to work well, but I'm worried actually forcing async tests to be async is going to break some tests out there--especially amongst those using --async-only.

Regardless of the above, I'm pretty sure what I've done here is "more correct."

I'm happy to pull that out, but the result is we cannot warn a user (or in fact do anything) about:

it('should blah', function (done) {
  return new Promise(function () {
    done();
  });
});

Yes, they returned a Promise, but it executes synchronously, so we can't inspect the return value of the test to even know it's a Promise.

cc @mochajs/core @elado @teckays @ScottFreeCode

[boneskull]

cc @papandreou

[dasilvacontin]

Interesting edge case. First workaround that comes to mind would be waiting for test sync execution to finish before paying attention to any calls to done.

[papandreou]

Remove .only. (This is why I always use -g myself :)

[boneskull]

😊

[papandreou]

Awesome. This is very close to what I had in mind when I looked at #2407. Haven't played around with your implementation, but the tests look sane :)

[boneskull]

I noticed that the stack traces here are useless. Making them better.

[boneskull]

And then I noticed:

• there are a whole lot more edge cases I didn't originally consider
• I've yet to determine a foolproof way of showing what the Promise was rejected with, what done(err) was called with, or both, and in what order.
• I'm seeing a lot of potential improvements around throwing "internal" errors (timeouts, done() called twice, this stuff, etc).

This is becoming kind of a rabbit hole. I'd rather do this the right way instead of half-assed. I want to shelve this for now; better documentation around the issue could help.

Is anyone dying to get this in?

[boneskull]

@ScottFreeCode mainly I'm concerned about adding a bunch of complexity around this. The idea was to help the user see they likely have a bug, or are otherwise misusing the API. The challenge is both covering all of the weird cases that exist (I can think of 7-8 offhand) and providing as much information as possible.

I'm not thrilled about forcing an "async" test to actually be async. On some level that makes a lot of sense. But IMO, it's too severe of a change for this issue. If we don't do it, we reduce the number of cases we can recognize and report to the user.

I can reduce this PR to cover the single case reported in #2407. That would be best course of action, since Mocha can't get too smart without significant changes to the runner.

Thoughts?

[ScottFreeCode]

I'm not thrilled about forcing an "async" test to actually be async. On some level that makes a lot of sense.

To clarify, we're not talking about disallowing, say, a Promise API that's being tested from using Promise.resolve(), right? That's a legitimate optimization that shouldn't affect whatever's using (or testing) the returned promises. If I'm following correctly, the issue is that we don't know what the function has returned by the time done is called if done is called synchronously, which is a more narrow case.

If we don't do it, we reduce the number of cases we can recognize and report to the user.

Maybe if done is called synchronously during the test, wait to act on it till the test also returns, so we can check whether the return value was a promise?

I can reduce this PR to cover the single case reported in #2407. That would be best course of action, since Mocha can't get too smart without significant changes to the runner.

Sounds reasonable to me; I may be repeating myself, but while I don't have a strong opinion on whether using promises and done together should be disallowed (I can see where in some cases it's redundant and in other's it's a probable bug, but also cases where it would be the most straightforward way to express the intended test behavior), my main concern with #2407 is that I don't think the timeout error is helpful in a completed-by-promise test where adding the done call would be a nominally different error.

[boneskull]

We are operating under the assertion that there is no valid use case for
doing both. Can you think of one?

[ScottFreeCode]

I thought there were some examples in the discussion surrounding the original issue. Not things where anyone had to use them together, just things where it might be marginally simpler to do so than to use promises alone. E.g. #2407 (comment), #2407 (comment) (I also have a vague recollection that it came up in Gitter at some point, but Gitter's not great for finding past conversation...)

I don't have a strong opinion on the value of such constructions, however; so I am fine either way when it comes to how extensive we want the current fix to be.

PR looks fine as far as I can tell at the moment, by the way, although I should really download it and try it out.

[boneskull]

Those two comments are both hypothetical; I would like to see an actual test that needed to use both Promises (or async/await) and done without a reasonable workaround.

cc @elado

UPDATE (Because in the general case this is a bug. We could downgrade it to a warning if somebody shows that it's useful and necessary to use both.)

[boneskull]

@ScottFreeCode This is one of those changes that, while not strictly violating semver, would potentially cause complaints about tests which magically start failing upon a minor or patch upgrade.

The crux of it is here.

In master, when you supply done in Mocha, your test does not run asynchronously.

For emphasis, your test does not run asynchronously.

Instead, done gives your test the capability of running asynchronously (via a callback).

With this change, your test would finish some milliseconds after you told it you were finished by calling done(). The most likely problem would be timeouts. In weird, synchronous tests which execute code after they call done(), this code would actually execute, potentially causing hard-to-trace exceptions.

Whether or not this is more correct or less correct than the current behavior is unclear (though I said it seemed "more correct" originally). The "most correct" thing, if we wanted to force async, would be to run the test function in a process.nextTick() or setImmediate() wrapper. But I don't know if we want to force async. At the very least, we should be more clear in the docs about how this works.

[bitsoflogic]

Those two comments are both hypothetical; I would like to see an actual test that needed to use both Promises (or async/await) and done without a reasonable workaround.

@boneskull My general use-case for using both is that there's no built-in ability to ensure that assertions are run. The pattern embraced is to use --async-only forcing our devs to use done. This way, our tests always have assertions immediately followed by done. The new async-await pattern makes for some great code readability.

it('does not allow deleting', async function(done) {
  var response = {
    send: function(val) {
      val.should.equal(400);
      done();
    }
  };

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

Without done, if route.del returns and never calls response.send the test would just silently pass.

P.S. I love the library and the work you're doing here. I hope you guys are able to find a way to allow async-await with done without breaking changes elsewhere. In the meantime, I'm fixing myself to Mocha 2.5.3 which supports using them together. If it helps, I'm using mocha --compilers js:babel-register

[dasilvacontin]

@bitsoflogic At #1320 (comment) I suggest checking for both Promise fulfillment and 'done' execution if both are 'requested'.

[boneskull]

with v2.x the behavior is unexpected w/r/t Promise fulfillment, and at worst non-deterministic. Again, this is the main reason it was eliminated.

A compromise would be to, if a Promise is returned, to change the definition of done--if and only if it has not yet been called--to wait upon its call before continuing to the next test. Otherwise--when done was already called--wait for Promise fulfillment instead. If the Promise is rejected via exception or otherwise, or done() is called with an error parameter, they must cause each other to fail. So for the implementation, there must be only "one way out" of an async Runnable. Doable, but likely non-trivial.

The difference between this behavior and 2.x is that if you use both, the Promise fulfillment is effectively ignored. In this proposal, neither would be ignored.

Thoughts? Would this work for everyone?

[dasilvacontin]

Hmm, that sounds like the 'waiting for both' I'm suggesting. I'd add reporting whether the Error came from the Promise or from 'done'.

On 28 Sep 2016, at 19:35, Christopher Hiller notifications@github.com wrote:

with v2.x the behavior is unexpected w/r/t Promise fulfillment, and at worst non-deterministic. Again, this is the main reason it was eliminated.

A compromise would be to, if a Promise is returned, to change the definition of done--if and only if it has not yet been called--to wait upon its call before continuing to the next test. Otherwise--when done was already called--wait for Promise fulfillment instead. If the Promise is rejected via exception or otherwise, or done() is called with an error parameter, they must cause each other to fail. So for the implementation, there must be only "one way out" of an async Runnable. Doable, but likely non-trivial.

The difference between this behavior and 2.x is that if you use both, the Promise fulfillment is effectively ignored. In this proposal, neither would be ignored.

Thoughts? Would this work for everyone?

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[bitsoflogic]

It sounds like you're both describing the same thing. I'm fine with that solution, although I think this would be best if we keep both the non-Promise and Promise done behavior the same. To elaborate, I've put together a few examples.

it('will work', function(done) {
  done();
  throw new Exception('oops');
});

// This was the original behavior
// Results in 1 passing, 1 failing test (even though there's only 1 test)

it('will work', async function(done) {
  done();
  // async promise throws...
});

// Results in 1 passing, 1 failing test (even though there's only 1 test)

or, keep them the same this way:

it('will work', function(done) {
  done();
  throw new Exception('oops');
});

// Results in 1 failing test

it('will work', async function(done) {
  done();
  // async promise throws...
});

// Results in 1 failing test

[boneskull]

@bitsoflogic I'm not sure I quite understand your examples. Can you be more explicit (where are Promises returned, or not returned, etc, and/or equivalent async/await usage)?

But anyway, it does sound like we're talking about the same thing.

I could implement this, but I'm not sure when that would happen. So if someone wants to beat me to it, I'll create a new issue.

[dasilvacontin]

Sounds like a fun / cool thing to code.

On 28 Sep 2016, at 20:45, Christopher Hiller notifications@github.com wrote:

Closed #2413.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[jtakalai]

@boneskull The way I ended up reading these threads was through https://github.com/mochajs/mocha/wiki/Spies

I thought the latter method (using done in EventEmitter callback) seemed very nifty, and got that perplexing error. Reason is my function is async and I do await-stuff in it; in other words, my test is multi-step async stuff, AND I'd like to resolve it by done-ing upon emitted event.

The way I'm going to do it now is to either just use the first suggested method (sinon.spy()) or stuff like https://www.npmjs.com/package/await-event. But that using done in an async function is out of question isn't trivial at all, mostly because async functions look so much like normal functions syntactically. Who even uses Promises anymore directly when you have async/await :coolface: