Repository: jsperf/jsperf.com (js)

Issue:
“Unexpected response from browserscope.org” shouldn’t prevent new test creation
Whenever the “Unexpected response from browserscope.org” occurs, jsPerf should still create the test, just without a Browserscope ID.

When the test page is accessed we can re-attempt to get a Browserscope ID for this test.

Pull request:
log errors from browserscope instead of rejecting
fixes #257 

Patch:
diff --git a/server/repositories/browserscope.js b/server/repositories/browserscope.js
index b880fbe..80a8406 100644
--- a/server/repositories/browserscope.js
+++ b/server/repositories/browserscope.js
@@ -22,10 +22,14 @@ exports.register = function (server, options, next) {
           try {
             resolve(JSON.parse(str).test_key);
           } catch (e) {
-            reject(new Error('Unexpected response from browserscope.org'));
+            server.log('error', e);
+            resolve();
           }
         });
-      }).on('error', reject);
+      }).on('error', function (e) {
+        server.log('error', e);
+        resolve();
+      });
     });
   });
 
diff --git a/server/services/pages.js b/server/services/pages.js
index 2ea4e18..a65925b 100644
--- a/server/services/pages.js
+++ b/server/services/pages.js
@@ -34,7 +34,9 @@ exports.register = function (server, options, next) {
     return browserscopeRepo.addTest(payload.title, payload.info, payload.slug)
       .then(function (testKey) {
         var page = _omit(payload, 'test');
-        page.browserscopeID = testKey;
+        if (testKey) {
+          page.browserscopeID = testKey;
+        }
         page.published = new Date();
 
         return pagesRepo.create(page);
@@ -49,7 +51,9 @@ exports.register = function (server, options, next) {
     return browserscopeRepo.addTest(payload.title, payload.info, payload.slug, newRev)
       .then(testKey => {
         let page = _omit(payload, 'test');
-        page.browserscopeID = testKey;
+        if (testKey) {
+          page.browserscopeID = testKey;
+        }
         if (isOwn) {
           return pagesRepo.updateById(page, pageId);
         } else {
@@ -109,13 +113,15 @@ exports.register = function (server, options, next) {
 
           browserscopeRepo.addTest(page.title, page.info, s)
             .then(function (testKey) {
-              page.browserscopeID = testKey;
-              return pagesRepo.update({ browserscopeID: testKey }, { id: page.id });
-            })
-            .then(function () {
-              resolve();
-            })
-            .catch(reject);
+              if (testKey) {
+                page.browserscopeID = testKey;
+                pagesRepo.update({ browserscopeID: testKey }, { id: page.id })
+                .then(resolve)
+                .catch(reject);
+              } else {
+                resolve();
+              }
+            });
         });
       })
       .then(function () {


Test patch:
diff --git a/test/unit/server/repositories/browserscope.js b/test/unit/server/repositories/browserscope.js
index 44e3829..d4e7033 100644
--- a/test/unit/server/repositories/browserscope.js
+++ b/test/unit/server/repositories/browserscope.js
@@ -48,8 +48,8 @@ lab.experiment('Browserscope Repository', function () {
     });
 
     lab.test('returns a test key', function (done) {
-      var testKey = 123;
-      var testResp = JSON.stringify({ 'test_key': testKey });
+      const testKey = 123;
+      const testResp = JSON.stringify({ 'test_key': testKey });
 
       browserscope.addTest('My Test', 'is great', 'great-test')
       .then(function (key) {
@@ -63,29 +63,26 @@ lab.experiment('Browserscope Repository', function () {
       Code.expect(emitter.emit('end')).to.be.true();
     });
 
-    lab.test('returns an error when response is wrong', function (done) {
-      browserscope.addTest('My Test', 'is great', 'great-test')
-      .catch(function (err) {
-        Code.expect(err).to.be.instanceof(Error);
-        Code.expect(err.message).to.include('Unexpected response');
+    lab.test('resolves when response is wrong', function (done) {
+      const testResp = 'Unexpected response';
 
-        done();
-      });
+      // error from unexpected response will be logged
+      browserscope.addTest('My Test', 'is great', 'great-test')
+      .then(done)
+      .catch(done);
 
-      emitter.emit('end');
+      // verify event has listener
+      Code.expect(emitter.emit('data', testResp)).to.be.true();
+      Code.expect(emitter.emit('end')).to.be.true();
     });
 
-    lab.test('returns an error when anything else goes wrong', function (done) {
+    lab.test('resolves when anything else goes wrong', function (done) {
       var testErrMsg = 'testing';
       var testErr = new Error(testErrMsg);
 
+      // error will be logged
       browserscope.addTest('My Test', 'is great', 'great-test')
-      .catch(function (err) {
-        Code.expect(err).to.be.instanceof(Error);
-        Code.expect(err.message).to.equal(testErrMsg);
-
-        done();
-      });
+      .then(done);
 
       // verify event has listener
       Code.expect(emitter.emit('error', testErr)).to.be.true();
diff --git a/test/unit/server/services/pages.js b/test/unit/server/services/pages.js
index 50fc068..a89e0ae 100644
--- a/test/unit/server/services/pages.js
+++ b/test/unit/server/services/pages.js
@@ -227,20 +227,14 @@ lab.experiment('Pages Service', function () {
       done();
     });
 
-    lab.test('returns error if browserscope fails to add test', function (done) {
-      var testErrMsg = 'testing';
-      var testErr = new Error(testErrMsg);
-
-      bsRepoStub.addTest.returns(Promise.reject(testErr));
+    lab.test('keeps rolling if browserscope fails to add test', function (done) {
+      bsRepoStub.addTest.returns(Promise.resolve());
+      pagesRepoStub.create.returns(Promise.resolve());
+      testsRepoStub.bulkCreate.returns(Promise.resolve());
 
       pages.create(payload)
-      .catch(function (err) {
-        Code.expect(err).to.be.instanceof(Error);
-        Code.expect(err.message).to.equal(testErrMsg);
-        Code.expect(pagesRepoStub.create.called).to.be.false();
-
-        done();
-      });
+      .then(done)
+      .catch(done);
     });
 
     lab.test('returns error if page fails to create', function (done) {
@@ -266,9 +260,7 @@ lab.experiment('Pages Service', function () {
       var testErr = new Error(testErrMsg);
 
       bsRepoStub.addTest.returns(Promise.resolve());
-
       pagesRepoStub.create.returns(Promise.resolve());
-
       testsRepoStub.bulkCreate.returns(Promise.reject(testErr));
 
       pages.create(payload)
@@ -281,10 +273,8 @@ lab.experiment('Pages Service', function () {
     });
 
     lab.test('adds browserscope test, page, and tests', function (done) {
-      bsRepoStub.addTest.returns(Promise.resolve());
-
+      bsRepoStub.addTest.returns(Promise.resolve('abc123'));
       pagesRepoStub.create.returns(Promise.resolve());
-
       testsRepoStub.bulkCreate.returns(Promise.resolve());
 
       pages.create(payload)
@@ -301,20 +291,14 @@ lab.experiment('Pages Service', function () {
       done();
     });
 
-    lab.test('returns error if browserscope fails to add test', function (done) {
-      var testErrMsg = 'testing';
-      var testErr = new Error(testErrMsg);
-
-      bsRepoStub.addTest.returns(Promise.reject(testErr));
+    lab.test('keeps rolling if browserscope fails to add test', function (done) {
+      bsRepoStub.addTest.returns(Promise.resolve());
+      pagesRepoStub.create.returns(Promise.resolve());
+      testsRepoStub.bulkCreate.returns(Promise.resolve());
 
       pages.edit(payload)
-      .catch(function (err) {
-        Code.expect(err).to.be.instanceof(Error);
-        Code.expect(err.message).to.equal(testErrMsg);
-        Code.expect(pagesRepoStub.create.called).to.be.false();
-
-        done();
-      });
+      .then(done)
+      .catch(done);
     });
 
     lab.test('returns error if page fails to create', function (done) {
@@ -322,7 +306,6 @@ lab.experiment('Pages Service', function () {
       var testErr = new Error(testErrMsg);
 
       bsRepoStub.addTest.returns(Promise.resolve());
-
       pagesRepoStub.create.returns(Promise.reject(testErr));
 
       pages.edit(payload)
@@ -340,7 +323,6 @@ lab.experiment('Pages Service', function () {
       var testErr = new Error(testErrMsg);
 
       bsRepoStub.addTest.returns(Promise.resolve());
-
       pagesRepoStub.updateById.returns(Promise.reject(testErr));
 
       pages.edit(payload, true)
@@ -358,9 +340,7 @@ lab.experiment('Pages Service', function () {
       var testErr = new Error(testErrMsg);
 
       bsRepoStub.addTest.returns(Promise.resolve());
-
       pagesRepoStub.create.returns(Promise.resolve());
-
       testsRepoStub.bulkUpdate.returns(Promise.reject(testErr));
 
       pages.edit(payload)
@@ -373,10 +353,8 @@ lab.experiment('Pages Service', function () {
     });
 
     lab.test('adds browserscope test, page, and tests', function (done) {
-      bsRepoStub.addTest.returns(Promise.resolve());
-
+      bsRepoStub.addTest.returns(Promise.resolve('abc123'));
       pagesRepoStub.create.returns(Promise.resolve());
-
       testsRepoStub.bulkCreate.returns(Promise.resolve());
 
       pages.edit(payload)
@@ -384,10 +362,8 @@ lab.experiment('Pages Service', function () {
     });
 
     lab.test('edits browserscope test, page, and tests', function (done) {
-      bsRepoStub.addTest.returns(Promise.resolve());
-
+      bsRepoStub.addTest.returns(Promise.resolve('abc123'));
       pagesRepoStub.updateById.returns(Promise.resolve());
-
       testsRepoStub.bulkUpdate.returns(Promise.resolve());
 
       pages.edit(payload, true)
@@ -395,10 +371,8 @@ lab.experiment('Pages Service', function () {
     });
 
     lab.test('edits page by calling pageRepo.updateById', function (done) {
-      bsRepoStub.addTest.returns(Promise.resolve());
-
+      bsRepoStub.addTest.returns(Promise.resolve('abc123'));
       pagesRepoStub.updateById.returns(Promise.resolve());
-
       testsRepoStub.bulkUpdate.returns(Promise.resolve());
 
       pages.edit({id: 222}, true, 1, 123)
@@ -509,19 +483,19 @@ lab.experiment('Pages Service', function () {
       });
     });
 
-    lab.test('rejects with error from adding browserscope test', function (done) {
-      var testErrMsg = 'testing';
-      var testErr = new Error(testErrMsg);
+    lab.test('keeps rolling without browserscopeID', function (done) {
       pagesRepoStub.getBySlug.returns(Promise.resolve([{ id: 1 }]));
-      bsRepoStub.addTest.returns(Promise.reject(testErr));
+      bsRepoStub.addTest.returns(Promise.resolve());
+      testsRepoStub.findByPageID.returns(Promise.resolve([]));
+      pagesRepoStub.findBySlug.returns(Promise.resolve([]));
+      commentsRepoStub.findByPageID.returns(Promise.resolve([]));
 
       pages.getBySlug(slug, rev)
-      .catch(function (err) {
-        Code.expect(err).to.be.instanceof(Error);
-        Code.expect(err.message).to.equal(testErrMsg);
-
+      .then(function (values) {
+        Code.expect(values).to.be.array();
         done();
-      });
+      })
+      .catch(done);
     });
 
     lab.test('rejects with error from updating browserscopeID of page', function (done) {