diff --git a/docs/scripting.md b/docs/scripting.md index 5cf92c773..6f6d82c9a 100644 --- a/docs/scripting.md +++ b/docs/scripting.md @@ -705,6 +705,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. diff --git a/package.json b/package.json index 4643c9dca..6fecb535a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,9 @@ { "name": "hubot", "version": "0.0.0-development", + "standard": { + "env": [ "mocha" ] + }, "publishConfig": { "tag": "next" }, @@ -23,6 +26,7 @@ "cline": "^0.8.2", "coffee-script": "1.6.3", "connect-multiparty": "^1.2.5", + "es6-promise": "^4.1.0", "express": "^3.21.2", "log": "1.4.0", "optparse": "1.0.4", diff --git a/src/middleware.js b/src/middleware.js index 3059706d7..952493fe8 100644 --- a/src/middleware.js +++ b/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 diff --git a/src/response.js b/src/response.js index 9656a6a08..d00cd9be8 100644 --- a/src/response.js +++ b/src/response.js @@ -24,10 +24,10 @@ 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 @@ -35,10 +35,10 @@ 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 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. @@ -46,10 +46,10 @@ 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 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 @@ -57,10 +57,10 @@ class Response { // 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 @@ -68,10 +68,10 @@ class Response { // 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 @@ -79,10 +79,10 @@ 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 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 diff --git a/src/robot.js b/src/robot.js index 36d266a17..aafd50155 100644 --- a/src/robot.js +++ b/src/robot.js @@ -285,12 +285,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. diff --git a/test/middleware_test.js b/test/middleware_test.js index 88045da2c..9910760f6 100644 --- a/test/middleware_test.js +++ b/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 }) }) }) diff --git a/test/response_test.js b/test/response_test.js new file mode 100644 index 000000000..0b5ea2e26 --- /dev/null +++ b/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' + ]) + }) + }) + }) + }) + }) +})