Repository: paularmstrong/build-tracker (ts)

Issue:
Error: [object Object]
# Problem

I got this locally when I was first setting up things... But surprisingly this repo's GH actions ran into it:  https://github.com/paularmstrong/build-tracker/pull/175/checks?check_run_id=518328757

![image](https://user-images.githubusercontent.com/39191/77030968-77bd6f80-695d-11ea-87cb-fb7f841a4cf2.png)


**Steps to Reproduce:**

Pretty sure if you don't set the BT_API_AUTH_TOKEN env var then the server will 401 and this'll happen. (That's how I ran into it).

Probably should stringify if its an object?
Silent failure when `BT_API_AUTH_TOKEN` wasn't provided
(just breaking this piece of #176 into a diff issue)

# Problem

New users are dumb. Like me. 

And when I first tried upload-build i hadn't yet set the env variable. The server 401'd (and that was a silentish failure thanks to #176). But it took a minute to realize I hadn't included `BT_API_AUTH_TOKEN`.


Your [CLI docs](https://buildtracker.dev/docs/packages/cli#upload-build) already include this env var, but I hadn't yet found those docs. :)

# Idea

Perhaps a friendly error message could be provided? Something a la..

> The server returned a 401 Unauthorized Error. Did you include the correct BT_API_AUTH_TOKEN?

--------

also wow the docs are really nice. docusaurus seems badass.

Pull request:
fix(api): ensure output from api/api-client gives context
<!--
Thank you so much for contributing to open source and the Build Tracker project!
-->

# Problem

Errors from the api-client and cli are pretty vague. They do not indicate what went wrong and how to fix it.

# Solution

Include more context in the response for `AuthError`s. Update the api-client to properly log the error response string instead of `[object Object]`

Example: <img width="1023" alt="Screen Shot 2020-03-20 at 9 06 46 AM" src="https://user-images.githubusercontent.com/33297/77183032-f49a3780-6a8a-11ea-86af-5254fee7d2cf.png">

Fixes #176 
Fixes #177

cc @paulirish 

# TODO

- [x] 🤓 Add & update tests (always try to _increase_ test coverage)
- [x] 🔬 Ensure CI is passing (`yarn lint:ci`, `yarn test`, `yarn tsc:ci`)
- [ ] 📖 Update relevant documentation


Patch:
diff --git a/src/api-client/src/modules/upload-build.ts b/src/api-client/src/modules/upload-build.ts
index fa50c09..d8a6253 100644
--- a/src/api-client/src/modules/upload-build.ts
+++ b/src/api-client/src/modules/upload-build.ts
@@ -45,26 +45,28 @@ export default async function uploadBuild(
       res.setEncoding('utf8');
 
       res.on('data', data => {
-        output.push(data);
-        logger.log(data);
+        output.push(typeof data === 'string' ? data : data.toString());
       });
 
-      res.on('end', () => {
+      res.on('end', async () => {
         const response = JSON.parse(output.join(''));
         if (res.statusCode >= 400) {
+          logger.error(response.error);
           reject(new Error(response.error));
-        } else {
-          const successResponse = response as ApiReturn;
-          if (onCompare) {
-            onCompare(successResponse).then(() => resolve(successResponse));
-          } else {
-            resolve(successResponse);
-          }
+          return;
         }
+
+        const successResponse = response as ApiReturn;
+        if (onCompare) {
+          await onCompare(successResponse);
+        }
+
+        logger.log(JSON.stringify(successResponse));
+        resolve(successResponse);
       });
     });
 
-    req.on('error', error => {
+    req.on('error', (error: Error) => {
       logger.error(error.toString());
       reject(error);
     });
diff --git a/src/app/src/components/ComparisonTable/DeltaCell.tsx b/src/app/src/components/ComparisonTable/DeltaCell.tsx
index 714a6a7..73dede2 100644
--- a/src/app/src/components/ComparisonTable/DeltaCell.tsx
+++ b/src/app/src/components/ComparisonTable/DeltaCell.tsx
@@ -72,7 +72,7 @@ export const DeltaCell = (props: Props): React.ReactElement => {
   const stringChange = `${sizeDelta} bytes (${(percentDelta * 100).toFixed(3)}%)`;
 
   const text = formatBytes(sizeDelta);
-  const hashChangedOnly = sizeDelta === 0 && cell.hashChanged;
+  const hashChangedOnly = cell.hashChangeUnexpected;
   const tooltipText = failingBudgets.length
     ? failingBudgets.map(budget => formatBudgetResult(budget, cell.name)).join(', ')
     : hashChangedOnly
diff --git a/src/cli/src/commands/upload-build.ts b/src/cli/src/commands/upload-build.ts
index 854b261..f23a12f 100644
--- a/src/cli/src/commands/upload-build.ts
+++ b/src/cli/src/commands/upload-build.ts
@@ -22,12 +22,16 @@ export const handler = async (args: Args): Promise<void> => {
     skipDirtyCheck: args['skip-dirty-check']
   });
 
-  await uploadBuild(config, build, process.env.BT_API_AUTH_TOKEN, {
-    log: input => {
-      process.stdout.write(input);
-    },
-    error: input => {
-      process.stderr.write(input);
-    }
-  });
+  try {
+    await uploadBuild(config, build, process.env.BT_API_AUTH_TOKEN, {
+      log: input => {
+        process.stdout.write(`${input}\n`);
+      },
+      error: input => {
+        process.stderr.write(`${input}\n`);
+      }
+    });
+  } catch (e) {
+    process.exit(1);
+  }
 };
diff --git a/src/comparator/src/ArtifactDelta.ts b/src/comparator/src/ArtifactDelta.ts
index 5f51171..0e59394 100644
--- a/src/comparator/src/ArtifactDelta.ts
+++ b/src/comparator/src/ArtifactDelta.ts
@@ -57,6 +57,14 @@ export default class ArtifactDelta {
     return this._hashChanged;
   }
 
+  public get hashChangeUnexpected(): boolean {
+    return (
+      this._hashChanged &&
+      this.failingBudgets.length === 0 &&
+      Object.keys(this.sizes).every(sizeKey => this.sizes[sizeKey] === 0)
+    );
+  }
+
   public get budgets(): Array<BudgetResult> {
     if (!this._results) {
       const sizes = this.sizes;
@@ -97,6 +105,7 @@ export default class ArtifactDelta {
       sizes: this.sizes,
       percents: this.percents,
       hashChanged: this.hashChanged,
+      hashChangeUnexpected: this.hashChangeUnexpected,
       budgets: this.budgets,
       failingBudgets: this.failingBudgets
     };
diff --git a/src/comparator/src/index.ts b/src/comparator/src/index.ts
index 3fc9685..3ec56d8 100644
--- a/src/comparator/src/index.ts
+++ b/src/comparator/src/index.ts
@@ -39,6 +39,7 @@ export interface DeltaCell {
   sizes: ArtifactSizes;
   percents: ArtifactSizes;
   hashChanged: boolean;
+  hashChangeUnexpected: boolean;
   budgets: Array<BudgetResult>;
   failingBudgets: Array<BudgetResult>;
 }
@@ -55,6 +56,7 @@ export interface TotalDeltaCell {
   sizes: ArtifactSizes;
   percents: ArtifactSizes;
   hashChanged: boolean;
+  hashChangeUnexpected: boolean;
   budgets: Array<BudgetResult>;
   failingBudgets: Array<BudgetResult>;
 }
@@ -122,14 +124,13 @@ const defaultFormatTotal = (cell: TotalCell, sizeKey: string): string => formatB
 const defaultFormatDelta = (cell: DeltaCell | TotalDeltaCell, sizeKey: string): string => {
   const errorFailingBudgets = cell.failingBudgets.some(result => result.level === BudgetLevel.ERROR);
   const warningFailingBudgets = cell.failingBudgets.some(result => result.level === BudgetLevel.WARN);
-  const warningHashChanged =
-    cell.hashChanged && cell.failingBudgets.length === 0 && Object.values(cell.sizes).every(size => size === 0);
+
   return `${
     errorFailingBudgets
       ? `${levelToEmoji[BudgetLevel.ERROR]} `
       : warningFailingBudgets
       ? `${levelToEmoji[BudgetLevel.WARN]} `
-      : warningHashChanged
+      : cell.hashChangeUnexpected
       ? `${levelToEmoji['hash']} `
       : ''
   }${formatBytes(cell.sizes[sizeKey] || 0)} (${((cell.percents[sizeKey] || 0) * 100).toFixed(1)}%)`;
@@ -493,11 +494,7 @@ export default class BuildComparator {
           memo.push(formatBudgetResult(budget, row[0].text, useEmoji));
         });
 
-        if (
-          cell.failingBudgets.length === 0 &&
-          cell.hashChanged &&
-          Object.values(cell.sizes).every(size => size === 0)
-        ) {
+        if (cell.hashChangeUnexpected) {
           memo.push(formatUnexpectedHashChange(row[0].text, useEmoji));
         }
       });
diff --git a/src/server/src/api/protected.ts b/src/server/src/api/protected.ts
index 2db3862..250b077 100644
--- a/src/server/src/api/protected.ts
+++ b/src/server/src/api/protected.ts
@@ -14,12 +14,18 @@ export default function(req: Request, res: Response, next: NextFunction): void {
   }
 
   if (!authHeaderValue) {
-    res.status(401).send({ error: new AuthError(`${req.path} request x-bt-auth header`) });
+    const error = new AuthError(
+      `${
+        req.path
+      } requires the x-bt-auth header to be set. This API is secured with a token, ensure you set the same BT_API_AUTH_TOKEN variable before requesting again`
+    );
+    res.status(error.status).send({ error: error.message });
     return;
   }
 
   if (authHeaderValue !== authToken) {
-    res.status(401).send({ error: new AuthError('invalid auth token') });
+    const error = new AuthError('invalid x-bt-auth token header');
+    res.status(error.status).send({ error: error.message });
     return;
   }
 


Test patch:
diff --git a/src/api-client/src/modules/__tests__/upload-build.test.ts b/src/api-client/src/modules/__tests__/upload-build.test.ts
index 47fccda..14f5b95 100644
--- a/src/api-client/src/modules/__tests__/upload-build.test.ts
+++ b/src/api-client/src/modules/__tests__/upload-build.test.ts
@@ -57,13 +57,17 @@ describe('uploadBuild', () => {
   });
 
   test('if request failed, responds with error and does not invoke `onCompare`', async () => {
+    const logger = { log: jest.fn(), error: jest.fn() };
     const onCompareSpy = jest.spyOn(config, 'onCompare').mockImplementationOnce(() => Promise.resolve());
 
     nock(`${config.applicationUrl}/`)
       .post('/api/builds')
       .reply(500, { error: 'Something went wrong.' });
 
-    await expect(uploadBuild(config, build)).rejects.toStrictEqual(new Error('Something went wrong.'));
+    await expect(uploadBuild(config, build, undefined, logger)).rejects.toStrictEqual(
+      new Error('Something went wrong.')
+    );
+    expect(logger.error).toHaveBeenCalledWith('Something went wrong.');
     expect(onCompareSpy).not.toHaveBeenCalled();
   });
 
@@ -102,6 +106,6 @@ describe('uploadBuild', () => {
       .replyWithError(responseError);
 
     await expect(uploadBuild(config, build, undefined, logger)).rejects.toEqual(responseError);
-    expect(logger.error).toHaveBeenLastCalledWith(responseError.toString());
+    expect(logger.error).toHaveBeenCalledWith('Error: some error');
   });
 });
diff --git a/src/app/src/components/ComparisonTable/__tests__/BodyRow.test.tsx b/src/app/src/components/ComparisonTable/__tests__/BodyRow.test.tsx
index 70b4286..2d42d18 100644
--- a/src/app/src/components/ComparisonTable/__tests__/BodyRow.test.tsx
+++ b/src/app/src/components/ComparisonTable/__tests__/BodyRow.test.tsx
@@ -51,7 +51,8 @@ describe('BodyRow', () => {
           failingBudgets: [],
           sizes: { stat: 4 },
           percents: { stat: 4 },
-          hashChanged: false
+          hashChanged: false,
+          hashChangeUnexpected: false
         }
       ];
       const { getByType } = render(
diff --git a/src/app/src/components/ComparisonTable/__tests__/DeltaCell.test.tsx b/src/app/src/components/ComparisonTable/__tests__/DeltaCell.test.tsx
index 8d509cc..e4aca29 100644
--- a/src/app/src/components/ComparisonTable/__tests__/DeltaCell.test.tsx
+++ b/src/app/src/components/ComparisonTable/__tests__/DeltaCell.test.tsx
@@ -24,6 +24,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { stat: 0.5 },
             hashChanged: true,
+            hashChangeUnexpected: false,
             sizes: { stat: 4300 }
           }}
           sizeKey="stat"
@@ -42,6 +43,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { stat: 0 },
             hashChanged: false,
+            hashChangeUnexpected: false,
             sizes: { stat: 0 }
           }}
           sizeKey="stat"
@@ -60,6 +62,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { stat: -0.5 },
             hashChanged: true,
+            hashChangeUnexpected: false,
             sizes: { stat: -134 }
           }}
           sizeKey="stat"
@@ -79,6 +82,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { stat: 0 },
             hashChanged: true,
+            hashChangeUnexpected: true,
             sizes: { stat: 0 }
           }}
           sizeKey="stat"
@@ -106,6 +110,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { stat: 0 },
             hashChanged: false,
+            hashChangeUnexpected: false,
             sizes: { stat: 5 }
           }}
           sizeKey="stat"
@@ -132,6 +137,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { stat: 0 },
             hashChanged: false,
+            hashChangeUnexpected: false,
             sizes: { stat: 5 }
           }}
           sizeKey="stat"
@@ -152,6 +158,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { gzip: -1 },
             hashChanged: true,
+            hashChangeUnexpected: false,
             sizes: { gzip: -4300 }
           }}
           sizeKey="gzip"
@@ -172,6 +179,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { gzip: 0.9 },
             hashChanged: true,
+            hashChangeUnexpected: false,
             sizes: { gzip: 4300 }
           }}
           sizeKey="gzip"
@@ -182,7 +190,7 @@ describe('DeltaCell', () => {
       });
     });
 
-    test('is warning color if no size change, but hash changed', () => {
+    test('is warning color if hash changed unexpectedly', () => {
       const { getByType } = render(
         <DeltaCell
           cell={{
@@ -192,6 +200,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { gzip: 0 },
             hashChanged: true,
+            hashChangeUnexpected: true,
             sizes: { gzip: 0 }
           }}
           sizeKey="gzip"
@@ -212,6 +221,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { gzip: 0 },
             hashChanged: false,
+            hashChangeUnexpected: false,
             sizes: { gzip: 0 }
           }}
           sizeKey="gzip"
@@ -237,6 +247,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { gzip: 1 },
             hashChanged: false,
+            hashChangeUnexpected: false,
             sizes: { gzip: 1024 }
           }}
           sizeKey="gzip"
@@ -256,6 +267,7 @@ describe('DeltaCell', () => {
             name: 'tacos',
             percents: { gzip: 1 },
             hashChanged: false,
+            hashChangeUnexpected: false,
             sizes: { gzip: 1024 }
           }}
           sizeKey="gzip"
diff --git a/src/app/src/components/ComparisonTable/__tests__/GroupRow.test.tsx b/src/app/src/components/ComparisonTable/__tests__/GroupRow.test.tsx
index 55ecc31..b5e94a5 100644
--- a/src/app/src/components/ComparisonTable/__tests__/GroupRow.test.tsx
+++ b/src/app/src/components/ComparisonTable/__tests__/GroupRow.test.tsx
@@ -64,6 +64,7 @@ describe('GroupRow', () => {
           type: CellType.TOTAL_DELTA,
           name: 'tacos',
           hashChanged: true,
+          hashChangeUnexpected: false,
           budgets: [],
           failingBudgets: [],
           sizes: { stat: 4 },
diff --git a/src/comparator/src/__tests__/ArtifactDelta.test.ts b/src/comparator/src/__tests__/ArtifactDelta.test.ts
index 87ba078..5beac78 100644
--- a/src/comparator/src/__tests__/ArtifactDelta.test.ts
+++ b/src/comparator/src/__tests__/ArtifactDelta.test.ts
@@ -32,6 +32,38 @@ describe('ArtifactDelta', () => {
     });
   });
 
+  describe('hashChangeUnexpected', () => {
+    test('is false when no hash change', () => {
+      expect(
+        new ArtifactDelta('tacos', [], { gzip: 1, stat: 2 }, { gzip: 1, stat: 2 }, false).hashChangeUnexpected
+      ).toBe(false);
+    });
+
+    test('is false when hash change and any size changes', () => {
+      expect(
+        new ArtifactDelta('tacos', [], { gzip: 2, stat: 2 }, { gzip: 1, stat: 2 }, true).hashChangeUnexpected
+      ).toBe(false);
+    });
+
+    test('is false when there is a budget failure', () => {
+      expect(
+        new ArtifactDelta(
+          'tacos',
+          [{ level: BudgetLevel.ERROR, type: BudgetType.SIZE, sizeKey: 'gzip', maximum: 3 }],
+          { gzip: 5, stat: 10 },
+          { gzip: 5, stat: 10 },
+          false
+        ).hashChangeUnexpected
+      ).toBe(false);
+    });
+
+    test('is true when all sizes are zero and no failing budgets', () => {
+      expect(
+        new ArtifactDelta('tacos', [], { gzip: 1, stat: 2 }, { gzip: 1, stat: 2 }, true).hashChangeUnexpected
+      ).toBe(true);
+    });
+  });
+
   describe('budgets', () => {
     test('returns a list with delta budget', () => {
       const ad = new ArtifactDelta(
@@ -142,6 +174,7 @@ describe('ArtifactDelta', () => {
         sizes: { stat: 1 },
         percents: { stat: 1 / 3 },
         hashChanged: false,
+        hashChangeUnexpected: false,
         budgets: [budget],
         failingBudgets: [budget]
       });
diff --git a/src/comparator/src/__tests__/index.test.ts b/src/comparator/src/__tests__/index.test.ts
index 17aead3..460e773 100644
--- a/src/comparator/src/__tests__/index.test.ts
+++ b/src/comparator/src/__tests__/index.test.ts
@@ -275,6 +275,7 @@ describe('BuildComparator', () => {
         budgets: [],
         failingBudgets: [],
         hashChanged: true,
+        hashChangeUnexpected: false,
         sizes: { gzip: 28, stat: 13 },
         percents: { gzip: 0.2074074074074074, stat: 0.022452504317789293 }
       });
@@ -306,6 +307,7 @@ describe('BuildComparator', () => {
         type: 'totalDelta',
         name: 'stuff',
         hashChanged: true,
+        hashChangeUnexpected: false,
         budgets: [],
         failingBudgets: [],
         percents: { gzip: 0.3333333333333333, stat: 0.02850877192982456 },
@@ -332,6 +334,7 @@ describe('BuildComparator', () => {
         type: 'totalDelta',
         name: 'stuff',
         hashChanged: true,
+        hashChangeUnexpected: false,
         budgets: [],
         failingBudgets: [],
         percents: { gzip: -0.6814814814814815, stat: -0.7875647668393783 },