From 29fba0f08eb8607af02d095e63b49415896c7116 Mon Sep 17 00:00:00 2001 From: timkinnane Date: Mon, 22 May 2017 15:24:42 +1000 Subject: [PATCH 1/8] feat(middleware): Return a promise that resolves when middleware stack done --- src/middleware.coffee | 20 ++++++++++++-------- test/middleware_test.coffee | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/src/middleware.coffee b/src/middleware.coffee index 7f9d16e39..a1aa1ba40 100644 --- a/src/middleware.coffee +++ b/src/middleware.coffee @@ -39,14 +39,18 @@ class Middleware @robot.emit('error', err, context.response) # Forcibly fail the middleware and stop executing deeper doneFunc() - - # Executed when the middleware stack is finished - allDone = (_, finalDoneFunc) -> next(context, finalDoneFunc) - - # Execute each piece of middleware, collecting the latest 'done' callback - # at each step. - process.nextTick => - async.reduce(@stack, done, executeSingleMiddleware, allDone) + + new Promise (resolve, reject) => + + # Executed when the middleware stack is finished + allDone = (_, finalDoneFunc) -> + next context, finalDoneFunc + resolve context + + # Execute each piece of middleware, collecting the latest 'done' callback + # at each step. + process.nextTick => + async.reduce(@stack, done, executeSingleMiddleware, allDone) # Public: Registers new middleware # diff --git a/test/middleware_test.coffee b/test/middleware_test.coffee index 4c56c142a..7ad073f18 100644 --- a/test/middleware_test.coffee +++ b/test/middleware_test.coffee @@ -208,6 +208,40 @@ describe 'Middleware', -> allDone ) + it 'returns a promise that resolves when async middleware stack is complete', (testDone) -> + + clock = sinon.useFakeTimers() + + testMiddlewareA = (context, next, done) => + setTimeout -> + context.A = 'done' + next(done) + , 250 + + testMiddlewareB = (context, next, done) -> + setTimeout -> + context.B = 'done' + next(done) + , 250 + + @middleware.register testMiddlewareA + @middleware.register testMiddlewareB + + middlewareFinished = -> clock.restore() + + middlewarePromise = @middleware.execute( + {} + (_, done) -> done() + middlewareFinished + ) + + middlewarePromise.then (finalContext) -> + expect(finalContext).to.deep.equal A: 'done', B: 'done' + testDone() + + clock.tick 600 + clock.restore() + describe 'error handling', -> it 'does not execute subsequent middleware after the error is thrown', (testDone) -> middlewareExecution = [] From a0cc04e79b834bc8ba48cbf1df60c1e9572b32ee Mon Sep 17 00:00:00 2001 From: timkinnane Date: Tue, 23 May 2017 10:51:57 +1000 Subject: [PATCH 2/8] fix(middleware): Add es6-promise to polyfill node promise support Builds in node 0.10.x were failing due to lack of promise support 1339 --- package.json | 1 + src/middleware.coffee | 1 + 2 files changed, 2 insertions(+) diff --git a/package.json b/package.json index 8c187e179..752e674b6 100644 --- a/package.json +++ b/package.json @@ -20,6 +20,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.coffee b/src/middleware.coffee index a1aa1ba40..ae114c7c2 100644 --- a/src/middleware.coffee +++ b/src/middleware.coffee @@ -1,4 +1,5 @@ async = require 'async' +Promise = global.Promise || require('es6-promise').Promise class Middleware # We use this recursively, and using nextTick recursively is deprecated in node 0.10. From 280627882c09079a530386211f377cb886d821a3 Mon Sep 17 00:00:00 2001 From: timkinnane Date: Tue, 23 May 2017 11:48:04 +1000 Subject: [PATCH 3/8] refactor(middleware): Use recommended es6-promise polyfill method --- src/middleware.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware.coffee b/src/middleware.coffee index ae114c7c2..2ee4f428d 100644 --- a/src/middleware.coffee +++ b/src/middleware.coffee @@ -1,5 +1,5 @@ async = require 'async' -Promise = global.Promise || require('es6-promise').Promise +require('es6-promise').polyfill() class Middleware # We use this recursively, and using nextTick recursively is deprecated in node 0.10. From 12bdbfadf0ba4e8f7d7a2b7aac43a36c24d0d4fb Mon Sep 17 00:00:00 2001 From: timkinnane Date: Tue, 23 May 2017 11:48:46 +1000 Subject: [PATCH 4/8] docs(middleware): Update with promise return info --- docs/scripting.md | 4 ++++ src/middleware.coffee | 2 +- src/response.coffee | 2 +- src/robot.coffee | 2 +- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/scripting.md b/docs/scripting.md index acabfb5b5..ba0a65c5a 100644 --- a/docs/scripting.md +++ b/docs/scripting.md @@ -700,6 +700,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/src/middleware.coffee b/src/middleware.coffee index 2ee4f428d..4f1a12551 100644 --- a/src/middleware.coffee +++ b/src/middleware.coffee @@ -22,7 +22,7 @@ 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) -> done ?= -> diff --git a/src/response.coffee b/src/response.coffee index 504ee382a..1e0b64c05 100644 --- a/src/response.coffee +++ b/src/response.coffee @@ -62,7 +62,7 @@ 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...) -> @runWithMiddleware("locked", { plaintext: true }, strings...) diff --git a/src/robot.coffee b/src/robot.coffee index 0233c7236..deeacba66 100644 --- a/src/robot.coffee +++ b/src/robot.coffee @@ -289,7 +289,7 @@ 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), From 944f292a93b4d4039d75f2786559cc08bf30a5cf Mon Sep 17 00:00:00 2001 From: timkinnane Date: Tue, 23 May 2017 11:50:17 +1000 Subject: [PATCH 5/8] fix(middleware): Wrap all execute logic in promise Allows rejecting if middleware throws related #1339 --- src/middleware.coffee | 36 +++++++++++++------------ test/middleware_test.coffee | 52 +++++++++++++++++++++++++++---------- 2 files changed, 57 insertions(+), 31 deletions(-) diff --git a/src/middleware.coffee b/src/middleware.coffee index 4f1a12551..b9fbb6ff9 100644 --- a/src/middleware.coffee +++ b/src/middleware.coffee @@ -25,24 +25,26 @@ class Middleware # Returns promise - resolves with context when middleware completes # Returns before executing any middleware execute: (context, next, done) -> - done ?= -> - # Execute a single piece of middleware and update the completion callback - # (each piece of middleware can wrap the 'done' callback with additional - # logic). - executeSingleMiddleware = (doneFunc, middlewareFunc, cb) => - # Match the async.reduce interface - nextFunc = (newDoneFunc) -> cb(null, newDoneFunc or doneFunc) - # Catch errors in synchronous middleware - try - middlewareFunc.call(undefined, context, nextFunc, doneFunc) - catch err - # Maintaining the existing error interface (Response object) - @robot.emit('error', err, context.response) - # Forcibly fail the middleware and stop executing deeper - doneFunc() - + new Promise (resolve, reject) => - + + done ?= -> + # Execute a single piece of middleware and update the completion callback + # (each piece of middleware can wrap the 'done' callback with additional + # logic). + executeSingleMiddleware = (doneFunc, middlewareFunc, cb) => + # Match the async.reduce interface + nextFunc = (newDoneFunc) -> cb(null, newDoneFunc or doneFunc) + # Catch errors in synchronous middleware + try + middlewareFunc.call(undefined, context, nextFunc, doneFunc) + catch err + # Maintaining the existing error interface (Response object) + @robot.emit('error', err, context.response) + # Forcibly fail the middleware and stop executing deeper + doneFunc() + reject err, context + # Executed when the middleware stack is finished allDone = (_, finalDoneFunc) -> next context, finalDoneFunc diff --git a/test/middleware_test.coffee b/test/middleware_test.coffee index 7ad073f18..d386601e7 100644 --- a/test/middleware_test.coffee +++ b/test/middleware_test.coffee @@ -210,24 +210,22 @@ describe 'Middleware', -> it 'returns a promise that resolves when async middleware stack is complete', (testDone) -> - clock = sinon.useFakeTimers() - - testMiddlewareA = (context, next, done) => - setTimeout -> + testMiddlewareA = (context, next, done) -> + setTimeout -> context.A = 'done' next(done) - , 250 + , 50 testMiddlewareB = (context, next, done) -> - setTimeout -> + setTimeout -> context.B = 'done' next(done) - , 250 + , 50 @middleware.register testMiddlewareA @middleware.register testMiddlewareB - middlewareFinished = -> clock.restore() + middlewareFinished = -> middlewarePromise = @middleware.execute( {} @@ -236,11 +234,37 @@ describe 'Middleware', -> ) middlewarePromise.then (finalContext) -> - expect(finalContext).to.deep.equal A: 'done', B: 'done' + expect(finalContext).to.eql A: 'done', B: 'done' testDone() + + it 'promise resolves when middleware completes early, with context at that point', (testDone) -> + + testMiddlewareA = (context, next, done) -> + setTimeout -> + context.A = 'done' + done() + , 50 + + testMiddlewareB = (context, next, done) -> + setTimeout -> + context.B = 'done' + next(done) + , 50 + + @middleware.register testMiddlewareA + @middleware.register testMiddlewareB + + middlewareFinished = -> + + middlewarePromise = @middleware.execute( + {} + (_, done) -> done() + middlewareFinished + ) - clock.tick 600 - clock.restore() + middlewarePromise.then (finalContext) -> + expect(finalContext).to.eql A: 'done' + testDone() describe 'error handling', -> it 'does not execute subsequent middleware after the error is thrown', (testDone) -> @@ -272,7 +296,7 @@ describe 'Middleware', -> {} middlewareFinished middlewareFailed - ) + ).catch (reason) -> # supress warning re unhandled promise rejection it 'emits an error event', (testDone) -> testResponse = {} @@ -297,7 +321,7 @@ describe 'Middleware', -> {response: testResponse}, middlewareFinished, middlewareFailed - ) + ).catch (reason) -> # supress warning re unhandled promise rejection it 'unwinds the middleware stack (calling all done functions)', (testDone) -> extraDoneFunc = null @@ -325,7 +349,7 @@ describe 'Middleware', -> {} middlewareFinished middlewareFailed - ) + ).catch (reason) -> # supress warning re unhandled promise rejection describe '#register', -> it 'adds to the list of middleware', -> From d92385e5c20d2e462ba084f05b2361fad3c317ff Mon Sep 17 00:00:00 2001 From: timkinnane Date: Wed, 31 May 2017 10:33:32 +1000 Subject: [PATCH 6/8] fix(middleware): Allow middleware to resolve early --- src/middleware.coffee | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/middleware.coffee b/src/middleware.coffee index b9fbb6ff9..c97b7bdcc 100644 --- a/src/middleware.coffee +++ b/src/middleware.coffee @@ -29,6 +29,12 @@ class Middleware new Promise (resolve, reject) => done ?= -> + + # Allow each middleware to resolve the promise early if it calls done() + pieceDone = -> + done() + resolve context + # Execute a single piece of middleware and update the completion callback # (each piece of middleware can wrap the 'done' callback with additional # logic). @@ -53,7 +59,7 @@ class Middleware # Execute each piece of middleware, collecting the latest 'done' callback # at each step. process.nextTick => - async.reduce(@stack, done, executeSingleMiddleware, allDone) + async.reduce(@stack, pieceDone, executeSingleMiddleware, allDone) # Public: Registers new middleware # From 903615f088a824379752ede6d697afe920a8fc6b Mon Sep 17 00:00:00 2001 From: timkinnane Date: Fri, 14 Jul 2017 15:42:20 +1000 Subject: [PATCH 7/8] build(standardjs): Exclude mocha globals from standardjs definition check --- package.json | 3 +++ 1 file changed, 3 insertions(+) diff --git a/package.json b/package.json index 1d54a5c44..afa75949c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,9 @@ { "name": "hubot", "version": "0.0.0-development", + "standard": { + "env": [ "mocha" ] + }, "publishConfig": { "tag": "next" }, From 58d3de5b69698a27f33302b99a1008451714ce56 Mon Sep 17 00:00:00 2001 From: timkinnane Date: Fri, 14 Jul 2017 15:43:16 +1000 Subject: [PATCH 8/8] test(response): Add unit tests for response method promise features --- test/response_test.js | 100 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 test/response_test.js 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' + ]) + }) + }) + }) + }) + }) +})