Merge pull request #1 from dx-pbuckley/timkinnane-hubot-async

Timkinnane hubot async

docs/scripting.md

@@ -743,6 +743,10 @@ Every middleware receives the same API signature of `context`, `next`, and
 `done`. Different kinds of middleware may receive different information in the
 `context` object. For more details, see the API for each type of middleware.
 
+Middleware execution returns a promise that resolves with the final `context`
+when the middleware stack completes. If the middleware stack throws an error,
+the promise will be rejected with the error and `context` at that point.
+
 ### Error Handling
 
 For synchronous middleware (never yields to the event loop), hubot will automatically catch errors and emit an an `error` event, just like in standard listeners. Hubot will also automatically call the most recent `done` callback to unwind the middleware stack. Asynchronous middleware should catch its own exceptions, emit an `error` event, and call `done`. Any uncaught exceptions will interrupt all execution of middleware completion callbacks.

package.json

@@ -1,6 +1,12 @@
 {
   "name": "hubot",
   "version": "0.0.0-development",
+  "standard": {
+     "env": [ "mocha" ]
+  },
+  "publishConfig": {
+    "tag": "next"
+  },
   "author": "hubot",
   "keywords": [
     "github",

src/middleware.js

@@ -22,43 +22,54 @@ class Middleware {
   // done() - Initial (final) completion callback. May be wrapped by
   //     executed middleware.
   //
-  // Returns nothing
+  // Returns promise - resolves with context when middleware completes
   // Returns before executing any middleware
   execute (context, next, done) {
-    const self = this
+    return new Promise((resolve, reject) => {
+      const self = this
 
-    if (done == null) {
-      done = function () {}
-    }
+      if (done == null) {
+        done = function () {}
+      }
 
-    // Execute a single piece of middleware and update the completion callback
-    // (each piece of middleware can wrap the 'done' callback with additional
-    // logic).
-    function executeSingleMiddleware (doneFunc, middlewareFunc, cb) {
-      // Match the async.reduce interface
-      function nextFunc (newDoneFunc) {
-        cb(null, newDoneFunc || doneFunc)
+      // Allow each middleware to resolve the promise early if it calls done()
+      const pieceDone = () => {
+        done()
+        resolve(context)
       }
 
-      // Catch errors in synchronous middleware
-      try {
-        middlewareFunc(context, nextFunc, doneFunc)
-      } catch (err) {
-        // Maintaining the existing error interface (Response object)
-        self.robot.emit('error', err, context.response)
-        // Forcibly fail the middleware and stop executing deeper
-        doneFunc()
+      // Execute a single piece of middleware and update the completion callback
+      // (each piece of middleware can wrap the 'done' callback with additional
+      // logic).
+      function executeSingleMiddleware (doneFunc, middlewareFunc, cb) {
+        // Match the async.reduce interface
+        function nextFunc (newDoneFunc) {
+          cb(null, newDoneFunc || doneFunc)
+        }
+
+        // Catch errors in synchronous middleware
+        try {
+          middlewareFunc(context, nextFunc, doneFunc)
+        } catch (err) {
+          // Maintaining the existing error interface (Response object)
+          self.robot.emit('error', err, context.response)
+          // Forcibly fail the middleware and stop executing deeper
+          doneFunc()
+          err.context = context
+          reject(err)
+        }
       }
-    }
 
-    // Executed when the middleware stack is finished
-    function allDone (_, finalDoneFunc) {
-      next(context, finalDoneFunc)
-    }
+      // Executed when the middleware stack is finished
+      function allDone (_, finalDoneFunc) {
+        next(context, finalDoneFunc)
+        resolve(context)
+      }
 
-    // Execute each piece of middleware, collecting the latest 'done' callback
-    // at each step.
-    process.nextTick(async.reduce.bind(null, this.stack, done, executeSingleMiddleware, allDone))
+      // Execute each piece of middleware, collecting the latest 'done' callback
+      // at each step.
+      process.nextTick(async.reduce.bind(null, this.stack, pieceDone, executeSingleMiddleware, allDone))
+    })
   }
 
   // Public: Registers new middleware

src/response.js

@@ -24,65 +24,65 @@ class Response {
   // strings - One or more strings to be posted. The order of these strings
   //           should be kept intact.
   //
-  // Returns nothing.
+  // Returns promise - resolves with context when middleware completes
   send (/* ...strings */) {
     const strings = [].slice.call(arguments)
-    this.runWithMiddleware.apply(this, ['send', { plaintext: true }].concat(strings))
+    return this.runWithMiddleware.apply(this, ['send', { plaintext: true }].concat(strings))
   }
 
   // Public: Posts an emote back to the chat source
   //
   // strings - One or more strings to be posted. The order of these strings
   //           should be kept intact.
   //
-  // Returns nothing.
+  // Returns promise - resolves with context when middleware completes
   emote (/* ...strings */) {
     const strings = [].slice.call(arguments)
-    this.runWithMiddleware.apply(this, ['emote', { plaintext: true }].concat(strings))
+    return this.runWithMiddleware.apply(this, ['emote', { plaintext: true }].concat(strings))
   }
 
   // Public: Posts a message mentioning the current user.
   //
   // strings - One or more strings to be posted. The order of these strings
   //           should be kept intact.
   //
-  // Returns nothing.
+  // Returns promise - resolves with context when middleware completes
   reply (/* ...strings */) {
     const strings = [].slice.call(arguments)
-    this.runWithMiddleware.apply(this, ['reply', { plaintext: true }].concat(strings))
+    return this.runWithMiddleware.apply(this, ['reply', { plaintext: true }].concat(strings))
   }
 
   // Public: Posts a topic changing message
   //
   // strings - One or more strings to set as the topic of the
   //           room the bot is in.
   //
-  // Returns nothing.
+  // Returns promise - resolves with context when middleware completes
   topic (/* ...strings */) {
     const strings = [].slice.call(arguments)
-    this.runWithMiddleware.apply(this, ['topic', { plaintext: true }].concat(strings))
+    return this.runWithMiddleware.apply(this, ['topic', { plaintext: true }].concat(strings))
   }
 
   // Public: Play a sound in the chat source
   //
   // strings - One or more strings to be posted as sounds to play. The order of
   //           these strings should be kept intact.
   //
-  // Returns nothing
+  // Returns promise - resolves with context when middleware completes
   play (/* ...strings */) {
     const strings = [].slice.call(arguments)
-    this.runWithMiddleware.apply(this, ['play'].concat(strings))
+    return this.runWithMiddleware.apply(this, ['play'].concat(strings))
   }
 
   // Public: Posts a message in an unlogged room
   //
   // strings - One or more strings to be posted. The order of these strings
   //           should be kept intact.
   //
-  // Returns nothing
+  // Returns promise - resolves with context when middleware completes
   locked (/* ...strings */) {
     const strings = [].slice.call(arguments)
-    this.runWithMiddleware.apply(this, ['locked', { plaintext: true }].concat(strings))
+    return this.runWithMiddleware.apply(this, ['locked', { plaintext: true }].concat(strings))
   }
 
   // Private: Call with a method for the given strings using response

src/robot.js

@@ -287,12 +287,12 @@ class Robot {
   //
   // cb - Optional callback that is called when message processing is complete
   //
-  // Returns nothing.
+  // Returns promise - resolves with context when middleware completes
   // Returns before executing callback
   receive (message, cb) {
     // When everything is finished (down the middleware stack and back up),
     // pass control back to the robot
-    this.middleware.receive.execute({ response: new Response(this, message) }, this.processListeners.bind(this), cb)
+    return this.middleware.receive.execute({ response: new Response(this, message) }, this.processListeners.bind(this), cb)
   }
 
   // Private: Passes the given message to any interested Listeners.

test/middleware_test.js

@@ -224,6 +224,70 @@ describe('Middleware', function () {
         )
       })
 
+      it('returns a promise that resolves when async middleware stack is complete', function (testDone) {
+        const testMiddlewareA = (context, next, done) => {
+          setTimeout(() => {
+            context.A = 'done'
+            next(done)
+          }, 50)
+        }
+
+        const testMiddlewareB = (context, next, done) => {
+          setTimeout(() => {
+            context.B = 'done'
+            next(done)
+          }, 50)
+        }
+
+        this.middleware.register(testMiddlewareA)
+        this.middleware.register(testMiddlewareB)
+
+        const middlewareFinished = () => {}
+
+        const middlewarePromise = this.middleware.execute(
+          {},
+          (_, done) => done(),
+          middlewareFinished
+        )
+
+        middlewarePromise.then((finalContext) => {
+          expect(finalContext).to.deep.equal({ A: 'done', B: 'done' })
+          testDone()
+        })
+      })
+
+      it('promise resolves when middleware completes early, with context at that point', function (testDone) {
+        const testMiddlewareA = (context, next, done) => {
+          setTimeout(() => {
+            context.A = 'done'
+            done()
+          }, 50)
+        }
+
+        const testMiddlewareB = (context, next, done) => {
+          setTimeout(() => {
+            context.B = 'done'
+            next(done)
+          }, 50)
+        }
+
+        this.middleware.register(testMiddlewareA)
+        this.middleware.register(testMiddlewareB)
+
+        const middlewareFinished = () => {}
+
+        const middlewarePromise = this.middleware.execute(
+          {},
+          (_, done) => done(),
+          middlewareFinished
+        )
+
+        middlewarePromise.then((finalContext) => {
+          expect(finalContext).to.deep.equal({ A: 'done' })
+          testDone()
+        })
+      })
+
       describe('error handling', function () {
         it('does not execute subsequent middleware after the error is thrown', function (testDone) {
           const middlewareExecution = []
@@ -258,7 +322,7 @@ describe('Middleware', function () {
             {},
             middlewareFinished,
             middlewareFailed
-          )
+          ).catch((reason) => {}) // supress unhandled promise rejection warning
         })
 
         it('emits an error event', function (testDone) {
@@ -287,7 +351,7 @@ describe('Middleware', function () {
             {response: testResponse},
             middlewareFinished,
             middlewareFailed
-          )
+          ).catch((reason) => {}) // supress unhandled promise rejection warning
         })
 
         it('unwinds the middleware stack (calling all done functions)', function (testDone) {
@@ -319,7 +383,7 @@ describe('Middleware', function () {
             {},
             middlewareFinished,
             middlewareFailed
-          )
+          ).catch((reason) => {}) // supress unhandled promise rejection warning
         })
       })
     })

test/response_test.js

@@ -0,0 +1,100 @@
+'use strict'
+
+// Assertions and Stubbing
+const chai = require('chai')
+const expect = chai.expect
+
+// Hubot classes
+const User = require('../src/user')
+const Robot = require('../src/robot')
+const TextMessage = require('../src/message').TextMessage
+const Response = require('../src/response')
+
+const asyncMiddleware = (context, next, done) => {
+  this.runOrder.push('middleware started')
+  setTimeout(() => {
+    this.runOrder.push('middleware finished')
+    next(done)
+  }, 100)
+}
+
+// mock `hubot-mock-adapter` module from fixture
+const mockery = require('mockery')
+
+describe('Response', function () {
+  describe('Unit Tests', function () {
+    beforeEach(function () {
+      // setup mock robot
+      this.user = new User({
+        id: 1,
+        name: 'hubottester',
+        room: '#mocha'
+      })
+      mockery.enable({
+        warnOnReplace: false,
+        warnOnUnregistered: false
+      })
+      mockery.registerMock('hubot-mock-adapter', require('./fixtures/mock-adapter'))
+      this.robot = new Robot(null, 'mock-adapter', true, 'TestHubot')
+      this.robot.alias = 'Hubot'
+      this.robot.run()
+
+      // async delayed middleware for promise return tests
+
+      // create a mock message and match
+      const message = new TextMessage(this.user, 'Hubot message123')
+      const regex = /(.*)/
+      const pattern = this.robot.respondPattern(regex)
+      const match = message.match(pattern)[1]
+
+      // generate response with mocks
+      this.res = new Response(this.robot, message, match)
+
+      // utility for tracking order of execution
+      this.runOrder = []
+
+      // sends don't send, just log
+      this.robot.send = x => this.runOrder.push(`sent ${x}`)
+    })
+
+    afterEach(function () {
+      this.robot.shutdown()
+    })
+
+    describe('#reply', function () {
+      context('with asynchronous middleware', function () {
+        beforeEach(function () {
+          this.robot.responseMiddleware((context, next, done) => asyncMiddleware.bind(this, context, next, done))
+        })
+
+        it('without using returned promise, .reply executes and continues before middleware completed', function () {
+          const _self = this
+
+          _self.runOrder.push('reply sending')
+          _self.res.reply('test')
+          _self.runOrder.push('reply finished')
+          expect(_self.runOrder).to.eql([
+            'reply sending',
+            'reply finished'
+          ])
+        })
+
+        it('using returned promise.then, .reply waits for middleware to complete before continueing', function () {
+          const _self = this
+
+          _self.runOrder.push('reply sending')
+          _self.res.reply('test')
+          .then(() => _self.runOrder.push('reply finished'))
+          .then(() => {
+            expect(_self.runOrder).to.eql([
+              'reply sending',
+              'middleware started',
+              'middleware finished',
+              'reply finished'
+            ])
+          })
+        })
+      })
+    })
+  })
+})