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

lib/runnable.js

@@ -20,6 +20,7 @@ var setTimeout = global.setTimeout;
 var setInterval = global.setInterval;
 var clearTimeout = global.clearTimeout;
 var clearInterval = global.clearInterval;
+var immediately = global.setImmediate || process.nextTick;
 /* eslint-enable no-unused-vars, no-native-reassign */
 
 /**
@@ -353,8 +354,22 @@ Runnable.prototype.run = function(fn) {
     }
   }
 
+  // we'll call this if the user pledges an async function and returns a promise.
+  function overspecified() {
+    var err = new Error('Resolution method is overspecified. Specify a callback *or* return a Promise; not both.');
+    function overspecifiedFailure(anotherError) {
+      if (anotherError) {
+        err.message += '\nAdditionally, the test resulted in an error:\n' + utils.stringify(anotherError);
+      }
+      done(err);
+    }
+    return overspecifiedFailure;
+  }
+
   function callFnAsync(fn) {
-    var result = fn.call(ctx, function(err) {
+    var fail;
+
+    function asyncDone(err) {
       if (err instanceof Error || toString.call(err) === '[object Error]') {
         return done(err);
       }
@@ -365,11 +380,33 @@ Runnable.prototype.run = function(fn) {
         }
         return done(new Error('done() invoked with non-Error: ' + err));
       }
-      if (result && utils.isPromise(result)) {
-        return done(new Error('Resolution method is overspecified. Specify a callback *or* return a Promise; not both.'));
+
+      // if we get here, then the user has returned a Promise.
+      if (fail) {
+        return fail(err);
       }
 
       done();
+    }
+
+    // call async function; the value of the user's "done" is the function below.
+    // in order to check the return value for a Promise, the processing of
+    // whatever the user called "done" with must be asynchronous.
+    var result = fn.call(ctx, function(err) {
+      // because Zalgo.
+      immediately(function() {
+        asyncDone(err);
+      });
     });
+
+    if (!finished && result && utils.isPromise(result)) {
+      fail = overspecified(result);
+      // wait for promise to resolve to fail; but note that this may
+      // never actually happen.
+      result.then(function() {
+        // discard any return value here
+        fail();
+      }, fail);
+    }
   }
 };

test/integration/fixtures/regression/issue-2407.js

@@ -0,0 +1,21 @@
+describe('async thing', function() {
+  it('should not just timeout if "done" never called (and report more info)', function(done) {
+    return new Promise(function(resolve, reject) {
+      reject('foo');
+    });
+  });
+
+  it('should not just timeout if "done" called but Promise never fulfilled', function(done) {
+    return new Promise(function(resolve) {
+      setTimeout(function() {
+        done(null, 'bar');
+      });
+    });
+  });
+
+  it('should not just timeout if "done" called but Promise never fulfilled (but is also synchronous)', function(done) {
+    return new Promise(function(resolve) {
+      done(null, 'bar');
+    });
+  });
+});

test/integration/regression.js

@@ -73,4 +73,28 @@ describe('regressions', function() {
       done();
     });
   });
+
+  it.only('issue-2407: should fail gracefully when overspecified but done() never called', function(done) {
+    if (!global.Promise) {
+      return this.skip();
+    }
+
+    this.timeout(2000);
+    runJSON('regression/issue-2407.js', [], function(err, res) {
+      assert(!err);
+      expect(res.tests[0].err).not.to.eql({});
+      expect(res.tests[0].err.message).not.to.match(/timeout/i);
+      expect(res.tests[0].err.message).to.match(/overspecified/i);
+      expect(res.tests[0].err.message).to.match(/additionally/i);
+      expect(res.tests[1].err).not.to.eql({});
+      expect(res.tests[1].err.message).not.to.match(/timeout/i);
+      expect(res.tests[1].err.message).to.match(/overspecified/i);
+      expect(res.tests[1].err.message).not.to.match(/additionally/i);
+      expect(res.tests[2].err).not.to.eql({});
+      expect(res.tests[2].err.message).not.to.match(/timeout/i);
+      expect(res.tests[2].err.message).to.match(/overspecified/i);
+      expect(res.tests[2].err.message).not.to.match(/additionally/i);
+      done();
+    });
+  });
 });