Allow --async-only to be satisfied by returning a promise

[jlai]

The --async-only flag is intended to catch programmer error where omitting the done callback from the test function arguments causes it to become a synchronous test instead, that does nothing. However, it complains if the test returns a promise, which is unhelpful/unusable when using promises or a mix of async (callback) functions and promises.

This PR proposes changing the behavior of --async-only so that it only fails if the function does not accept a done callback or return a promise. This makes the flag even more useful since forgetting to return a promise will now report an error when using --async-only.

This results in a slight change to the behavior of --async-only: instead of failing immediately for synchronous functions, check to see if the test function returned a promise (or otherwise failed due to an assert, etc) before complaining about not having a done callback.

[aheckmann]

+1

[dasilvacontin]

I would consider this a breaking change since return value was previously disregarded. Specially since some languages than compile into JS (eg CoffeeScript) automatically return in the last line of functions.

[dasilvacontin]

+1 from me.

[danielstjules]

+1 from me as well :)

@dasilvacontin I might be entirely wrong here, but I'm thinking this would only require a minor revision bump. Maybe even just a patch.

It doesn't seem to be breaking backwards compatibility for languages like coffeescript as it only affects the result of returning a promise when using the option. Any javascript/coffescript test that returned a value other than a promise will continue to get flagged when using --async-only. Any test that returned a promise, however, will no longer be incorrectly labeled as not accepting a callback - since they do (

mocha/lib/runnable.js

Lines 267 to 275 in ca84810

	if (result && typeof result.then === 'function') {
	self.resetTimeout();
	result
	.then(function() {
	done()
	},
	function(reason) {
	done(reason || new Error('Promise rejected with no or falsy reason'))
	});

) I'd see this as fixing a bug.

[danielstjules]

@mochajs/owners any thoughts on this?

[dasilvacontin]

It doesn't seem to be breaking backwards compatibility for languages like coffeescript as it only affects the result of returning a promise when using the option.

True, I think I didn't look at the code enough. LGTM. minor?

Reminder to update documentation if this gets merged.

[boneskull]

@mochajs/mocha

OK, I've created branch v2.3.0 for the next major minor, and merged this via 7be4891.

[dasilvacontin]

next major

minor?

[boneskull]

@dasilvacontin minor 😄

[dasilvacontin]