Repository: paularmstrong/build-tracker
Pull request #109 (2020-02-13 18:30:34)

Fix upload build response
<!--
Thank you so much for contributing to open source and the Build Tracker project!
-->

# Problem

The value of `response` that's passed into `onCompare` does not match the param type (`ApiReturn`).

```sh-session
$ node
> Buffer.from(['{"success":"yep"}']).toJSON()
{ type: 'Buffer', data: [ 0 ] }
```

I had to make this change to get it working.

I don't understand how the tests are passing?

# 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

- [x] 🤓 Add & update tests (always try to _increase_ test coverage)
- [x] 🔬 Ensure CI is passing (`yarn lint:ci`, `yarn test`, `yarn tsc:ci`)
- [x] 📖 Update relevant documentation


OliverJAsh: Ah, just realised that the docs are correct, but they don't reflect the latest version that's on npm. This commit hasn't been published yet: https://github.com/paularmstrong/build-tracker/commit/23ac9f7d45a27e3063fd9bdfa55759bf7a970fcc#diff-3ebd3c0d7bc0a813b71e1aae5f610dbc.
OliverJAsh: > If you get a chance to add one

Done!

Patch:
diff --git a/src/cli/src/commands/upload-build.ts b/src/cli/src/commands/upload-build.ts
index e84395d..9f51039 100644
--- a/src/cli/src/commands/upload-build.ts
+++ b/src/cli/src/commands/upload-build.ts
@@ -3,10 +3,10 @@
  */
 import { Argv } from 'yargs';
 import { handler as createBuild } from './create-build';
-import getConfig from '../modules/config';
 import http from 'http';
 import https from 'https';
 import { URL } from 'url';
+import getConfig, { ApiReturn } from '../modules/config';
 
 export const command = 'upload-build';
 
@@ -76,9 +76,8 @@ export const handler = async (args: Args): Promise<void> => {
       });
 
       res.on('end', () => {
-        const response = Buffer.from(output).toJSON();
+        const response = JSON.parse(output.join('')) as ApiReturn;
         if (config.onCompare) {
-          // @ts-ignore
           config.onCompare(response).then(resolve);
         } else {
           resolve();
diff --git a/src/cli/src/modules/config.ts b/src/cli/src/modules/config.ts
index 247d807..39018ba 100644
--- a/src/cli/src/modules/config.ts
+++ b/src/cli/src/modules/config.ts
@@ -10,7 +10,7 @@ interface BuildJson {
   artifacts: Array<Artifact<ArtifactSizes>>;
 }
 
-interface ApiReturn {
+export interface ApiReturn {
   build: BuildJson;
   parentBuild: BuildJson;
   json: ComparisonMatrix;
@@ -35,7 +35,11 @@ export default async function getConfig(path?: string): Promise<Config> {
   try {
     result = !path ? await explorer.search() : await explorer.load(path);
   } catch (e) {
-    throw new Error('Could not find configuration file');
+    if (e.code === 'ENOENT') {
+      throw new Error('Could not find configuration file');
+    } else {
+      throw e;
+    }
   }
 
   return {
diff --git a/src/fixtures/cli-configs/rc/.build-trackerrc-invalid.js b/src/fixtures/cli-configs/rc/.build-trackerrc-invalid.js
new file mode 100644
index 0000000..478d9ec
--- /dev/null
+++ b/src/fixtures/cli-configs/rc/.build-trackerrc-invalid.js
@@ -0,0 +1,1 @@
+throw new Error('test');
\ No newline at end of file
diff --git a/src/fixtures/cli-configs/rc/.build-trackerrc.js b/src/fixtures/cli-configs/rc/.build-trackerrc.js
index ce560b6..f7a25ae 100644
--- a/src/fixtures/cli-configs/rc/.build-trackerrc.js
+++ b/src/fixtures/cli-configs/rc/.build-trackerrc.js
@@ -4,5 +4,6 @@ module.exports = {
   applicationUrl: 'https://build-tracker.local',
   artifacts: ['../fakedist/*.js'],
   baseDir: path.resolve(__dirname, 'fakedist'),
-  cwd: __dirname
+  cwd: __dirname,
+  onCompare: data => Promise.resolve()
 };
diff --git a/src/server/src/api/insert.ts b/src/server/src/api/insert.ts
index fd188d0..3160582 100644
--- a/src/server/src/api/insert.ts
+++ b/src/server/src/api/insert.ts
@@ -19,17 +19,27 @@ export const insertBuild = (
   queries.insert(build).then(() =>
     queries
       .byRevision(build.getMetaValue('parentRevision'))
-      .then(parentBuildData => {
-        const parentBuild = new Build(parentBuildData.meta, parentBuildData.artifacts);
-        return {
-          comparator: new Comparator({
-            artifactBudgets: artifactConfig.budgets,
-            artifactFilters: artifactConfig.filters,
-            builds: [build, parentBuild],
-            groups: artifactConfig.groups
-          })
-        };
-      })
+      .then(
+        parentBuildData => {
+          const parentBuild = new Build(parentBuildData.meta, parentBuildData.artifacts);
+          return [build, parentBuild];
+        },
+        error => {
+          if (error instanceof NotFoundError) {
+            return [build];
+          } else {
+            throw error;
+          }
+        }
+      )
+      .then(builds => ({
+        comparator: new Comparator({
+          artifactBudgets: artifactConfig.budgets,
+          artifactFilters: artifactConfig.filters,
+          builds,
+          groups: artifactConfig.groups
+        })
+      }))
       .then(context => {
         return onInserted(context.comparator).then(() => context);
       })
@@ -40,10 +50,6 @@ export const insertBuild = (
         });
       })
       .catch(error => {
-        if (error instanceof NotFoundError) {
-          res.send({ error });
-          return;
-        }
         res.send({ error });
       })
   );


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 e3640dc..85fa180 100644
--- a/src/cli/src/commands/__tests__/upload-build.test.ts
+++ b/src/cli/src/commands/__tests__/upload-build.test.ts
@@ -50,6 +50,19 @@ describe('upload-build', () => {
       expect(writeSpy).toHaveBeenCalledWith('{"success":"yep"}');
     });
 
+    test('invokes `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(200, { success: 'yep' });
+
+      await expect(Command.handler({ config, out: true, 'skip-dirty-check': true })).resolves.toBeUndefined();
+      expect(onCompareSpy).toHaveBeenCalledWith({ success: 'yep' });
+    });
+
     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/cli/src/modules/__tests__/config.test.ts b/src/cli/src/modules/__tests__/config.test.ts
index 95d3e36..42165fb 100644
--- a/src/cli/src/modules/__tests__/config.test.ts
+++ b/src/cli/src/modules/__tests__/config.test.ts
@@ -31,6 +31,14 @@ describe('getConfig', () => {
     });
   });
 
+  test('propagate errors', () => {
+    return getConfig(
+      path.join(path.dirname(require.resolve('@build-tracker/fixtures')), 'cli-configs/rc/.build-trackerrc-invalid.js')
+    ).catch(e => {
+      expect(e.message).toMatch('test');
+    });
+  });
+
   test('throws if no configuration found', () => {
     return getConfig('tacos').catch(e => {
       expect(e.message).toMatch('Could not find configuration file');
diff --git a/src/server/src/api/__tests__/insert.test.ts b/src/server/src/api/__tests__/insert.test.ts
index d351fa8..81af5e6 100644
--- a/src/server/src/api/__tests__/insert.test.ts
+++ b/src/server/src/api/__tests__/insert.test.ts
@@ -6,6 +6,7 @@ import Build from '@build-tracker/build';
 import Comparator from '@build-tracker/comparator';
 import express from 'express';
 import { insertBuild } from '../insert';
+import { NotFoundError } from '@build-tracker/api-errors';
 import request from 'supertest';
 
 const build = new Build({ branch: 'master', revision: 'abc', parentRevision: 'def', timestamp: Date.now() }, [
@@ -65,27 +66,103 @@ describe('insert build handler', () => {
     });
   });
 
-  describe('onInserted', () => {
-    test('called with a comparator of the inserted branch against its parentRevision', () => {
-      const handleInsert = jest.fn(() => Promise.resolve());
-      const handler = insertBuild(queries, config, handleInsert);
-      app.post('/test', handler);
+  describe('parent revision', () => {
+    describe('onInserted', () => {
+      test('called with a comparator of the inserted branch against its parentRevision', () => {
+        const handleInsert = jest.fn(() => Promise.resolve());
+        const handler = insertBuild(queries, config, handleInsert);
+        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(() => {
-          expect(handleInsert).toHaveBeenCalled();
-          // @ts-ignore
-          const [comparator] = handleInsert.mock.calls[0];
-          // @ts-ignore
-          expect(comparator.builds.map(build => build.getMetaValue('revision'))).toEqual([
-            build.getMetaValue('revision'),
-            parentBuild.getMetaValue('revision')
-          ]);
-        });
+        return request(app)
+          .post('/test')
+          .send({ meta: build.meta, artifacts: build.artifacts })
+          .set('Content-Type', 'application/json')
+          .set('Accept', 'application/json')
+          .then(() => {
+            expect(handleInsert).toHaveBeenCalled();
+            // @ts-ignore
+            const [comparator] = handleInsert.mock.calls[0];
+            // @ts-ignore
+            expect(comparator.builds.map(build => build.getMetaValue('revision'))).toEqual([
+              build.getMetaValue('revision'),
+              parentBuild.getMetaValue('revision')
+            ]);
+          });
+      });
+    });
+  });
+
+  describe('no parent revision', () => {
+    let app, config, queries;
+    beforeEach(() => {
+      queries = {
+        byRevision: jest.fn(() => Promise.reject(new NotFoundError())),
+        insert: jest.fn(() => Promise.resolve('1234'))
+      };
+      config = {
+        artifacts: {},
+        queries
+      };
+      app = express();
+      app.use(bodyParser.json());
+    });
+
+    describe('insert', () => {
+      test('inserts the build', () => {
+        const handler = insertBuild(queries, 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(() => {
+            expect(queries.insert).toHaveBeenCalled();
+          });
+      });
+    });
+
+    describe('response', () => {
+      test('returns a JSON serialized Comparator and summary', () => {
+        const handler = insertBuild(queries, config);
+        app.post('/test', handler);
+
+        const comparator = new Comparator({ builds: [build] });
+
+        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.body.comparatorData).toEqual(comparator.serialize());
+            expect(res.body.summary).toEqual(comparator.toSummary());
+          });
+      });
+    });
+
+    describe('onInserted', () => {
+      test('called with a comparator of the inserted branch against its parentRevision', () => {
+        const handleInsert = jest.fn(() => Promise.resolve());
+        const handler = insertBuild(queries, config, handleInsert);
+        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(() => {
+            expect(handleInsert).toHaveBeenCalled();
+            // @ts-ignore
+            const [comparator] = handleInsert.mock.calls[0];
+            // @ts-ignore
+            expect(comparator.builds.map(build => build.getMetaValue('revision'))).toEqual([
+              build.getMetaValue('revision')
+            ]);
+          });
+      });
     });
   });
 });