Merge pull request #6 from mbland/unhandled-promises

Updates for Node v6.6.0+ UnhandledPromiseRejectionWarning
mbland · Apr 28, 2017 · 22c3df2 · 22c3df2
2 parents 425bf03 + 12f3bda
commit 22c3df2
Show file tree

Hide file tree

Showing 11 changed files with 115 additions and 77 deletions.
diff --git a/_pages/components/middleware.md b/_pages/components/middleware.md
@@ -488,7 +488,8 @@ moving on to the next section.
 
 Now that `findMatchingRule` is in place, let's return to `execute` and begin
 to test it. As a base case, make sure that `execute` calls `next(done)` and
-returns if no rule matches:
+returns an empty rejected `Promise` if no rule matches, to signify that no
+action was taken and no error occurred:
 
 ```js
 Middleware.prototype.execute = function(context, next, done) {
@@ -497,7 +498,8 @@ Middleware.prototype.execute = function(context, next, done) {
       rule = this.findMatchingRule(message);
 
   if (!rule) {
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
   return 'not yet implemented';
 };
@@ -575,8 +577,10 @@ parse a message and file an issue` case:
 ```js
     it('should ignore messages that do not match', function() {
       delete context.response.message.rawMessage;
-      expect(middleware.execute(context, next, hubotDone)).to.be.undefined;
-      next.calledWith(hubotDone).should.be.true;
+      middleware.execute(context, next, hubotDone)
+        .should.be.rejectedWith(undefined).then(function() {;
+          next.calledWith(hubotDone).should.be.true;
+        });
     });
 ```
 
@@ -643,7 +647,8 @@ Middleware.prototype.execute = function(context, next, done) {
       msgId;
 
   if (!rule) {
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
 
   msgId = messageId(message);
@@ -1413,18 +1418,18 @@ should return `undefined`:
 
 ```js
     it('should not file another issue for the same message when ' +
-      'one is in progress', function(done) {
+      'one is in progress', function() {
       var result;
 
       result = middleware.execute(context, next, hubotDone);
-      if (middleware.execute(context, next, hubotDone) !== undefined) {
-        return done(new Error('middleware.execute did not prevent filing a ' +
-          'second issue when one was already in progress'));
-      }
-
-      result.should.become(helpers.ISSUE_URL).then(function() {
-        logger.info.args.should.contain(helpers.logArgs('already in progress'));
-      });
+      return middleware.execute(context, next, hubotDone)
+        .should.be.rejectedWith(undefined).then(function() {
+          return result.should.become(helpers.ISSUE_URL).then(function() {
+            logger.info.args.should.include.something.that.deep.equals(
+              helpers.logArgs('already in progress'));
+            next.calledWith(hubotDone).should.be.true;
+          });
+        });
     });
 ```
 
@@ -1493,7 +1498,8 @@ and add the message ID as a property inside `execute`:
   msgId = messageId(message);
   if (this.inProgress[msgId]) {
     this.logger(msgId, 'already in progress');
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
   this.inProgress[msgId] = true;
 ```
@@ -1583,16 +1589,17 @@ one more `middleware.execute` call _inside the callback_, and return the
 result of the assertion:
 
 ```js
-      result.should.become(helpers.ISSUE_URL).then(function() {
-        logger.info.args.should.include.something.that.deep.equals(
-          helpers.logArgs('already in progress'));
+          return result.should.become(helpers.ISSUE_URL).then(function() {
+            logger.info.args.should.include.something.that.deep.equals(
+              helpers.logArgs('already in progress'));
+            next.calledWith(hubotDone).should.be.true;
 
-        // Make another call to ensure that the ID is cleaned up. Normally the
-        // message will have a successReaction after the first successful
-        // request, but we'll test that in another case.
-        return middleware.execute(context, next, hubotDone)
-          .should.become(helpers.ISSUE_URL);
-      });
+            // Make another call to ensure that the ID is cleaned up. Normally
+            // the message will have a successReaction after the first
+            // successful request, but we'll test that in another case.
+            return middleware.execute(context, next, hubotDone)
+              .should.become(helpers.ISSUE_URL);
+          });
 ```
 
 Again, in frameworks other than Mocha that don't have native `Promise`
@@ -2086,7 +2093,8 @@ Middleware.prototype.execute = function(context, next, done) {
         JSON.stringify(context.response.message.rawMessage, null, 2);
     this.logger.error(null, errorMessage);
     context.response.reply(errorMessage);
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
 };
 ```

diff --git a/_pages/testing-component-integration.md b/_pages/testing-component-integration.md
@@ -81,7 +81,9 @@ module.exports = function(robot) {
         logger);
 
   robot.receiveMiddleware(function(context, next, done) {
-    impl.execute(context, next, done);
+    // Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
+    // in Node.js v6.6.0 and higher.
+    impl.execute(context, next, done).catch(function() { });
   });
   logger.info(null, 'registered receiveMiddleware');
 };
@@ -703,7 +705,9 @@ module.exports = function(robot) {
       logger);
 
     robot.receiveMiddleware(function(context, next, done) {
-      impl.execute(context, next, done);
+      // Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
+      // in Node.js v6.6.0 and higher.
+      impl.execute(context, next, done).catch(function() { });
     });
     logger.info(null, 'registered receiveMiddleware');
 
@@ -976,7 +980,9 @@ module.exports = function(robot) {
       logger);
 
     middleware = function(context, next, done) {
-      impl.execute(context, next, done);
+      // Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
+      // in Node.js v6.6.0 and higher.
+      impl.execute(context, next, done).catch(function() { });
     };
     middleware.impl = impl;
     robot.receiveMiddleware(middleware);

diff --git a/solutions/05-middleware/lib/middleware.js b/solutions/05-middleware/lib/middleware.js
@@ -28,7 +28,8 @@ Middleware.prototype.execute = function(context, next, done) {
         JSON.stringify(context.response.message.rawMessage, null, 2);
     this.logger.error(null, errorMessage);
     context.response.reply(errorMessage);
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
 };
 
@@ -40,13 +41,15 @@ function doExecute(middleware, context, next, done) {
       finish;
 
   if (!rule) {
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
 
   msgId = messageId(message);
   if (middleware.inProgress[msgId]) {
     middleware.logger.info(msgId, 'already in progress');
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
   middleware.inProgress[msgId] = true;
 

diff --git a/solutions/05-middleware/test/middleware-test.js b/solutions/05-middleware/test/middleware-test.js
@@ -142,29 +142,31 @@ describe('Middleware', function() {
 
     it('should ignore messages that do not match', function() {
       delete context.response.message.rawMessage;
-      expect(middleware.execute(context, next, hubotDone)).to.be.undefined;
-      next.calledWith(hubotDone).should.be.true;
+      middleware.execute(context, next, hubotDone)
+        .should.be.rejectedWith(undefined).then(function() {
+          next.calledWith(hubotDone).should.be.true;
+        });
     });
 
     it('should not file another issue for the same message when ' +
       'one is in progress', function() {
       var result;
 
       result = middleware.execute(context, next, hubotDone);
-      expect(middleware.execute(context, next, hubotDone)).to.eql(undefined,
-        'middleware.execute did not prevent filing a second issue ' +
-        'when one was already in progress');
-
-      return result.should.become(helpers.ISSUE_URL).then(function() {
-        logger.info.args.should.include.something.that.deep.equals(
-          helpers.logArgs('already in progress'));
-
-        // Make another call to ensure that the ID is cleaned up. Normally the
-        // message will have a successReaction after the first successful
-        // request, but we'll test that in another case.
-        return middleware.execute(context, next, hubotDone)
-          .should.become(helpers.ISSUE_URL);
-      });
+      return middleware.execute(context, next, hubotDone)
+        .should.be.rejectedWith(undefined).then(function() {
+          return result.should.become(helpers.ISSUE_URL).then(function() {
+            logger.info.args.should.include.something.that.deep.equals(
+              helpers.logArgs('already in progress'));
+            next.calledWith(hubotDone).should.be.true;
+
+            // Make another call to ensure that the ID is cleaned up. Normally
+            // the message will have a successReaction after the first
+            // successful request, but we'll test that in another case.
+            return middleware.execute(context, next, hubotDone)
+              .should.become(helpers.ISSUE_URL);
+          });
+        });
     });
 
     it('should not file another issue for the same message when ' +
@@ -250,7 +252,8 @@ describe('Middleware', function() {
             JSON.stringify(helpers.reactionAddedMessage(), null, 2);
 
       slackClient.getChannelName.throws();
-      expect(middleware.execute(context, next, hubotDone)).to.be.undefined;
+      middleware.execute(context, next, hubotDone)
+        .should.be.rejectedWith(undefined);
       next.calledWith(hubotDone).should.be.true;
       context.response.reply.args.should.eql([[errorMessage]]);
       logger.error.args.should.eql([[null, errorMessage]]);

diff --git a/solutions/06-integration/lib/middleware.js b/solutions/06-integration/lib/middleware.js
@@ -28,7 +28,8 @@ Middleware.prototype.execute = function(context, next, done) {
         JSON.stringify(context.response.message.rawMessage, null, 2);
     this.logger.error(null, errorMessage);
     context.response.reply(errorMessage);
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
 };
 
@@ -40,13 +41,15 @@ function doExecute(middleware, context, next, done) {
       finish;
 
   if (!rule) {
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
 
   msgId = messageId(message);
   if (middleware.inProgress[msgId]) {
     middleware.logger.info(msgId, 'already in progress');
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
   middleware.inProgress[msgId] = true;
 

diff --git a/solutions/06-integration/scripts/slack-github-issues.js b/solutions/06-integration/scripts/slack-github-issues.js
@@ -25,7 +25,9 @@ module.exports = function(robot) {
       logger);
 
     middleware = function(context, next, done) {
-      impl.execute(context, next, done);
+      // Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
+      // in Node.js v6.6.0 and higher.
+      impl.execute(context, next, done).catch(function() { });
     };
     middleware.impl = impl;
     robot.receiveMiddleware(middleware);

diff --git a/solutions/07-system/lib/middleware.js b/solutions/07-system/lib/middleware.js
@@ -28,7 +28,8 @@ Middleware.prototype.execute = function(context, next, done) {
         JSON.stringify(context.response.message.rawMessage, null, 2);
     this.logger.error(null, errorMessage);
     context.response.reply(errorMessage);
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
 };
 
@@ -40,13 +41,15 @@ function doExecute(middleware, context, next, done) {
       finish;
 
   if (!rule) {
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
 
   msgId = messageId(message);
   if (middleware.inProgress[msgId]) {
     middleware.logger.info(msgId, 'already in progress');
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
   middleware.inProgress[msgId] = true;
 

diff --git a/solutions/07-system/scripts/slack-github-issues.js b/solutions/07-system/scripts/slack-github-issues.js
@@ -25,7 +25,9 @@ module.exports = function(robot) {
       logger);
 
     middleware = function(context, next, done) {
-      impl.execute(context, next, done);
+      // Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
+      // in Node.js v6.6.0 and higher.
+      impl.execute(context, next, done).catch(function() { });
     };
     middleware.impl = impl;
     robot.receiveMiddleware(middleware);

diff --git a/solutions/complete/lib/middleware.js b/solutions/complete/lib/middleware.js
@@ -28,7 +28,8 @@ Middleware.prototype.execute = function(context, next, done) {
         JSON.stringify(context.response.message.rawMessage, null, 2);
     this.logger.error(null, errorMessage);
     context.response.reply(errorMessage);
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
 };
 
@@ -40,13 +41,15 @@ function doExecute(middleware, context, next, done) {
       finish;
 
   if (!rule) {
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
 
   msgId = messageId(message);
   if (middleware.inProgress[msgId]) {
     middleware.logger.info(msgId, 'already in progress');
-    return next(done);
+    next(done);
+    return Promise.reject();
   }
   middleware.inProgress[msgId] = true;
 

diff --git a/solutions/complete/scripts/slack-github-issues.js b/solutions/complete/scripts/slack-github-issues.js
@@ -25,7 +25,9 @@ module.exports = function(robot) {
       logger);
 
     middleware = function(context, next, done) {
-      impl.execute(context, next, done);
+      // Catch rejected Promises to avoid the UnhandledPromiseRejectionWarning
+      // in Node.js v6.6.0 and higher.
+      impl.execute(context, next, done).catch(function() { });
     };
     middleware.impl = impl;
     robot.receiveMiddleware(middleware);