core(fr): collect devtoolsLogs on pageLoadError (#12980)

GoogleChrome · Aug 25, 2021 · 43dcb02 · 43dcb02
1 parent bf7cc5e
commit 43dcb02
Show file tree

Hide file tree

Showing 8 changed files with 32 additions and 26 deletions.
diff --git 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 --invert-match pwa offline oopif errors-infinite-loop errors-expired-ssl perf-diagnostics-third-party
+      run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2 --invert-match pwa offline oopif perf-diagnostics-third-party
 
     # Fail if any changes were written to source files.
     - run: git diff --exit-code

diff --git a/lighthouse-cli/test/smokehouse/report-assert.js b/lighthouse-cli/test/smokehouse/report-assert.js
@@ -290,12 +290,7 @@ function collateResults(localConsole, actual, expected) {
   }
 
   return [
-    {
-      name: 'final url',
-      actual: actual.lhr.finalUrl,
-      expected: expected.lhr.finalUrl,
-      equal: actual.lhr.finalUrl === expected.lhr.finalUrl,
-    },
+    makeComparison('final url', actual.lhr.finalUrl, expected.lhr.finalUrl),
     runtimeErrorAssertion,
     runWarningsAssertion,
     ...requestCountAssertion,

diff --git a/lighthouse-cli/test/smokehouse/test-definitions/errors/error-expectations.js b/lighthouse-cli/test/smokehouse/test-definitions/errors/error-expectations.js
@@ -31,9 +31,12 @@ const infiniteLoop = {
   artifacts: {
     PageLoadError: {code: 'PAGE_HUNG'},
     devtoolsLogs: {
-      'pageLoadError-defaultPass': NONEMPTY_ARRAY,
+      'pageLoadError-defaultPass': {...NONEMPTY_ARRAY, _legacyOnly: true},
+      'pageLoadError-default': {...NONEMPTY_ARRAY, _fraggleRockOnly: true},
     },
     traces: {
+      // Fraggle Rock treats traces as regular artifacts which are not collected on error.
+      '_legacyOnly': true,
       'pageLoadError-defaultPass': {traceEvents: NONEMPTY_ARRAY},
     },
   },
@@ -46,9 +49,12 @@ const infiniteLoop = {
 const expiredSsl = {
   lhr: {
     requestedUrl: 'https://expired.badssl.com',
-    finalUrl: 'https://expired.badssl.com/',
+    finalUrl: /(expired.badssl.com|chrome-error)/,
     runtimeError: {code: 'INSECURE_DOCUMENT_REQUEST'},
-    runWarnings: ['The URL you have provided does not have a valid security certificate. net::ERR_CERT_DATE_INVALID'],
+    runWarnings: Object.defineProperty([
+      /expired.badssl.*redirected to chrome-error:/, // This warning was not provided in legacy reports.
+      'The URL you have provided does not have a valid security certificate. net::ERR_CERT_DATE_INVALID',
+    ], '_fraggleRockOnly', {value: true, enumerable: true}),
     audits: {
       'first-contentful-paint': {
         scoreDisplayMode: 'error',
@@ -59,9 +65,12 @@ const expiredSsl = {
   artifacts: {
     PageLoadError: {code: 'INSECURE_DOCUMENT_REQUEST'},
     devtoolsLogs: {
-      'pageLoadError-defaultPass': NONEMPTY_ARRAY,
+      'pageLoadError-defaultPass': {...NONEMPTY_ARRAY, _legacyOnly: true},
+      'pageLoadError-default': {...NONEMPTY_ARRAY, _fraggleRockOnly: true},
     },
     traces: {
+      // Fraggle Rock treats traces as regular artifacts which are not collected on error.
+      '_legacyOnly': true,
       'pageLoadError-defaultPass': {traceEvents: NONEMPTY_ARRAY},
     },
   },

diff --git a/lighthouse-core/fraggle-rock/gather/navigation-runner.js b/lighthouse-core/fraggle-rock/gather/navigation-runner.js
@@ -105,9 +105,9 @@ async function _navigate(navigationContext) {
 /**
  * @param {NavigationContext} navigationContext
  * @param {PhaseState} phaseState
- * @return {Promise<Array<LH.Artifacts.NetworkRequest> | undefined>}
+ * @return {Promise<{devtoolsLog: LH.DevtoolsLog, records: Array<LH.Artifacts.NetworkRequest>} | undefined>}
  */
-async function _collectNetworkRecords(navigationContext, phaseState) {
+async function _collectNetworkData(navigationContext, phaseState) {
   const devtoolsLogArtifactDefn = phaseState.artifactDefinitions.find(
     definition => definition.gatherer.instance.meta.symbol === DevtoolsLog.symbol
   );
@@ -119,7 +119,7 @@ async function _collectNetworkRecords(navigationContext, phaseState) {
 
   const devtoolsLog = await phaseState.artifactState.getArtifact[devtoolsLogArtifactId];
   const records = await NetworkRecords.request(devtoolsLog, navigationContext);
-  return records;
+  return {devtoolsLog, records};
 }
 
 /**
@@ -137,12 +137,12 @@ async function _computeNavigationResult(
 ) {
   const {navigationError, finalUrl} = navigateResult;
   const warnings = [...setupResult.warnings, ...navigateResult.warnings];
-  const networkRecords = await _collectNetworkRecords(navigationContext, phaseState);
-  const pageLoadError = networkRecords
+  const networkData = await _collectNetworkData(navigationContext, phaseState);
+  const pageLoadError = networkData
     ? getPageLoadError(navigationError, {
       url: finalUrl,
       loadFailureMode: navigationContext.navigation.loadFailureMode,
-      networkRecords,
+      networkRecords: networkData.records,
     })
     : navigationError;
 
@@ -151,10 +151,15 @@ async function _computeNavigationResult(
     const localizedMessage = i18n.getFormatted(pageLoadError.friendlyMessage, locale);
     log.error('NavigationRunner', localizedMessage, navigationContext.requestedUrl);
 
+    /** @type {Partial<LH.GathererArtifacts>} */
+    const artifacts = {};
+    const pageLoadErrorId = `pageLoadError-${navigationContext.navigation.id}`;
+    if (networkData) artifacts.devtoolsLogs = {[pageLoadErrorId]: networkData.devtoolsLog};
+
     return {
       finalUrl,
       pageLoadError,
-      artifacts: {},
+      artifacts,
       warnings: [...warnings, pageLoadError.friendlyMessage],
     };
   } else {

diff --git a/lighthouse-core/gather/driver/navigation.js b/lighthouse-core/gather/driver/navigation.js
@@ -142,10 +142,7 @@ function getNavigationWarnings(navigation) {
 
   if (navigation.timedOut) warnings.push(str_(UIStrings.warningTimeout));
 
-  if (
-    !URL.equalWithExcludedFragments(requestedUrl, finalUrl) &&
-    !finalUrl.startsWith('chrome-error://')
-  ) {
+  if (!URL.equalWithExcludedFragments(requestedUrl, finalUrl)) {
     warnings.push(str_(UIStrings.warningRedirected, {
       requested: requestedUrl,
       final: finalUrl,

diff --git a/lighthouse-core/test/fraggle-rock/gather/navigation-runner-test.js b/lighthouse-core/test/fraggle-rock/gather/navigation-runner-test.js
@@ -336,7 +336,7 @@ describe('NavigationRunner', () => {
 
       const {artifacts, pageLoadError} = await run(navigation);
       expect(pageLoadError).toBeInstanceOf(LighthouseError);
-      expect(artifacts).toEqual({});
+      expect(artifacts).toEqual({devtoolsLogs: {'pageLoadError-default': expect.any(Array)}});
     });
 
     it('cleans up throttling before getArtifact', async () => {

diff --git a/lighthouse-core/test/gather/driver/navigation-test.js b/lighthouse-core/test/gather/driver/navigation-test.js
@@ -234,9 +234,9 @@ describe('.getNavigationWarnings()', () => {
     expect(warnings).toHaveLength(0);
   });
 
-  it('does not add a url mismatch warning for failed navigations', () => {
+  it('adds a url mismatch warning for failed navigations', () => {
     const finalUrl = 'chrome-error://chromewebdata/';
     const warnings = getNavigationWarnings({...normalNavigation, finalUrl});
-    expect(warnings).toHaveLength(0);
+    expect(warnings).toHaveLength(1);
   });
 });
diff --git a/types/smokehouse.d.ts b/types/smokehouse.d.ts
@@ -13,7 +13,7 @@ declare global {
     interface ExpectedLHR {
       audits: Record<string, any>;
       requestedUrl: string;
-      finalUrl: string;
+      finalUrl: string | RegExp;
       runWarnings?: Array<string|RegExp>;
       runtimeError?: {
         code?: any;