Repository: paularmstrong/build-tracker (ts)

Issue:
Sometimes the markdown table does not show hash changes when the web UI does
# Problem

Sometimes, the _unexpected hash change_ does not show up in the Markdown table, but it does show up in the web UI.

**Steps to Reproduce:**

If one of the delta sizes are zero, but not all, and the filename's hash changes.

**Expected Result**

They are expected to have the same behavior

**Actual Result**

The web UI shows a hash icon, but the markdown table will not


Pull request:
fix(comparator): ensure hash change only shows when all sizes unchanged
<!--
Thank you so much for contributing to open source and the Build Tracker project!
-->

# Problem

There is a discrepency between when the _unexpected hash change_ icon #️⃣is shown in markdown (from comparator) vs the web UI.

# Solution

The web UI had different logic in that it showed the icon if-and-only-if the _current_ size delta is 0, but not if one of the other size keys had a delta. for example, it would show if the gzip size is unchanged, but the stat size has changed. This is incorrect, because we would actually expect the hash to change in that case.

This adds a getter to the `ArtifactDelta` object as a source of truth to know if the hash change was unexpected. This removes the need for duplicative logic in a few places.

Fixes #178 

# 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/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/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));
         }
       });


Test patch:
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 },