[appveyor] Error handling in BaseService (#1590)

Make a clear distinction between programmer errors ("internal errors") and runtime errors, and allow configuring the server to let the programmer errors bubble up in development and unit testing. This saves a huge amount of time because it generates ordinary stack traces when things go wrong. And, if these errors occur in production, we'll catch them, and display **shields | internal error** which is the equivalent of a 500 error.

services/appveyor/appveyor.js

@@ -1,26 +1,23 @@
 'use strict';
 
 const BaseService = require('../base');
+const {
+  checkErrorResponse,
+  asJson,
+} = require('../../lib/error-helper');
 
-/**
- * AppVeyor CI integration.
- */
 module.exports = class AppVeyor extends BaseService {
   async handle({repo, branch}) {
     let apiUrl = 'https://ci.appveyor.com/api/projects/' + repo;
     if (branch != null) {
       apiUrl += '/branch/' + branch;
     }
-    const {buffer, res} = await this._sendAndCacheRequest(apiUrl, {
+    const json = await this._sendAndCacheRequest(apiUrl, {
       headers: { 'Accept': 'application/json' }
-    });
+    }).then(checkErrorResponse.asPromise({ notFoundMessage: 'project not found or access denied' }))
+      .then(asJson);
 
-    if (res.statusCode === 404) {
-      return {message: 'project not found or access denied'};
-    }
-
-    const data = JSON.parse(buffer);
-    const status = data.build.status;
+    const { build: { status } } = json;
     if (status === 'success') {
       return {message: 'passing', color: 'brightgreen'};
     } else if (status !== 'running' && status !== 'queued') {

services/appveyor/appveyor.tester.js

@@ -25,6 +25,11 @@ t.create('CI 404')
 .get('/ci/somerandomproject/thatdoesntexits.json')
   .expectJSON({ name: 'build', value: 'project not found or access denied' });
 
+t.create('CI (connection error)')
+  .get('/ci/this-one/is-not-real-either.json')
+  .networkOff()
+  .expectJSON({ name: 'build', value: 'inaccessible' });
+
 // Test AppVeyor tests status badge
 t.create('tests status')
   .get('/tests/NZSmartie/coap-net-iu0to.json')

services/base.js

@@ -1,5 +1,10 @@
 'use strict';
 
+const {
+  NotFound,
+  InvalidResponse,
+  Inaccessible,
+} = require('./errors');
 const {
   makeLogo,
   toArray,
@@ -8,8 +13,9 @@ const {
 } = require('../lib/badge-data');
 
 module.exports = class BaseService {
-  constructor({sendAndCacheRequest}) {
+  constructor({ sendAndCacheRequest }, { handleInternalErrors }) {
     this._sendAndCacheRequest = sendAndCacheRequest;
+    this._handleInternalErrors = handleInternalErrors;
   }
 
   /**
@@ -32,6 +38,7 @@ module.exports = class BaseService {
   static get category() {
     return 'unknown';
   }
+
   /**
    * Returns an object:
    *  - base: (Optional) The base path of the URLs for this service. This is
@@ -95,8 +102,27 @@ module.exports = class BaseService {
     try {
       return await this.handle(namedParams);
     } catch (error) {
-      console.log(error);
-      return { message: 'error' };
+      if (error instanceof NotFound) {
+        return {
+          message: error.prettyMessage,
+          color: 'red',
+        };
+      } else if (error instanceof InvalidResponse ||
+        error instanceof Inaccessible) {
+        return {
+          message: error.prettyMessage,
+          color: 'lightgray',
+        };
+      } else if (this._handleInternalErrors) {
+        console.log(error);
+        return {
+          label: 'shields',
+          message: 'internal error',
+          color: 'lightgray',
+        };
+      } else {
+        throw error;
+      }
     }
   }
 
@@ -118,15 +144,15 @@ module.exports = class BaseService {
       link: serviceLink,
     } = serviceData;
 
-    const defaultLabel = this.category;
     const {
       color: defaultColor,
       logo: defaultLogo,
+      label: defaultLabel,
     } = this.defaultBadgeData;
 
     const badgeData = {
       text: [
-        overrideLabel || serviceLabel || defaultLabel,
+        overrideLabel || serviceLabel || defaultLabel || this.category,
         serviceMessage || 'n/a',
       ],
       template: style,
@@ -141,7 +167,7 @@ module.exports = class BaseService {
     return badgeData;
   }
 
-  static register(camp, handleRequest) {
+  static register(camp, handleRequest, { handleInternalErrors }) {
     const ServiceClass = this; // In a static context, "this" is the class.
 
     camp.route(this._regex,
@@ -149,7 +175,7 @@ module.exports = class BaseService {
       const namedParams = this._namedParamsForMatch(match);
       const serviceInstance = new ServiceClass({
         sendAndCacheRequest: request.asPromise,
-      });
+      }, { handleInternalErrors });
       const serviceData = await serviceInstance.invokeHandler(namedParams);
       const badgeData = this._makeBadgeData(queryParams, serviceData);
 

services/base.spec.js

@@ -26,6 +26,8 @@ class DummyService extends BaseService {
 }
 
 describe('BaseService', () => {
+  const defaultConfig = { handleInternalErrors: false };
+
   describe('URL pattern matching', function () {
     const regexExec = str => DummyService._regex.exec(str);
     const getSomeArg = str => {
@@ -66,17 +68,21 @@ describe('BaseService', () => {
   });
 
   it('Invokes the handler as expected', async function () {
-    const serviceInstance = new DummyService({});
+    const serviceInstance = new DummyService({}, defaultConfig);
     const serviceData = await serviceInstance.invokeHandler({ someArg: 'bar.bar.bar' });
     expect(serviceData).to.deep.equal({ message: 'Hello bar.bar.bar' });
   });
 
   describe('Error handling', function () {
     it('Handles internal errors', async function () {
-      const serviceInstance = new DummyService({});
+      const serviceInstance = new DummyService({}, { handleInternalErrors: true });
       serviceInstance.handle = () => { throw Error("I've made a huge mistake"); };
       const serviceData = await serviceInstance.invokeHandler({ someArg: 'bar.bar.bar' });
-      expect(serviceData).to.deep.equal({ message: 'error' });
+      expect(serviceData).to.deep.equal({
+        color: 'lightgray',
+        label: 'shields',
+        message: 'internal error',
+      });
     });
   });
 
@@ -129,7 +135,7 @@ describe('BaseService', () => {
         route: sinon.spy(),
       };
       mockHandleRequest = sinon.spy();
-      DummyService.register(mockCamp, mockHandleRequest);
+      DummyService.register(mockCamp, mockHandleRequest, defaultConfig);
     });
 
     it('registers the service', () => {

services/errors.js

@@ -0,0 +1,39 @@
+'use strict';
+
+class NotFound extends Error {
+  constructor(prettyMessage = 'not found') {
+    const message = prettyMessage === 'not found'
+      ? 'Not Found'
+      : `Not Found: ${prettyMessage}`;
+    super(message);
+    this.prettyMessage = prettyMessage;
+    this.name = 'NotFound';
+  }
+}
+
+class InvalidResponse extends Error {
+  constructor(prettyMessage = 'invalid', underlyingError) {
+    const message = underlyingError
+      ? `Invalid Response: ${underlyingError.message}`
+      : 'Invalid Response';
+    super(message);
+    this.stack = underlyingError.stack;
+    this.prettyMessage = prettyMessage;
+    this.name = 'InvalidResponse';
+  }
+}
+
+class Inaccessible extends Error {
+  constructor(underlyingError, prettyMessage = 'inaccessible') {
+    super(`Inaccessible: ${underlyingError.message}`);
+    this.stack = underlyingError.stack;
+    this.prettyMessage = prettyMessage;
+    this.name = 'Inaccessible';
+  }
+}
+
+module.exports = {
+  NotFound,
+  InvalidResponse,
+  Inaccessible,
+};