Return a Promise from report method (#50)

* Return a Promise from report method

Remove the callback argument from StackdriverErrorReporter report()
and sendErrorPayload() and instead return a Promise that resolves
once both the stacktrace is generated and the XHR request completes.

Update tests to return promises to the framework rather than use the
done() callback where possible.

Suppress promise rejections in for top level error handlers to avoid
potential loops.

* Add new tests for XHR errors

These reflect current behaviour and should be updated when the
interface is more clearly defined.

Author: bz2

stackdriver-errors.js

@@ -64,24 +64,25 @@
     this.disabled = config.disabled || false;
 
     // Register as global error handler if requested
+    var noop = function() {};
     var that = this;
     if(this.reportUncaughtExceptions) {
-      var oldErrorHandler = window.onerror || function(){};
+      var oldErrorHandler = window.onerror || noop;
 
       window.onerror = function(message, source, lineno, colno, error) {
         if(error){
-          that.report(error);
+          that.report(error).catch(noop);
         }
         oldErrorHandler(message, source, lineno, colno, error);
         return true;
       };
     }
     if(this.reportUnhandledPromiseRejections) {
-      var oldPromiseRejectionHandler = window.onunhandledrejection || function(){};
+      var oldPromiseRejectionHandler = window.onunhandledrejection || noop;
 
       window.onunhandledrejection = function(promiseRejectionEvent) {
         if(promiseRejectionEvent){
-          that.report(promiseRejectionEvent.reason);
+          that.report(promiseRejectionEvent.reason).catch(noop);
         }
         oldPromiseRejectionHandler(promiseRejectionEvent.reason);
         return true;
@@ -92,14 +93,14 @@
   /**
    * Report an error to the Stackdriver Error Reporting API
    * @param {Error|String} err - The Error object or message string to report.
-   * @param callback - Calback function to be called once error has been reported.
+   * @returns {Promise} A promise that completes when the report has been sent.
    */
-  StackdriverErrorReporter.prototype.report = function(err, callback) {
+  StackdriverErrorReporter.prototype.report = function(err) {
     if(this.disabled) {
-      return typeof callback === 'function' && callback();
+      return Promise.resolve(null);
     }
     if(!err) {
-      return typeof callback === 'function' && callback('no error to report');
+      return Promise.reject(new Error('no error to report'));
     }
 
     var payload = {};
@@ -123,7 +124,7 @@
     }
     var that = this;
     // This will use sourcemaps and normalize the stack frames
-    StackTrace.fromError(err).then(function(stack){
+    return StackTrace.fromError(err).then(function(stack){
       payload.message = err.toString();
       for(var s = firstFrameIndex; s < stack.length; s++) {
         payload.message += '\n';
@@ -133,32 +134,31 @@
         // If functionName or methodName isn't available <anonymous> will be used as the name.
         payload.message += ['    at ', stack[s].getFunctionName() || '<anonymous>', ' (', stack[s].getFileName(), ':', stack[s].getLineNumber() ,':', stack[s].getColumnNumber() , ')'].join('');
       }
-      that.sendErrorPayload(payload, callback);
+      return that.sendErrorPayload(payload);
     }, function(reason) {
       // Failure to extract stacktrace
       payload.message = [
         'Error extracting stack trace: ', reason, '\n',
         err.toString(), '\n',
         '    (', err.file, ':', err.line, ':', err.column, ')',
       ].join('');
-      that.sendErrorPayload(payload, callback);
+      return that.sendErrorPayload(payload);
     });
   };
 
-  StackdriverErrorReporter.prototype.sendErrorPayload = function(payload, callback) {
+  StackdriverErrorReporter.prototype.sendErrorPayload = function(payload) {
     var defaultUrl = baseAPIUrl + this.projectId + "/events:report?key=" + this.apiKey;
     var url = this.targetUrl || defaultUrl;
 
     var xhr = new XMLHttpRequest();
     xhr.open('POST', url, true);
     xhr.setRequestHeader('Content-Type', 'application/json; charset=UTF-8');
-    xhr.onloadend = function() {
-      return typeof callback === 'function' && callback();
-    };
-    xhr.onerror = function(e) {
-      return typeof callback === 'function' && callback(e);
-    };
-    xhr.send(JSON.stringify(payload));
+
+    return new Promise(function(resolve, reject) {
+      xhr.onloadend = resolve;
+      xhr.onerror = reject;
+      xhr.send(JSON.stringify(payload));
+    });
   };
 
   /**

test/test.js

@@ -16,7 +16,7 @@
 var expect = chai.expect;
 
 var errorHandler;
-var xhr, requests;
+var xhr, requests, requestHandler;
 var WAIT_FOR_STACKTRACE_FROMERROR = 15;
 
 /** 
@@ -49,13 +49,16 @@ beforeEach(function() {
   });
 
   requests = [];
+  requestHandler = function (req) {
+    req.respond(200, {"Content-Type": "application/json"}, '{}');
+  }
   xhr.onCreate = function (req) {
     // Allow `onCreate` to complete so `xhr` can finish instantiating.
     setTimeout(function(){
       if(req.url.match('clouderrorreporting')) {
         requests.push(req);
       }
-      req.respond(200, {"Content-Type": "application/json"}, '{}');
+      requestHandler(req);
     }, 1);
   };
 });
@@ -109,12 +112,11 @@ describe('Initialization', function () {
 
 describe('Disabling', function () {
 
- it('should not report errors if disabled', function (done) {
+ it('should not report errors if disabled', function () {
    errorHandler.start({key:'key', projectId:'projectId', disabled: true});
-    errorHandler.report('do not report', function() {
-      expect(requests.length).to.equal(0);
-      done();
-    });
+   return errorHandler.report('do not report').then(function() {
+     expect(requests.length).to.equal(0);
+   });
  });
 
 });
@@ -125,67 +127,88 @@ describe('Reporting errors', function () {
       errorHandler.start({key:'key', projectId:'projectId'});
     });
 
-    it('should report error messages with location', function (done) {
+    it('should report error messages with location', function () {
       var message = 'Something broke!';
-      errorHandler.report(message, function() {
+      return errorHandler.report(message).then(function() {
         expectRequestWithMessage(message);
-        done();
       });
     });
 
-    it('should extract and send stack traces from Errors', function (done) {
+    it('should extract and send stack traces from Errors', function () {
       var message = 'custom message';
       // PhantomJS only attaches a stack to thrown errors
       try {
         throw new TypeError(message);
       } catch(e) {
-        errorHandler.report(e, function() {
+        return errorHandler.report(e).then(function() {
           expectRequestWithMessage(message);
-          done();
         });
       }
     });
 
-    it('should extract and send functionName in stack traces', function (done) {
+    it('should extract and send functionName in stack traces', function () {
       var message = 'custom message';
       // PhantomJS only attaches a stack to thrown errors
       try {
         throwError(message)
       } catch(e) {
-        errorHandler.report(e, function() {
+        return errorHandler.report(e).then(function() {
           expectRequestWithMessage('throwError');
-          done();
         });
       }
     });
 
-    it('should set in stack traces when frame is anonymous', function (done) {
+    it('should set in stack traces when frame is anonymous', function () {
       var message = 'custom message';
       // PhantomJS only attaches a stack to thrown errors
       try {
         (function () {
           throw new TypeError(message);
         })()
       } catch(e) {
-        errorHandler.report(e, function() {
+        return errorHandler.report(e).then(function() {
           expectRequestWithMessage('<anonymous>');
-          done();
         });
       }
     });
+
+    describe('XHR error handling', function() {
+      it('should handle network error', function () {
+        requestHandler = function (req) {
+          req.error();
+        };
+        var message = 'News that will fail to send';
+        return errorHandler.report(message).then(function() {
+          throw new Error('unexpected fulfilled report');
+        }, function(e) {
+          expectRequestWithMessage(message);
+          // TODO: Expose a tidied up error object
+          expect(e.target.status).to.equal(0);
+        });
+      });
+
+      it('should handle http error', function () {
+        requestHandler = function (req) {
+          req.respond(503, {"Content-Type": "text/plain"}, '');
+        };
+        errorHandler.start({key:'key', projectId:'projectId'});
+        var message = 'News that was rejected on send';
+        return errorHandler.report(message).then(function() {
+          expectRequestWithMessage(message);
+        });
+      });
+    });
   });
 
   describe('Custom target url configuration', function() {
-    it('should report error messages with custom url config', function (done) {
+    it('should report error messages with custom url config', function () {
       var targetUrl = 'config-uri-clouderrorreporting';
       errorHandler.start({targetUrl:targetUrl});
 
       var message = 'Something broke!';
-      errorHandler.report(message, function() {
+      return errorHandler.report(message).then(function() {
         expectRequestWithMessage(message);
         expect(requests[0].url).to.equal(targetUrl);
-
-        done();
       });
     });
   });