[RPP] Add tests for insight sorting

Also "help" sorting by INP savings in the case where there is no
observed INP, by using a default INP score for purposes of normalizing
estimated savings.

Bug: 368135130
Change-Id: I5d396ecca8758ab39c86916b5e708f8a213143ef
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6094001
Reviewed-by: Paul Irish <[email protected]>
Commit-Queue: Connor Clark <[email protected]>

Author: Connor Clark

front_end/models/trace/Processor.test.ts

@@ -365,5 +365,67 @@ describeWithEnvironment('TraceProcessor', function() {
       assert.strictEqual(insights[2].model.RenderBlocking.renderBlockingRequests.length, 0);
       assert.strictEqual(insights[3].model.RenderBlocking.renderBlockingRequests.length, 1);
     });
+
+    it('sorts insights by estimated savings and field data', async function() {
+      const getInsightOrder = async (includeMetadata: boolean) => {
+        const processor = Trace.Processor.TraceProcessor.createWithAllHandlers();
+        const file = await TraceLoader.rawEvents(this, 'image-delivery.json.gz');
+
+        let metadata;
+        if (includeMetadata) {
+          metadata = await TraceLoader.metadata(this, 'image-delivery.json.gz');
+        }
+
+        await processor.parse(file, {isFreshRecording: true, isCPUProfile: false, metadata});
+        if (!processor.insights) {
+          throw new Error('No insights');
+        }
+
+        const insightSet = Array.from(processor.insights.values()).at(-1);
+        if (!insightSet) {
+          throw new Error('No insight set');
+        }
+
+        // It's been sorted already ... but let's add some fake estimated savings and re-sort to
+        // better test the sorting.
+        insightSet.model.CLSCulprits.metricSavings = {CLS: 0.07};
+        processor.sortInsightSet(processor.insights, insightSet, metadata ?? null);
+
+        return Object.keys(insightSet.model);
+      };
+
+      const orderWithoutMetadata = await getInsightOrder(false);
+      assert.deepStrictEqual(orderWithoutMetadata, [
+        'CLSCulprits',
+        'Viewport',
+        'InteractionToNextPaint',
+        'LCPPhases',
+        'LCPDiscovery',
+        'RenderBlocking',
+        'ImageDelivery',
+        'DocumentLatency',
+        'FontDisplay',
+        'DOMSize',
+        'ThirdParties',
+        'SlowCSSSelector',
+      ]);
+
+      const orderWithMetadata = await getInsightOrder(true);
+      // Viewport is first, before CLSCulprits, since the field data produces a higher weight for INP than for CLS.
+      assert.deepStrictEqual(orderWithMetadata, [
+        'Viewport',
+        'CLSCulprits',
+        'InteractionToNextPaint',
+        'LCPPhases',
+        'LCPDiscovery',
+        'RenderBlocking',
+        'ImageDelivery',
+        'DocumentLatency',
+        'FontDisplay',
+        'DOMSize',
+        'ThirdParties',
+        'SlowCSSSelector',
+      ]);
+    });
   });
 });

front_end/models/trace/Processor.ts

@@ -347,8 +347,9 @@ export class TraceProcessor extends EventTarget {
    * Sort the insight models based on the impact of each insight's estimated savings, additionally weighted by the
    * worst metrics according to field data (if present).
    */
-  #sortInsightSet(
-      insights: Insights.Types.TraceInsightSets, insightSet: Insights.Types.InsightSet, options: ParseOptions): void {
+  sortInsightSet(
+      insights: Insights.Types.TraceInsightSets, insightSet: Insights.Types.InsightSet,
+      metadata: Types.File.MetaData|null): void {
     // The initial order of the insights is alphabetical, based on `front_end/models/trace/insights/Models.ts`.
     // The order here provides a baseline that groups insights in a more logical way.
     const baselineOrder: Record<keyof Insights.Types.InsightModels, null> = {
@@ -367,17 +368,21 @@ export class TraceProcessor extends EventTarget {
     };
 
     // Determine the weights for each metric based on field data, utilizing the same scoring curve that Lighthouse uses.
-    const weights = Insights.Common.calculateMetricWeightsForSorting(insightSet, options.metadata ?? null);
+    const weights = Insights.Common.calculateMetricWeightsForSorting(insightSet, metadata);
 
     // Normalize the estimated savings to a single number, weighted by its relative impact
     // to the page experience based on the same scoring curve that Lighthouse uses.
     const observedLcp = Insights.Common.getLCP(insights, insightSet.id)?.value;
-    const observedInp = Insights.Common.getINP(insights, insightSet.id)?.value;
     const observedCls = Insights.Common.getCLS(insights, insightSet.id).value;
+
+    // INP is special - if users did not interact with the page, we'll have no INP, but we should still
+    // be able to prioritize insights based on this metric. When we observe no interaction, instead use
+    // a default value for the baseline INP.
+    const observedInp = Insights.Common.getINP(insights, insightSet.id)?.value ?? 200;
+
     const observedLcpScore =
         observedLcp !== undefined ? Insights.Common.evaluateLCPMetricScore(observedLcp) : undefined;
-    const observedInpScore =
-        observedInp !== undefined ? Insights.Common.evaluateINPMetricScore(observedInp) : undefined;
+    const observedInpScore = Insights.Common.evaluateINPMetricScore(observedInp);
     const observedClsScore = Insights.Common.evaluateCLSMetricScore(observedCls);
 
     const insightToSortingRank = new Map<string, number>();
@@ -387,17 +392,17 @@ export class TraceProcessor extends EventTarget {
       const cls = model.metricSavings?.CLS ?? 0;
 
       const lcpPostSavings = observedLcp !== undefined ? Math.max(0, observedLcp - lcp) : undefined;
-      const inpPostSavings = observedInp !== undefined ? Math.max(0, observedInp - inp) : undefined;
-      const clsPostSavings = observedCls !== undefined ? Math.max(0, observedCls - cls) : undefined;
+      const inpPostSavings = Math.max(0, observedInp - inp);
+      const clsPostSavings = Math.max(0, observedCls - cls);
 
       let score = 0;
       if (weights.lcp && lcp && observedLcpScore !== undefined && lcpPostSavings !== undefined) {
         score += weights.lcp * (Insights.Common.evaluateLCPMetricScore(lcpPostSavings) - observedLcpScore);
       }
-      if (weights.inp && inp && observedInpScore !== undefined && inpPostSavings !== undefined) {
+      if (weights.inp && inp && observedInpScore !== undefined) {
         score += weights.inp * (Insights.Common.evaluateINPMetricScore(inpPostSavings) - observedInpScore);
       }
-      if (weights.cls && cls && observedClsScore !== undefined && clsPostSavings !== undefined) {
+      if (weights.cls && cls && observedClsScore !== undefined) {
         score += weights.cls * (Insights.Common.evaluateCLSMetricScore(clsPostSavings) - observedClsScore);
       }
 
@@ -476,7 +481,7 @@ export class TraceProcessor extends EventTarget {
       model,
     };
     insights.set(insightSet.id, insightSet);
-    this.#sortInsightSet(insights, insightSet, options);
+    this.sortInsightSet(insights, insightSet, options.metadata ?? null);
   }
 
   /**

front_end/models/trace/insights/BUILD.gn

@@ -50,6 +50,7 @@ ts_library("unittests") {
 
   sources = [
     "CLSCulprits.test.ts",
+    "Common.test.ts",
     "DOMSize.test.ts",
     "DocumentLatency.test.ts",
     "FontDisplay.test.ts",
@@ -59,6 +60,7 @@ ts_library("unittests") {
     "LCPPhases.test.ts",
     "RenderBlocking.test.ts",
     "SlowCSSSelector.test.ts",
+    "Statistics.test.ts",
     "ThirdParties.test.ts",
     "Viewport.test.ts",
   ]

front_end/models/trace/insights/CLSCulprits.test.ts

@@ -3,22 +3,12 @@
 // found in the LICENSE file.
 
 import {describeWithEnvironment} from '../../../testing/EnvironmentHelpers.js';
-import {getFirstOrError, getInsightOrError} from '../../../testing/InsightHelpers.js';
-import {TraceLoader} from '../../../testing/TraceLoader.js';
+import {getFirstOrError, getInsightOrError, processTrace} from '../../../testing/InsightHelpers.js';
 import * as Helpers from '../helpers/helpers.js';
 import * as Types from '../types/types.js';
 
 import {Models} from './insights.js';
 
-export async function processTrace(testContext: Mocha.Suite|Mocha.Context|null, traceFile: string) {
-  const {parsedTrace, insights} = await TraceLoader.traceEngine(testContext, traceFile);
-  if (!insights) {
-    throw new Error('No insights');
-  }
-
-  return {data: parsedTrace, insights};
-}
-
 // Root cause invalidation window.
 const INVALIDATION_WINDOW = Helpers.Timing.secondsToMicroseconds(Types.Timing.Seconds(0.5));
 

front_end/models/trace/insights/Common.test.ts

@@ -0,0 +1,52 @@
+// Copyright 2024 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import {describeWithEnvironment} from '../../../testing/EnvironmentHelpers.js';
+import {getFirstOrError, processTrace} from '../../../testing/InsightHelpers.js';
+
+import * as Insights from './insights.js';
+
+const {calculateMetricWeightsForSorting} = Insights.Common;
+
+describeWithEnvironment('Common', function() {
+  describe('calculateMetricWeightsForSorting', () => {
+    async function process(testContext: Mocha.Suite|Mocha.Context|null, traceFile: string) {
+      const {data, insights, metadata} = await processTrace(testContext, traceFile);
+      if (!metadata) {
+        throw new Error('missing metadata');
+      }
+
+      const insightSetKey = getFirstOrError(data.Meta.navigationsByNavigationId.values()).args.data.navigationId;
+      const insightSet = insights.get(insightSetKey);
+      if (!insightSet) {
+        throw new Error('missing insight set');
+      }
+
+      // Clone so it may be modified.
+      const clonedMetadata = structuredClone(metadata);
+
+      return {insightSet, metadata: clonedMetadata};
+    }
+
+    it('returns default weights when there is no field data', async () => {
+      const {insightSet, metadata} = await process(this, 'image-delivery.json.gz');
+
+      // No field data defaults to even split of weights.
+      metadata.cruxFieldData = undefined;
+      let weights = calculateMetricWeightsForSorting(insightSet, metadata);
+      assert.deepStrictEqual(weights, {lcp: 1 / 3, inp: 1 / 3, cls: 1 / 3});
+
+      metadata.cruxFieldData = [];
+      weights = calculateMetricWeightsForSorting(insightSet, metadata);
+      assert.deepStrictEqual(weights, {lcp: 1 / 3, inp: 1 / 3, cls: 1 / 3});
+    });
+
+    it('returns weights based on field data', async () => {
+      const {insightSet, metadata} = await process(this, 'image-delivery.json.gz');
+
+      const weights = calculateMetricWeightsForSorting(insightSet, metadata);
+      assert.deepStrictEqual(weights, {lcp: 0.07778127820223579, inp: 0.5504200439526509, cls: 0.37179867784511333});
+    });
+  });
+});

front_end/models/trace/insights/DOMSize.test.ts

@@ -3,17 +3,7 @@
 // found in the LICENSE file.
 
 import {describeWithEnvironment} from '../../../testing/EnvironmentHelpers.js';
-import {getFirstOrError, getInsightOrError} from '../../../testing/InsightHelpers.js';
-import {TraceLoader} from '../../../testing/TraceLoader.js';
-
-export async function processTrace(testContext: Mocha.Suite|Mocha.Context|null, traceFile: string) {
-  const {parsedTrace, insights} = await TraceLoader.traceEngine(testContext, traceFile);
-  if (!insights) {
-    throw new Error('No insights');
-  }
-
-  return {data: parsedTrace, insights};
-}
+import {getFirstOrError, getInsightOrError, processTrace} from '../../../testing/InsightHelpers.js';
 
 describeWithEnvironment('DOMSize', function() {
   it('finds layout reflows and style recalcs affected by DOM size',

front_end/models/trace/insights/DocumentLatency.test.ts

@@ -3,20 +3,16 @@
 // found in the LICENSE file.
 
 import {describeWithEnvironment} from '../../../testing/EnvironmentHelpers.js';
-import {createContextForNavigation, getFirstOrError, getInsightOrError} from '../../../testing/InsightHelpers.js';
+import {
+  createContextForNavigation,
+  getFirstOrError,
+  getInsightOrError,
+  processTrace,
+} from '../../../testing/InsightHelpers.js';
 import {TraceLoader} from '../../../testing/TraceLoader.js';
 import * as Trace from '../trace.js';
 import * as Types from '../types/types.js';
 
-export async function processTrace(testContext: Mocha.Suite|Mocha.Context|null, traceFile: string) {
-  const {parsedTrace, insights} = await TraceLoader.traceEngine(testContext, traceFile);
-  if (!insights) {
-    throw new Error('No insights');
-  }
-
-  return {data: parsedTrace, insights};
-}
-
 describeWithEnvironment('DocumentLatency', function() {
   it('reports savings for main document with redirects', async () => {
     const {data, insights} = await processTrace(this, 'lantern/redirect/trace.json.gz');

front_end/models/trace/insights/FontDisplay.test.ts

@@ -3,17 +3,7 @@
 // found in the LICENSE file.
 
 import {describeWithEnvironment} from '../../../testing/EnvironmentHelpers.js';
-import {getFirstOrError, getInsightOrError} from '../../../testing/InsightHelpers.js';
-import {TraceLoader} from '../../../testing/TraceLoader.js';
-
-export async function processTrace(testContext: Mocha.Suite|Mocha.Context|null, traceFile: string) {
-  const {parsedTrace, insights} = await TraceLoader.traceEngine(testContext, traceFile);
-  if (!insights) {
-    throw new Error('No insights');
-  }
-
-  return {data: parsedTrace, insights};
-}
+import {getFirstOrError, getInsightOrError, processTrace} from '../../../testing/InsightHelpers.js';
 
 describeWithEnvironment('FontDisplay', function() {
   it('finds no requests for remote fonts', async () => {

front_end/models/trace/insights/ImageDelivery.test.ts

@@ -3,22 +3,12 @@
 // found in the LICENSE file.
 
 import {describeWithEnvironment} from '../../../testing/EnvironmentHelpers.js';
-import {getFirstOrError, getInsightOrError} from '../../../testing/InsightHelpers.js';
-import {TraceLoader} from '../../../testing/TraceLoader.js';
+import {getFirstOrError, getInsightOrError, processTrace} from '../../../testing/InsightHelpers.js';
 
 import * as Insights from './insights.js';
 
 const {ImageOptimizationType} = Insights.Models.ImageDelivery;
 
-export async function processTrace(testContext: Mocha.Suite|Mocha.Context|null, traceFile: string) {
-  const {parsedTrace, insights} = await TraceLoader.traceEngine(testContext, traceFile);
-  if (!insights) {
-    throw new Error('No insights');
-  }
-
-  return {data: parsedTrace, insights};
-}
-
 describeWithEnvironment('ImageDelivery', function() {
   it('finds requests for remote fonts', async () => {
     // See the following for a description of each test case:

front_end/models/trace/insights/InteractionToNextPaint.test.ts

@@ -3,19 +3,9 @@
 // found in the LICENSE file.
 
 import {describeWithEnvironment} from '../../../testing/EnvironmentHelpers.js';
-import {createContextForNavigation, getFirst} from '../../../testing/InsightHelpers.js';
-import {TraceLoader} from '../../../testing/TraceLoader.js';
+import {createContextForNavigation, getFirst, processTrace} from '../../../testing/InsightHelpers.js';
 import * as Trace from '../trace.js';
 
-export async function processTrace(testContext: Mocha.Suite|Mocha.Context|null, traceFile: string) {
-  const {parsedTrace, insights} = await TraceLoader.traceEngine(testContext, traceFile);
-  if (!insights) {
-    throw new Error('No insights');
-  }
-
-  return {data: parsedTrace, insights};
-}
-
 describeWithEnvironment('InteractionToNextPaint', function() {
   const test = (traceFile: string, longest?: number, highPercentile?: number) => {
     if (highPercentile === undefined) {