Repository: GoogleChrome/lighthouse
Pull request #12873 (2021-08-05 21:21:17)

core(fr): override quiet windows for observed performance
**Summary**
Overrides the quiet windows when throttlingMethod isn't simulate and enables the metrics Fraggle Rock smoke tests to prevent regressions.

**Related Issues/PRs**
ref #12861 #11313 



Patch:
diff --git a/.github/workflows/smoke.yml b/.github/workflows/smoke.yml
index 8db90bf53..1eb3c50a4 100644
--- a/.github/workflows/smoke.yml
+++ b/.github/workflows/smoke.yml
@@ -106,7 +106,7 @@ jobs:
 
     - run: sudo apt-get install xvfb
     - name: yarn smoke --fraggle-rock
-      run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2 a11y lantern seo-passing seo-failing csp
+      run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2 a11y lantern seo-passing seo-failing csp metrics
 
     # Fail if any changes were written to source files.
     - run: git diff --exit-code
diff --git a/lighthouse-core/fraggle-rock/config/config.js b/lighthouse-core/fraggle-rock/config/config.js
index ed8eb804b..99cc3d0be 100644
--- a/lighthouse-core/fraggle-rock/config/config.js
+++ b/lighthouse-core/fraggle-rock/config/config.js
@@ -9,7 +9,7 @@ const path = require('path');
 const log = require('lighthouse-logger');
 const Runner = require('../../runner.js');
 const defaultConfig = require('./default-config.js');
-const {defaultNavigationConfig} = require('../../config/constants.js');
+const {defaultNavigationConfig, nonSimulatedPassConfigOverrides} = require('../../config/constants.js'); // eslint-disable-line max-len
 const {
   isFRGathererDefn,
   throwInvalidDependencyOrder,
@@ -157,13 +157,42 @@ function resolveArtifactsToDefns(artifacts, configDir) {
   return artifactDefns;
 }
 
+/**
+ * Overrides the quiet windows when throttlingMethod requires observation.
+ *
+ * @param {LH.Config.NavigationDefn} navigation
+ * @param {LH.Config.Settings} settings
+ */
+function overrideNavigationThrottlingWindows(navigation, settings) {
+  if (navigation.disableThrottling) return;
+  if (settings.throttlingMethod === 'simulate') return;
+
+  navigation.cpuQuietThresholdMs = Math.max(
+    navigation.cpuQuietThresholdMs || 0,
+    nonSimulatedPassConfigOverrides.cpuQuietThresholdMs
+  );
+  navigation.networkQuietThresholdMs = Math.max(
+    navigation.networkQuietThresholdMs || 0,
+    nonSimulatedPassConfigOverrides.networkQuietThresholdMs
+  );
+  navigation.pauseAfterFcpMs = Math.max(
+    navigation.pauseAfterFcpMs || 0,
+    nonSimulatedPassConfigOverrides.pauseAfterFcpMs
+  );
+  navigation.pauseAfterLoadMs = Math.max(
+    navigation.pauseAfterLoadMs || 0,
+    nonSimulatedPassConfigOverrides.pauseAfterLoadMs
+  );
+}
+
 /**
  *
  * @param {LH.Config.NavigationJson[]|null|undefined} navigations
  * @param {LH.Config.AnyArtifactDefn[]|null|undefined} artifactDefns
+ * @param {LH.Config.Settings} settings
  * @return {LH.Config.NavigationDefn[] | null}
  */
-function resolveNavigationsToDefns(navigations, artifactDefns) {
+function resolveNavigationsToDefns(navigations, artifactDefns, settings) {
   if (!navigations) return null;
   if (!artifactDefns) throw new Error('Cannot use navigations without defining artifacts');
 
@@ -181,9 +210,9 @@ function resolveNavigationsToDefns(navigations, artifactDefns) {
       return artifact;
     });
 
-    // TODO(FR-COMPAT): enforce navigation throttling invariants
-
-    return {...navigationWithDefaults, artifacts};
+    const resolvedNavigation = {...navigationWithDefaults, artifacts};
+    overrideNavigationThrottlingWindows(resolvedNavigation, settings);
+    return resolvedNavigation;
   });
 
   assertArtifactTopologicalOrder(navigationDefns);
@@ -209,7 +238,7 @@ function initializeConfig(configJSON, context) {
 
   const settings = resolveSettings(configWorkingCopy.settings || {}, context.settingsOverrides);
   const artifacts = resolveArtifactsToDefns(configWorkingCopy.artifacts, configDir);
-  const navigations = resolveNavigationsToDefns(configWorkingCopy.navigations, artifacts);
+  const navigations = resolveNavigationsToDefns(configWorkingCopy.navigations, artifacts, settings);
 
   /** @type {LH.Config.FRConfig} */
   let config = {
diff --git a/report/renderer/dom.js b/report/renderer/dom.js
index 8b3894300..550f41dc7 100644
--- a/report/renderer/dom.js
+++ b/report/renderer/dom.js
@@ -154,6 +154,9 @@ export class DOM {
    * @param {string} url
    */
   safelySetHref(elem, url) {
+    // Defaults to '' to fix proto roundtrip issue. See https://github.com/GoogleChrome/lighthouse/issues/12868
+    url = url || '';
+
     // In-page anchor links are safe.
     if (url.startsWith('#')) {
       elem.href = url;


Test patch:
diff --git a/lighthouse-core/test/fraggle-rock/config/config-test.js b/lighthouse-core/test/fraggle-rock/config/config-test.js
index 3d73b666e..98ff41b95 100644
--- a/lighthouse-core/test/fraggle-rock/config/config-test.js
+++ b/lighthouse-core/test/fraggle-rock/config/config-test.js
@@ -6,6 +6,7 @@
 'use strict';
 
 const BaseAudit = require('../../../audits/audit.js');
+const {nonSimulatedPassConfigOverrides} = require('../../../config/constants.js');
 const BaseGatherer = require('../../../fraggle-rock/gather/base-gatherer.js');
 const {initializeConfig} = require('../../../fraggle-rock/config/config.js');
 
@@ -68,61 +69,6 @@ describe('Fraggle Rock Config', () => {
     expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/HTTPRedirect gatherer/);
   });
 
-  it('should resolve navigation definitions', () => {
-    gatherMode = 'navigation';
-    const configJson = {
-      artifacts: [{id: 'Accessibility', gatherer: 'accessibility'}],
-      navigations: [{id: 'default', artifacts: ['Accessibility']}],
-    };
-    const {config} = initializeConfig(configJson, {gatherMode});
-
-    expect(config).toMatchObject({
-      artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}],
-      navigations: [
-        {id: 'default', artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}]},
-      ],
-    });
-  });
-
-  it('should throw when navigations are defined without artifacts', () => {
-    const configJson = {
-      navigations: [{id: 'default', artifacts: ['Accessibility']}],
-    };
-
-    expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/Cannot use navigations/);
-  });
-
-  it('should throw when navigations use unrecognized artifacts', () => {
-    const configJson = {
-      artifacts: [],
-      navigations: [{id: 'default', artifacts: ['Accessibility']}],
-    };
-
-    expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/Unrecognized artifact/);
-  });
-
-  it('should set default properties on navigations', () => {
-    gatherMode = 'navigation';
-    const configJson = {
-      artifacts: [{id: 'Accessibility', gatherer: 'accessibility'}],
-      navigations: [{id: 'default', artifacts: ['Accessibility']}],
-    };
-    const {config} = initializeConfig(configJson, {gatherMode});
-
-    expect(config).toMatchObject({
-      navigations: [
-        {
-          id: 'default',
-          blankPage: 'about:blank',
-          artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}],
-          disableThrottling: false,
-          networkQuietThresholdMs: 0,
-          cpuQuietThresholdMs: 0,
-        },
-      ],
-    });
-  });
-
   it('should filter configuration by gatherMode', () => {
     const timespanGatherer = new BaseGatherer();
     timespanGatherer.meta = {supportedModes: ['timespan']};
@@ -263,6 +209,94 @@ describe('Fraggle Rock Config', () => {
     });
   });
 
+  describe('.resolveNavigationsToDefns', () => {
+    it('should resolve navigation definitions', () => {
+      gatherMode = 'navigation';
+      const configJson = {
+        artifacts: [{id: 'Accessibility', gatherer: 'accessibility'}],
+        navigations: [{id: 'default', artifacts: ['Accessibility']}],
+      };
+      const {config} = initializeConfig(configJson, {gatherMode});
+
+      expect(config).toMatchObject({
+        artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}],
+        navigations: [
+          {id: 'default', artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}]},
+        ],
+      });
+    });
+
+    it('should throw when navigations are defined without artifacts', () => {
+      const configJson = {
+        navigations: [{id: 'default', artifacts: ['Accessibility']}],
+      };
+
+      expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/Cannot use navigations/);
+    });
+
+    it('should throw when navigations use unrecognized artifacts', () => {
+      const configJson = {
+        artifacts: [],
+        navigations: [{id: 'default', artifacts: ['Accessibility']}],
+      };
+
+      expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/Unrecognized artifact/);
+    });
+
+    it('should set default properties on navigations', () => {
+      gatherMode = 'navigation';
+      const configJson = {
+        artifacts: [{id: 'Accessibility', gatherer: 'accessibility'}],
+        navigations: [{id: 'default', artifacts: ['Accessibility']}],
+      };
+      const {config} = initializeConfig(configJson, {gatherMode});
+
+      expect(config).toMatchObject({
+        navigations: [
+          {
+            id: 'default',
+            blankPage: 'about:blank',
+            artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}],
+            loadFailureMode: 'fatal',
+            disableThrottling: false,
+            networkQuietThresholdMs: 0,
+            cpuQuietThresholdMs: 0,
+          },
+        ],
+      });
+    });
+
+    it('should ensure minimum quiet thresholds when throttlingMethod is devtools', () => {
+      gatherMode = 'navigation';
+      const configJson = {
+        artifacts: [{id: 'Accessibility', gatherer: 'accessibility'}],
+        navigations: [
+          {id: 'default', artifacts: ['Accessibility']},
+          {id: 'noThrottling', artifacts: ['Accessibility'], disableThrottling: true},
+          {id: 'alreadyHigh', artifacts: ['Accessibility'], cpuQuietThresholdMs: 10_000},
+        ],
+      };
+
+      const {config} = initializeConfig(configJson, {
+        gatherMode,
+        settingsOverrides: {throttlingMethod: 'devtools'},
+      });
+
+      expect(config).toMatchObject({
+        navigations: [
+          {
+            pauseAfterFcpMs: nonSimulatedPassConfigOverrides.pauseAfterFcpMs,
+            pauseAfterLoadMs: nonSimulatedPassConfigOverrides.pauseAfterLoadMs,
+            networkQuietThresholdMs: nonSimulatedPassConfigOverrides.networkQuietThresholdMs,
+            cpuQuietThresholdMs: nonSimulatedPassConfigOverrides.cpuQuietThresholdMs,
+          },
+          {networkQuietThresholdMs: 0, cpuQuietThresholdMs: 0},
+          {cpuQuietThresholdMs: 10_000},
+        ],
+      });
+    });
+  });
+
   describe('.resolveExtensions', () => {
     /** @type {LH.Config.Json} */
     let extensionConfig;
diff --git a/report/test/renderer/dom-test.js b/report/test/renderer/dom-test.js
index aadf62802..867c99f14 100644
--- a/report/test/renderer/dom-test.js
+++ b/report/test/renderer/dom-test.js
@@ -193,8 +193,13 @@ describe('DOM', () => {
         expect(a.getAttribute('href')).toEqual(null);
       });
     });
-  });
 
+    it('handles if url is undefined (because of proto roundtrip)', () => {
+      const a = dom.createElement('a');
+      dom.safelySetHref(a, undefined);
+      expect(a.href).toEqual('');
+    });
+  });
 
   describe('safelySetBlobHref', () => {
     it('sets href for safe blob types', () => {