Warn that suites cannot return a value (#3550)

* Give a `DeprecationWarning` on suite callback returning any value.
* Deprecation warning: Show a message only once; use `process.nextTick` when falling back to `console.warn`
* Add a test for `utils.deprecate`
* style: Make prettier happy
* test(deprecate.spec.js): Make PR requested changes for approval (per @boneskull)

lib/interfaces/common.js

@@ -1,6 +1,7 @@
 'use strict';
 
 var Suite = require('../suite');
+var utils = require('../utils');
 
 /**
  * Functions common to more than one interface.
@@ -92,6 +93,7 @@ module.exports = function(suites, context, mocha) {
 
       /**
        * Creates a suite.
+       *
        * @param {Object} opts Options
        * @param {string} opts.title Title of Suite
        * @param {Function} [opts.fn] Suite Function (not always applicable)
@@ -109,13 +111,22 @@ module.exports = function(suites, context, mocha) {
           suite.parent._onlySuites = suite.parent._onlySuites.concat(suite);
         }
         if (typeof opts.fn === 'function') {
-          opts.fn.call(suite);
+          var result = opts.fn.call(suite);
+          if (typeof result !== 'undefined') {
+            utils.deprecate(
+              'Deprecation Warning: Suites do not support a return value;' +
+                opts.title +
+                ' returned :' +
+                result
+            );
+          }
           suites.shift();
         } else if (typeof opts.fn === 'undefined' && !suite.pending) {
           throw new Error(
             'Suite "' +
               suite.fullTitle() +
-              '" was defined but no callback was supplied. Supply a callback or explicitly skip the suite.'
+              '" was defined but no callback was supplied. ' +
+              'Supply a callback or explicitly skip the suite.'
           );
         } else if (!opts.fn && suite.pending) {
           suites.shift();

lib/utils.js

@@ -584,6 +584,34 @@ exports.getError = function(err) {
   return err || exports.undefinedError();
 };
 
+/**
+ * Show a deprecation warning. Each distinct message is only displayed once.
+ *
+ * @param {string} msg
+ */
+exports.deprecate = function(msg) {
+  msg = String(msg);
+  if (seenDeprecationMsg.hasOwnProperty(msg)) {
+    return;
+  }
+  seenDeprecationMsg[msg] = true;
+  doDeprecationWarning(msg);
+};
+
+var seenDeprecationMsg = {};
+
+var doDeprecationWarning = process.emitWarning
+  ? function(msg) {
+      // Node.js v6+
+      process.emitWarning(msg, 'DeprecationWarning');
+    }
+  : function(msg) {
+      // Everything else
+      process.nextTick(function() {
+        console.warn(msg);
+      });
+    };
+
 /**
  * @summary
  * This Filter based on `mocha-clean` module.(see: `github.com/rstacruz/mocha-clean`)

test/integration/deprecate.spec.js

@@ -0,0 +1,18 @@
+'use strict';
+
+var assert = require('assert');
+var run = require('./helpers').runMocha;
+var args = [];
+
+describe('utils.deprecate test', function() {
+  it('should print unique deprecation only once', function(done) {
+    run('deprecate.fixture.js', args, function(err, res) {
+      if (err) {
+        return done(err);
+      }
+      var result = res.output.match(/deprecated thing/g) || [];
+      assert.equal(result.length, 2);
+      done();
+    });
+  });
+});

test/integration/fixtures/deprecate.fixture.js

@@ -0,0 +1,9 @@
+'use strict';
+
+var utils = require("../../../lib/utils");
+
+it('consolidates identical calls to deprecate', function() {
+  utils.deprecate("suite foo did a deprecated thing");
+  utils.deprecate("suite foo did a deprecated thing");
+  utils.deprecate("suite bar did a deprecated thing");
+});

test/integration/fixtures/suite/suite-returning-value.fixture.js

@@ -0,0 +1,5 @@
+'use strict';
+
+describe('a suite returning a value', function () {
+  return Promise.resolve();
+});

test/integration/suite.spec.js

@@ -9,8 +9,7 @@ describe('suite w/no callback', function() {
   it('should throw a helpful error message when a callback for suite is not supplied', function(done) {
     run('suite/suite-no-callback.fixture.js', args, function(err, res) {
       if (err) {
-        done(err);
-        return;
+        return done(err);
       }
       var result = res.output.match(/no callback was supplied/) || [];
       assert.equal(result.length, 1);
@@ -24,8 +23,7 @@ describe('skipped suite w/no callback', function() {
   it('should not throw an error when a callback for skipped suite is not supplied', function(done) {
     run('suite/suite-skipped-no-callback.fixture.js', args, function(err, res) {
       if (err) {
-        done(err);
-        return;
+        return done(err);
       }
       var pattern = new RegExp('Error', 'g');
       var result = res.output.match(pattern) || [];
@@ -40,8 +38,7 @@ describe('skipped suite w/ callback', function() {
   it('should not throw an error when a callback for skipped suite is supplied', function(done) {
     run('suite/suite-skipped-callback.fixture.js', args, function(err, res) {
       if (err) {
-        done(err);
-        return;
+        return done(err);
       }
       var pattern = new RegExp('Error', 'g');
       var result = res.output.match(pattern) || [];
@@ -50,3 +47,18 @@ describe('skipped suite w/ callback', function() {
     });
   });
 });
+
+describe('suite returning a value', function() {
+  this.timeout(2000);
+  it('should give a deprecation warning for suite callback returning a value', function(done) {
+    run('suite/suite-returning-value.fixture.js', args, function(err, res) {
+      if (err) {
+        return done(err);
+      }
+      var pattern = new RegExp('Deprecation Warning', 'g');
+      var result = res.output.match(pattern) || [];
+      assert.equal(result.length, 1);
+      done();
+    });
+  });
+});