Repository: GoogleChrome/lighthouse
Pull request #9895 (2019-10-28 18:28:25)

core(lightwallet): refactor budget matching logic
**Summary**
- Moves the logic for determining the matching budget to `budget.js`. In a subsequent PR, the `'performance-budget'` audit will be split into `'resource-budget'` and `'timing-budget`', so this refactor allows the logic to be re-used across both audits.
- Minor fix: Use deep copy instead of shallow copy when finding matching budget.

khempenius: @brendankenny We probably don’t want it to be possible to mutate the objects in context.settings.budgets. Currently, everything would still work w/ shallow copy, but I think switching to deep copy is a good safeguard to prevent accidental mutation from happening in the future.
brendankenny: > @brendankenny We probably don’t want it to be possible to mutate the objects in context.settings.budgets. Currently, everything would still work w/ shallow copy, but I think switching to deep copy is a good safeguard to prevent accidental mutation from happening in the future.

I agree in general, but that seems better to fix at the `context.settings` or `context.settings.budgets` level (deep `Object.freeze` or `readonly` type system checks or whatever) rather than have functions each handling settings immutability in different ways.
brendankenny: To clarify, it seems like there isn't a specific need (or worry) for this data to be a copy of the original budget, just a general desire to not have audits or others modifying the core settings.

That's reasonable, but the tragedy of the commons/end game is bespoke cloning for every method touching the settings, and future efforts to refactor to a centralized solution will have to parse why each function was doing what it was (e.g. in this case, was it cloning because callers are meant to modify it, or cloning just for safety).

Absent a specific threat, my preference would be to open a "we should make settings/context immutable" issue and skip on extra `JSON.parse(JSON.stringify())` cloning here, but that's not a blocker if you feel strongly the other way :)
khempenius: @brendankenny SGTM. I'll open an issue for that.

Patch:
diff --git a/lighthouse-core/audits/performance-budget.js b/lighthouse-core/audits/performance-budget.js
index 14f357add..98ab5e5b1 100644
--- a/lighthouse-core/audits/performance-budget.js
+++ b/lighthouse-core/audits/performance-budget.js
@@ -124,13 +124,7 @@ class ResourceBudget extends Audit {
     const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
     const summary = await ResourceSummary.request({devtoolsLog, URL: artifacts.URL}, context);
     const mainResource = await MainResource.request({URL: artifacts.URL, devtoolsLog}, context);
-    // Clones budget so that the user-supplied version is not mutated.
-    /** @type {Array<LH.Budget>} */
-    const budgets = Array.from(context.settings.budgets || []);
-    // Applies the LAST matching budget
-    const budget = budgets ? budgets.reverse().find((b) => {
-      return Budget.urlMatchesPattern(mainResource.url, b.path);
-    }) : undefined;
+    const budget = Budget.getMatchingBudget(context.settings.budgets, mainResource.url);
 
     if (!budget) {
       return {
diff --git a/lighthouse-core/config/budget.js b/lighthouse-core/config/budget.js
index c5f950987..be478f751 100644
--- a/lighthouse-core/config/budget.js
+++ b/lighthouse-core/config/budget.js
@@ -132,6 +132,26 @@ class Budget {
     return path;
   }
 
+  /**
+   * Returns the budget that applies to a given URL.
+   * If multiple budgets match based on thier 'path' property,
+   * then the last-listed of those budgets is returned.
+   * @param {Array<LH.Budget>|null} budgets
+   * @param {string} url
+   * @return {LH.Budget | undefined} budget
+   */
+  static getMatchingBudget(budgets, url) {
+    if (budgets === null) return;
+
+    // Applies the LAST matching budget.
+    for (let i = budgets.length - 1; i >= 0; i--) {
+      const budget = budgets[i];
+      if (this.urlMatchesPattern(url, budget.path)) {
+        return budget;
+      }
+    }
+  }
+
   /**
    * Determines whether a URL matches against a robots.txt-style "path".
    * Pattern should use the robots.txt format. E.g. "/*-article.html" or "/". Reference:


Test patch:
diff --git a/lighthouse-core/test/audits/performance-budget-test.js b/lighthouse-core/test/audits/performance-budget-test.js
index 1aafcdb19..4d2bf4c87 100644
--- a/lighthouse-core/test/audits/performance-budget-test.js
+++ b/lighthouse-core/test/audits/performance-budget-test.js
@@ -100,13 +100,6 @@ describe('Performance: Resource budgets audit', () => {
       });
     });
 
-    it('does not mutate the budget config', async () => {
-      const configBefore = JSON.parse(JSON.stringify(context.settings.budgets));
-      await ResourceBudgetAudit.audit(artifacts, context);
-      const configAfter = JSON.parse(JSON.stringify(context.settings.budgets));
-      expect(configBefore).toEqual(configAfter);
-    });
-
     it('only includes rows for resource types with budgets', async () => {
       const result = await ResourceBudgetAudit.audit(artifacts, context);
       expect(result.details.items).toHaveLength(2);
@@ -136,8 +129,11 @@ describe('Performance: Resource budgets audit', () => {
         expect(item.size).toBeGreaterThanOrEqual(items[index + 1].size);
       });
     });
-    describe('budget path', () => {
-      it('applies the last matching budget', async () => {
+  });
+
+  describe('budget selection', () => {
+    describe('with a matching budget', () => {
+      it('applies the correct budget', async () => {
         context.settings.budgets = [{
           path: '/',
           resourceSizes: [
@@ -169,7 +165,10 @@ describe('Performance: Resource budgets audit', () => {
         const result = await ResourceBudgetAudit.audit(artifacts, context);
         expect(result.details.items[0].resourceType).toBe('image');
       });
-      it('returns "audit does not apply" if no budget matches', async () => {
+    });
+
+    describe('without a matching budget', () => {
+      it('returns "audit does not apply"', async () => {
         context.settings.budgets = [{
           path: '/not-a-match',
           resourceSizes: [
@@ -185,17 +184,17 @@ describe('Performance: Resource budgets audit', () => {
         expect(result.notApplicable).toBe(true);
       });
     });
-  });
 
-  describe('without a budget.json', () => {
-    beforeEach(() => {
-      context.settings.budgets = null;
-    });
+    describe('without a budget.json', () => {
+      beforeEach(() => {
+        context.settings.budgets = null;
+      });
 
-    it('audit does not apply', async () => {
-      const result = await ResourceBudgetAudit.audit(artifacts, context);
-      expect(result.details).toBeUndefined();
-      expect(result.notApplicable).toBe(true);
+      it('returns "audit does not apply"', async () => {
+        const result = await ResourceBudgetAudit.audit(artifacts, context);
+        expect(result.details).toBeUndefined();
+        expect(result.notApplicable).toBe(true);
+      });
     });
   });
 });
diff --git a/lighthouse-core/test/config/budget-test.js b/lighthouse-core/test/config/budget-test.js
index 7b5b83683..c096e0ffb 100644
--- a/lighthouse-core/test/config/budget-test.js
+++ b/lighthouse-core/test/config/budget-test.js
@@ -197,6 +197,53 @@ describe('Budget', () => {
     });
   });
 
+  describe('budget matching', () => {
+    const budgets = [{
+      path: '/',
+      resourceSizes: [
+        {
+          resourceType: 'script',
+          budget: 0,
+        },
+      ],
+    },
+    {
+      path: '/file.html',
+      resourceSizes: [
+        {
+          resourceType: 'image',
+          budget: 0,
+        },
+      ],
+    },
+    {
+      path: '/not-a-match',
+      resourceSizes: [
+        {
+          resourceType: 'document',
+          budget: 0,
+        },
+      ],
+    },
+    ];
+    it('returns the last matching budget', () => {
+      const budget = Budget.getMatchingBudget(budgets, 'http://example.com/file.html');
+      expect(budget).toEqual(budgets[1]);
+    });
+
+    it('does not mutate the budget config', async () => {
+      const configBefore = JSON.parse(JSON.stringify(budgets));
+      Budget.getMatchingBudget(configBefore, 'https://example.com');
+      const configAfter = JSON.parse(JSON.stringify(budgets));
+      expect(configBefore).toEqual(configAfter);
+    });
+
+    it('returns "undefined" when there is no budget config', () => {
+      const budget = Budget.getMatchingBudget(null, 'https://example.com');
+      expect(budget).toEqual(undefined);
+    });
+  });
+
   describe('path validation', () => {
     it('recognizes valid budgets', () => {
       let budgets = [{path: '/'}];