Give a deprecation warning if a suite callback returns a value.

[jleedev]

Description of the Change

As long as this is not supported, the behavior is confusing. It can produce a suite that is not properly initialized. Turning this into a hard error will make it clear that this is not supported.

In short, the test suite below currently does nothing; this change will make it into an error.

describe("my suite", async function() {
  await Promise.resolve();
  it("does a thing", function() {
    assert(false);
  });
});

Other usage could possibly produce a partially-initialized test suite that fails for unknown reasons; this will make it fail obviously.

Alternate Designs

Original proposal was to throw, but settled for a deprecation warning in the accepted version.
This would need be revisited in future to throw an Error instead.

Why should this be in core?

N/A

Benefits

Reduction in user confusion/expectations attempting to use describe function as anything but a namespace.

Possible Drawbacks

Users will get deprecation warning for unsupported behavior.
No real downside to knowledge.

Applicable issues

Fixes #2975
semver-patch

[jsf-clabot]

All committers have signed the CLA.

[coveralls]

Coverage decreased (-0.03%) to 89.973% when pulling 0570f42 on jleedev:async-describe into ffd760e on mochajs:master.

[Bamieh]

@jleedev Awesome!

Indeed the describe function should not be async, the approach is to use before for such cases.

[boneskull]

Thanks. I'm not sure adding more checks around ways in which Mocha can be misused is something we need to be in the business of. Nowhere in the documentation does it state that the return value of a suite's callback is consumed.

Similarly, you could pass a done callback to the suite, which is also unsupported--should we add a special exception for that? It's really a slippery slope...

[boneskull]

@Bamieh Given my comments above, do you still think this should be merged?

[jleedev]

My argument here is that "just slap async on the function" is so tempting to integrate with whatever resources are needed, and it's easy to break your entire test suite in a subtle way. I don't think it's at all the same as attemping to pass done to the suite (and not just because calling done() would raise a runtime error). It's easy to forget that function and async function have a big difference, exactly because the point is to make async code look similar to synchronous code.

That being said, it could be beneficial to call out a suggested fix — "You probably want to use before() instead."

[ORESoftware]

if the describe block returns anything truthy, throw an error

[boneskull]

I get what you're saying about before(), but it happens at a totally different phase of execution. You don't want to mix that stuff up.

I like @ORESoftware's suggestion, actually, but that is a breaking change, because it doesn't necessarily cause anybody's tests to fail. For example:

describe('something', function () {
  it('is my test', function () {
    // ...
  });

  return true;
});

It's pointless, but it currently works, unlike using async function.

What would we think of adding a soft deprecation warning if a value is returned from a describe block, instead of an error? Next major would then toss an exception.

[ORESoftware]

@boneskull add a warning if anything truthy is returned for the next few patches, then maybe next minor release throw an error?

[boneskull]

@ORESoftware well, next major since it's breaking, but yeah, that's the idea.

[ORESoftware]

yeah I guess that makes sense

[boneskull]

@jleedev are you on-board changing this to a deprecation message? basically:

• If anything is returned from a suite, let's warn. Use something like:

  // lib/utils.js (untested)
  exports.deprecate = process.emitWarning 
    // Node.js v6+
    ? function (msg) { process.emitWarning(msg, 'DeprecationWarning'); }
    // Everything else
    : function (msg) { console.warn(msg); };
  
  // elsewhere
  if (suiteReturnsSomething) {
    utils.deprecate('Returning a value from a Suite is deprecated; this will throw an exception in the next major version of Mocha');
  }

• Ensure this is done in a way which will work in a browser

[jleedev]

Yes, I like that. Branch updated with this strawman. Robots are running the browser tests.

[Bamieh]

@boneskull IMHO we should not be warning users for mocha misuse, so in most cases we should not be doing this. However i feel that this warning is reasonable to prevent very subtle errors, but again it is restricting our users for no real reason on the library level, a small warning text in the documentation might suffice here.

[boneskull]

Thanks, looks good. I have a couple changes needed to avoid user complaints 😉

[boneskull]

Sorry to complicate this, but it's a bit more complicated. 😝

1. The user should only see this notice once. For each unique msg, we need some to keep some flag (probably in an object, as its key, since we have no access to Set) and do nothing if the flag is already set. For safety, coerce msg into a string via msg = String(msg)
2. Internally, process.emitWarning emits on the next tick. So, console.warn(msg) must happen within process.nextTick as well. This is to avoid bloating or interfering w/ the call stack

[boneskull]

Also, please rebase onto master to make the Snyk warning go away

[jleedev]

If process.nextTick is unavailable (outside of Node), is it preferred to use Promise.resolve().then(…) or setTimeout(…, 0)?

(Aha, Snyk wouldn't actually show me anything.)

Did this change and rebased.

[jleedev]

Does this also need to hook into the --no-deprecation flags?

[boneskull]

Thanks! Looking good.

There's a unit test suite for the utils module; could you add a quick unit test for exports.deprecate()? It's throwing off the coverage numbers.

After we have a test (and the other change I mentioned) it'll be good to merge. Great work!

[jleedev]

unit test suite for the utils module

I actually added to the integration tests. Is it possible to verify a call to console.warn otherwise?

[plroebuck]

utils.deprecate('Returning a value from a Suite is deprecated; this will throw an exception in the next major version of Mocha');

The message itself implies that this was previously supported.

'Returning a value from a Suite was previously undocumented; it is now explicitly verboten and will throw an exception in the next major version of Mocha'

[boneskull]

@plroebuck

The message itself implies that this was previously supported

I disagree; it was not disallowed, not supported. If you pass an extra parameter to any given function in Mocha's API, if it doesn't break, that doesn't imply it's supported.

@Bamieh

@boneskull IMHO we should not be warning users for mocha misuse, so in most cases we should not be doing this. However i feel that this warning is reasonable to prevent very subtle errors, but again it is restricting our users for no real reason on the library level, a small warning text in the documentation might suffice here.

I agree that we shouldn't generally be in the business of warning users about misusing the API. The only reason I'm entertaining it is because of the potential for causing difficult-to-solve problems.

IMO the real problem here is that suites don't support async behavior. Maybe that should change. If we do want that to change (at some point), we shouldn't merge this, and instead add a note to the docs.

@mochajs/core would love further thoughts

[Bamieh]

@boneskull I agree with you about not merging this if there is a possibility of having async suites.

I do not see the value of supporting async suites, as I usually encourage people to write all the fetching logic in the before of the suite, the describe body can only hold empty var/let variables to be assigned in the test cases of the suite to solve the function scoping (some people use this directly to avoid this as well, but we do not encourage it). Following that logic, having async describe might be contradictory and unnecessary.

[jleedev]

I don't understand why the possibility of async suites in the future should prevent merging this right now. Either way, adding async suites will break compatibility to the same degree, and the warning can be removed. No one should be returning a Promise from a suite before they're actually supported.

[plroebuck]

@jleedev #2975 would be "fixed" (for certain definitions of fixed) by this PR, right?

[plroebuck]

@boneskull were your concerns addressed?
if so, could you resolve any remaining conversations and explicitly approve?

[boneskull]

I don't understand why the possibility of async suites in the future should prevent merging this right now. Either way, adding async suites will break compatibility to the same degree, and the warning can be removed. No one should be returning a Promise from a suite before they're actually supported.

churn, mainly. but async suites is probably a ways off.

[boneskull]

@plroebuck I've noted two problems. fine to merge and then fix them after; I don't want to ask @jleedev to do more work.

[boneskull]

I've "approved", but the stuff I noted must be addressed one way or another

[plroebuck]

I'll ask. Since this was intended for patch release, plan going forward is to?

1. correct to throw error instead of deprecation warn (for 6.0) Gomer's reaction
2. merge as is and throw error (for 7.0)