Warn if a runable both takes a callback and returns a promise

lib/jasmine-core/jasmine.js

@@ -7056,7 +7056,8 @@ getJasmineRequireObj().QueueRunner = function(j$) {
       }),
       errored = false,
       queueableFn = self.queueableFns[iterativeIndex],
-      timeoutId;
+      timeoutId,
+      maybeThenable;
 
     next.fail = function nextFail() {
       self.fail.apply(null, arguments);
@@ -7084,7 +7085,7 @@ getJasmineRequireObj().QueueRunner = function(j$) {
 
     try {
       if (queueableFn.fn.length === 0) {
-        var maybeThenable = queueableFn.fn.call(self.userContext);
+        maybeThenable = queueableFn.fn.call(self.userContext);
 
         if (maybeThenable && j$.isFunction_(maybeThenable.then)) {
           maybeThenable.then(next, onPromiseRejection);
@@ -7092,7 +7093,8 @@ getJasmineRequireObj().QueueRunner = function(j$) {
           return { completedSynchronously: false };
         }
       } else {
-        queueableFn.fn.call(self.userContext, next);
+        maybeThenable = queueableFn.fn.call(self.userContext, next);
+        this.diagnoseConflictingAsync_(queueableFn.fn, maybeThenable);
         completedSynchronously = false;
         return { completedSynchronously: false };
       }
@@ -7145,6 +7147,25 @@ getJasmineRequireObj().QueueRunner = function(j$) {
     });
   };
 
+  QueueRunner.prototype.diagnoseConflictingAsync_ = function(fn, retval) {
+    if (retval && j$.isFunction_(retval.then)) {
+      // Issue a warning that matches the user's code
+      if (j$.isAsyncFunction_(fn)) {
+        this.deprecated('An asynchronous before/it/after ' +
+          'function was defined with the async keyword but also took a ' +
+          'done callback. This is not supported and will stop working in' +
+          ' the future. Either remove the done callback (recommended) or ' +
+          'remove the async keyword.');
+      } else {
+        this.deprecated('An asynchronous before/it/after ' +
+          'function took a done callback but also returned a promise. ' +
+          'This is not supported and will stop working in the future. ' +
+          'Either remove the done callback (recommended) or change the ' +
+          'function to not return a promise.');
+      }
+    }
+  };
+
   return QueueRunner;
 };
 

spec/core/QueueRunnerSpec.js

@@ -512,6 +512,48 @@ describe('QueueRunner', function() {
       expect(onExceptionCallback).toHaveBeenCalledWith('foo');
       expect(queueableFn2.fn).toHaveBeenCalled();
     });
+
+    it('issues a deprecation if the function also takes a parameter', function() {
+      var queueableFn = {
+          fn: function(done) {
+            return new StubPromise();
+          }
+        },
+        deprecated = jasmine.createSpy('deprecated'),
+        queueRunner = new jasmineUnderTest.QueueRunner({
+          queueableFns: [queueableFn],
+          deprecated: deprecated
+        }),
+        env = jasmineUnderTest.getEnv();
+
+      queueRunner.execute();
+
+      expect(deprecated).toHaveBeenCalledWith('An asynchronous ' +
+        'before/it/after function took a done callback but also returned a '+
+        'promise. This is not supported and will stop working in the future. ' +
+        'Either remove the done callback (recommended) or change the function ' +
+        'to not return a promise.'
+      );
+    });
+
+    it('issues a more specific deprecation if the function is `async`', function() {
+      jasmine.getEnv().requireAsyncAwait();
+      eval('var fn = async function(done){};');
+      var deprecated = jasmine.createSpy('deprecated'),
+        queueRunner = new jasmineUnderTest.QueueRunner({
+          queueableFns: [{fn: fn}],
+          deprecated: deprecated
+        });
+
+      queueRunner.execute();
+
+      expect(deprecated).toHaveBeenCalledWith('An asynchronous ' +
+        'before/it/after function was defined with the async keyword but ' +
+        'also took a done callback. This is not supported and will stop ' +
+        'working in the future. Either remove the done callback ' +
+        '(recommended) or remove the async keyword.'
+      );
+    });
   });
 
   it('passes the error instance to exception handlers in HTML browsers', function() {

src/core/QueueRunner.js

@@ -123,7 +123,8 @@ getJasmineRequireObj().QueueRunner = function(j$) {
       }),
       errored = false,
       queueableFn = self.queueableFns[iterativeIndex],
-      timeoutId;
+      timeoutId,
+      maybeThenable;
 
     next.fail = function nextFail() {
       self.fail.apply(null, arguments);
@@ -151,7 +152,7 @@ getJasmineRequireObj().QueueRunner = function(j$) {
 
     try {
       if (queueableFn.fn.length === 0) {
-        var maybeThenable = queueableFn.fn.call(self.userContext);
+        maybeThenable = queueableFn.fn.call(self.userContext);
 
         if (maybeThenable && j$.isFunction_(maybeThenable.then)) {
           maybeThenable.then(next, onPromiseRejection);
@@ -159,7 +160,8 @@ getJasmineRequireObj().QueueRunner = function(j$) {
           return { completedSynchronously: false };
         }
       } else {
-        queueableFn.fn.call(self.userContext, next);
+        maybeThenable = queueableFn.fn.call(self.userContext, next);
+        this.diagnoseConflictingAsync_(queueableFn.fn, maybeThenable);
         completedSynchronously = false;
         return { completedSynchronously: false };
       }
@@ -212,5 +214,24 @@ getJasmineRequireObj().QueueRunner = function(j$) {
     });
   };
 
+  QueueRunner.prototype.diagnoseConflictingAsync_ = function(fn, retval) {
+    if (retval && j$.isFunction_(retval.then)) {
+      // Issue a warning that matches the user's code
+      if (j$.isAsyncFunction_(fn)) {
+        this.deprecated('An asynchronous before/it/after ' +
+          'function was defined with the async keyword but also took a ' +
+          'done callback. This is not supported and will stop working in' +
+          ' the future. Either remove the done callback (recommended) or ' +
+          'remove the async keyword.');
+      } else {
+        this.deprecated('An asynchronous before/it/after ' +
+          'function took a done callback but also returned a promise. ' +
+          'This is not supported and will stop working in the future. ' +
+          'Either remove the done callback (recommended) or change the ' +
+          'function to not return a promise.');
+      }
+    }
+  };
+
   return QueueRunner;
 };