Repository: paularmstrong/build-tracker
Pull request #134 (2020-02-19 16:41:23)

Upload build: handle errors
<!--
Thank you so much for contributing to open source and the Build Tracker project!
-->

# Problem

See tests/comments.

# Solution

<!--
When trying to solve more solutions with Build Tracker, please keep in mind some of the following goals of the project:
* Be lightweight: small package sizes (single-digit KiBs, gzipped)
* Be easy: too many options in an API can become confusing
* Be clear: the intended purpose of every method should be as obvious as possible
* Is it easy to do this in "userland"? Would it be better off done there?
-->

Explain your approach. Sometimes it helps to justify your approach against some others that you didn't choose to explain why yours is better.

# TODO

- [ ] 🤓 Add & update tests (always try to _increase_ test coverage)
- [ ] 🔬 Ensure CI is passing (`yarn lint:ci`, `yarn test`, `yarn tsc:ci`)
- [ ] 📖 Update relevant documentation



Patch:
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 48f7d8b..2ea94fd 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -1,1 +1,1 @@
-Please refer to the contributing guidelines at https://buildtracker.dev/docs/contributing
+Please refer to the contributing guidelines at https://buildtracker.dev/docs/guides/contributing
diff --git a/docs/docs/guides/contributing.md b/docs/docs/guides/contributing.md
index 1ee77af..711f938 100644
--- a/docs/docs/guides/contributing.md
+++ b/docs/docs/guides/contributing.md
@@ -153,8 +153,8 @@ Build Tracker is written in Typescript throughout all workspaces. Please do not
 To run the development environment, some shortcuts are provided in the `package.json` scripts:
 
 - `yarn dev` Run the server and application in a hot-reloadable environment using pre-seeded builds on the filesystem
-- `yarn dev:maria` (coming soon) Run the server using a local MariaDB instance
-- `yarn dev:postgres` (coming soon) Run the server using a local Postgres instance
+- `yarn dev:maria` Run the server using a local MariaDB instance
+- `yarn dev:postgres` Run the server using a local Postgres instance
 
 ## Code integrity
 
diff --git a/src/cli/src/commands/upload-build.ts b/src/cli/src/commands/upload-build.ts
index 9f51039..15b5fbe 100644
--- a/src/cli/src/commands/upload-build.ts
+++ b/src/cli/src/commands/upload-build.ts
@@ -36,6 +36,11 @@ export const builder = (yargs): Argv<Args> =>
       group,
       normalize: true
     })
+    .option('parent-revision', {
+      description: 'Manually provide the parent revision instead of reading it automatically',
+      group,
+      type: 'string'
+    })
     .option('skip-dirty-check', {
       default: false,
       description: 'Skip the git work tree state check',
@@ -76,11 +81,16 @@ export const handler = async (args: Args): Promise<void> => {
       });
 
       res.on('end', () => {
-        const response = JSON.parse(output.join('')) as ApiReturn;
-        if (config.onCompare) {
-          config.onCompare(response).then(resolve);
+        const response = JSON.parse(output.join(''));
+        if (res.statusCode >= 400) {
+          reject(new Error(response.error));
         } else {
-          resolve();
+          const successResponse = response as ApiReturn;
+          if (config.onCompare) {
+            config.onCompare(successResponse).then(resolve);
+          } else {
+            resolve();
+          }
         }
       });
     });
diff --git a/src/server/src/api/insert.ts b/src/server/src/api/insert.ts
index bdc6a56..feeb2bd 100644
--- a/src/server/src/api/insert.ts
+++ b/src/server/src/api/insert.ts
@@ -16,42 +16,45 @@ export const insertBuild = (
   const { artifacts, meta } = req.body;
   const { artifacts: artifactConfig = {}, budgets } = config;
   const build = new Build(meta, artifacts);
-  queries.insert(build).then(() =>
-    queries
-      .byRevision(build.getMetaValue('parentRevision'))
-      .then(
-        parentBuildData => {
-          const parentBuild = new Build(parentBuildData.meta, parentBuildData.artifacts);
-          return [parentBuild, build];
-        },
-        error => {
-          if (error instanceof NotFoundError) {
-            return [build];
-          } else {
-            throw error;
+  queries
+    .insert(build)
+    .then(() =>
+      queries
+        .byRevision(build.getMetaValue('parentRevision'))
+        .then(
+          parentBuildData => {
+            const parentBuild = new Build(parentBuildData.meta, parentBuildData.artifacts);
+            return [parentBuild, build];
+          },
+          error => {
+            if (error instanceof NotFoundError) {
+              return [build];
+            } else {
+              throw error;
+            }
           }
-        }
-      )
-      .then(builds => ({
-        comparator: new Comparator({
-          artifactBudgets: artifactConfig.budgets,
-          artifactFilters: artifactConfig.filters,
-          builds,
-          budgets,
-          groups: artifactConfig.groups
+        )
+        .then(builds => ({
+          comparator: new Comparator({
+            artifactBudgets: artifactConfig.budgets,
+            artifactFilters: artifactConfig.filters,
+            builds,
+            budgets,
+            groups: artifactConfig.groups
+          })
+        }))
+        .then(context => {
+          return onInserted(context.comparator).then(() => context);
         })
-      }))
-      .then(context => {
-        return onInserted(context.comparator).then(() => context);
-      })
-      .then(({ comparator }) => {
-        res.send({
-          comparatorData: comparator.serialize(),
-          summary: comparator.toSummary()
-        });
-      })
-      .catch(error => {
-        res.send({ error });
-      })
-  );
+        .then(({ comparator }) => {
+          res.send({
+            comparatorData: comparator.serialize(),
+            summary: comparator.toSummary()
+          });
+        })
+    )
+    .catch(error => {
+      res.status(500);
+      res.send({ error: error.message });
+    });
 };


Test patch:
diff --git a/src/cli/src/commands/__tests__/upload-build.test.ts b/src/cli/src/commands/__tests__/upload-build.test.ts
index 85fa180..8894e20 100644
--- a/src/cli/src/commands/__tests__/upload-build.test.ts
+++ b/src/cli/src/commands/__tests__/upload-build.test.ts
@@ -63,6 +63,21 @@ describe('upload-build', () => {
       expect(onCompareSpy).toHaveBeenCalledWith({ success: 'yep' });
     });
 
+    test('if request failed, responds with error and does not invoke `onCompare`', async () => {
+      jest.spyOn(CreateBuild, 'handler').mockImplementation(() => Promise.resolve(postData));
+      const configValues = require(config);
+      const onCompareSpy = jest.spyOn(configValues, 'onCompare').mockImplementationOnce(() => Promise.resolve());
+
+      nock(`${configValues.applicationUrl}/`)
+        .post('/api/builds')
+        .reply(500, { error: 'Something went wrong.' });
+
+      await expect(Command.handler({ config, out: true, 'skip-dirty-check': true })).rejects.toStrictEqual(
+        new Error('Something went wrong.')
+      );
+      expect(onCompareSpy).not.toHaveBeenCalled();
+    });
+
     test('uploads the current build over http', async () => {
       jest.spyOn(CreateBuild, 'handler').mockImplementation(() => Promise.resolve(postData));
       const writeSpy = jest.spyOn(process.stdout, 'write').mockImplementationOnce(() => true);
diff --git a/src/server/src/api/__tests__/insert.test.ts b/src/server/src/api/__tests__/insert.test.ts
index 1a1e6ae..c38a6f7 100644
--- a/src/server/src/api/__tests__/insert.test.ts
+++ b/src/server/src/api/__tests__/insert.test.ts
@@ -47,6 +47,26 @@ describe('insert build handler', () => {
     });
   });
 
+  describe('insert failed', () => {
+    test('responds with error', () => {
+      const handler = insertBuild(
+        { ...queries, insert: () => Promise.reject(new Error('Something went wrong.')) },
+        config
+      );
+      app.post('/test', handler);
+
+      return request(app)
+        .post('/test')
+        .send({ meta: build.meta, artifacts: build.artifacts })
+        .set('Content-Type', 'application/json')
+        .set('Accept', 'application/json')
+        .then(res => {
+          expect(res.status).toEqual(500);
+          expect(res.body).toEqual({ error: 'Something went wrong.' });
+        });
+    });
+  });
+
   describe('response', () => {
     test('returns a JSON serialized Comparator and summary', () => {
       const handler = insertBuild(queries, config);