Merge pull request #152 from probot/error-handling

Catch and log errors from plugins

index.js

@@ -30,25 +30,22 @@ module.exports = options => {
   const server = createServer(webhook);
   const robot = createRobot({integration, webhook, cache, logger});
 
+  // Log all webhook errors
+  webhook.on('error', logger.error.bind(logger));
+
+  // Log all unhandled rejections
+  process.on('unhandledRejection', logger.error.bind(logger));
+
+  // If sentry is configured, report all logged errors
   if (process.env.SENTRY_URL) {
     Raven.disableConsoleAlerts();
     Raven.config(process.env.SENTRY_URL, {
-      captureUnhandledRejections: true,
       autoBreadcrumbs: true
     }).install({});
 
     logger.addStream(sentryStream(Raven));
-  } else {
-    process.on('unhandledRejection', reason => {
-      robot.log.error(reason);
-    });
   }
 
-  // Handle case when webhook creation fails
-  webhook.on('error', err => {
-    logger.error(err);
-  });
-
   return {
     server,
     robot,

lib/robot.js

@@ -49,8 +49,12 @@ class Robot {
 
     return this.webhook.on(name, async event => {
       if (!action || action === event.payload.action) {
-        const github = await this.auth(event.payload.installation.id);
-        return callback(event, new Context(event, github));
+        try {
+          const github = await this.auth(event.payload.installation.id);
+          return callback(event, new Context(event, github));
+        } catch (err) {
+          this.log.error(err);
+        }
       }
     });
   }

test/robot.js

@@ -2,10 +2,15 @@ const expect = require('expect');
 const Context = require('../lib/context');
 const createRobot = require('../lib/robot');
 
-const nullLogger = {};
-['trace', 'debug', 'info', 'warn', 'error', 'fatal'].forEach(level => {
-  nullLogger[level] = function () { };
-});
+function createNullLogger() {
+  const logger = expect.createSpy();
+
+  ['trace', 'debug', 'info', 'warn', 'error', 'fatal'].forEach(level => {
+    logger[level] = expect.createSpy();
+  });
+
+  return logger;
+}
 
 describe('Robot', function () {
   let webhook;
@@ -13,6 +18,7 @@ describe('Robot', function () {
   let event;
   let callbacks;
   let spy;
+  let logger;
 
   beforeEach(function () {
     callbacks = {};
@@ -21,11 +27,14 @@ describe('Robot', function () {
         callbacks[name] = callback;
       },
       emit: (name, event) => {
-        return callbacks[name](event);
+        if (callbacks[name]) {
+          return callbacks[name](event);
+        }
       }
     };
 
-    robot = createRobot({webhook, logger: nullLogger});
+    logger = createNullLogger();
+    robot = createRobot({webhook, logger});
     robot.auth = () => {};
 
     event = {
@@ -63,4 +72,18 @@ describe('Robot', function () {
       expect(spy).toNotHaveBeenCalled();
     });
   });
+
+  describe('error handling', () => {
+    it('logs errors throw from handlers', async () => {
+      const error = new Error('testing');
+
+      robot.on('test', () => {
+        throw error;
+      });
+
+      await webhook.emit('test', event);
+
+      expect(logger.error).toHaveBeenCalledWith(error);
+    });
+  });
 });