From a9ae6a40fd10c08a2dfaca40a3374a879ca03eaf Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Wed, 26 Jul 2023 13:24:44 -0700 Subject: [PATCH 1/4] core: remove error pass id --- .../test-definitions/errors-expired-ssl.js | 10 +++------- .../test-definitions/errors-iframe-expired-ssl.js | 8 ++------ .../test-definitions/errors-infinite-loop.js | 10 +++------- core/config/constants.js | 2 +- core/config/default-config.js | 8 ++++---- core/gather/navigation-runner.js | 12 +++++++++--- core/runner.js | 2 ++ core/test/gather/navigation-runner-test.js | 6 ++++-- 8 files changed, 28 insertions(+), 30 deletions(-) diff --git a/cli/test/smokehouse/test-definitions/errors-expired-ssl.js b/cli/test/smokehouse/test-definitions/errors-expired-ssl.js index e6726fa67516..76e4faa3c0dc 100644 --- a/cli/test/smokehouse/test-definitions/errors-expired-ssl.js +++ b/cli/test/smokehouse/test-definitions/errors-expired-ssl.js @@ -40,18 +40,14 @@ const expectations = { audits: { 'first-contentful-paint': { scoreDisplayMode: 'error', - errorMessage: 'Required traces gatherer did not run.', + errorMessage: 'The URL you have provided does not have a valid security certificate. net::ERR_CERT_DATE_INVALID', }, }, }, artifacts: { PageLoadError: {code: 'INSECURE_DOCUMENT_REQUEST'}, - devtoolsLogs: { - 'pageLoadError-default': {...NONEMPTY_ARRAY}, - }, - traces: { - 'pageLoadError-default': {traceEvents: NONEMPTY_ARRAY}, - }, + DevtoolsLog: NONEMPTY_ARRAY, + Trace: {traceEvents: NONEMPTY_ARRAY}, }, }; diff --git a/cli/test/smokehouse/test-definitions/errors-iframe-expired-ssl.js b/cli/test/smokehouse/test-definitions/errors-iframe-expired-ssl.js index 6e81f8312283..42717989cd27 100644 --- a/cli/test/smokehouse/test-definitions/errors-iframe-expired-ssl.js +++ b/cli/test/smokehouse/test-definitions/errors-iframe-expired-ssl.js @@ -43,12 +43,8 @@ const expectations = { }, }, artifacts: { - devtoolsLogs: { - defaultPass: NONEMPTY_ARRAY, - }, - traces: { - defaultPass: {traceEvents: NONEMPTY_ARRAY}, - }, + DevtoolsLog: NONEMPTY_ARRAY, + Trace: {traceEvents: NONEMPTY_ARRAY}, }, }; diff --git a/cli/test/smokehouse/test-definitions/errors-infinite-loop.js b/cli/test/smokehouse/test-definitions/errors-infinite-loop.js index 335e7ac04f40..af4a83bf55c4 100644 --- a/cli/test/smokehouse/test-definitions/errors-infinite-loop.js +++ b/cli/test/smokehouse/test-definitions/errors-infinite-loop.js @@ -37,18 +37,14 @@ const expectations = { audits: { 'first-contentful-paint': { scoreDisplayMode: 'error', - errorMessage: 'Required traces gatherer did not run.', + errorMessage: 'Lighthouse was unable to reliably load the URL you requested because the page stopped responding.', }, }, }, artifacts: { PageLoadError: {code: 'PAGE_HUNG'}, - devtoolsLogs: { - 'pageLoadError-default': {...NONEMPTY_ARRAY}, - }, - traces: { - 'pageLoadError-default': {traceEvents: NONEMPTY_ARRAY}, - }, + DevtoolsLog: NONEMPTY_ARRAY, + Trace: {traceEvents: NONEMPTY_ARRAY}, }, }; diff --git a/core/config/constants.js b/core/config/constants.js index 65578a0c1d60..b66350f3a62e 100644 --- a/core/config/constants.js +++ b/core/config/constants.js @@ -129,7 +129,7 @@ const defaultSettings = { /** @type {Required} */ const defaultNavigationConfig = { - id: 'default', + id: 'defaultPass', loadFailureMode: 'fatal', disableThrottling: false, disableStorageReset: false, diff --git a/core/config/default-config.js b/core/config/default-config.js index f2f2b07f018c..e588e3f5fd61 100644 --- a/core/config/default-config.js +++ b/core/config/default-config.js @@ -593,7 +593,7 @@ const defaultConfig = { supportedModes: ['navigation', 'timespan', 'snapshot'], auditRefs: [ // Trust & Safety - {id: 'is-on-https', weight: 1, group: 'best-practices-trust-safety'}, + {id: 'is-on-https', weight: 5, group: 'best-practices-trust-safety'}, {id: 'geolocation-on-start', weight: 1, group: 'best-practices-trust-safety'}, {id: 'notification-on-start', weight: 1, group: 'best-practices-trust-safety'}, {id: 'csp-xss', weight: 0, group: 'best-practices-trust-safety'}, @@ -608,10 +608,10 @@ const defaultConfig = { // General Group {id: 'no-unload-listeners', weight: 1, group: 'best-practices-general'}, {id: 'js-libraries', weight: 0, group: 'best-practices-general'}, - {id: 'deprecations', weight: 1, group: 'best-practices-general'}, - {id: 'errors-in-console', weight: 1, group: 'best-practices-general'}, + {id: 'deprecations', weight: 5, group: 'best-practices-general'}, + {id: 'errors-in-console', weight: 3, group: 'best-practices-general'}, {id: 'valid-source-maps', weight: 0, group: 'best-practices-general'}, - {id: 'inspector-issues', weight: 1, group: 'best-practices-general'}, + {id: 'inspector-issues', weight: 3, group: 'best-practices-general'}, ], }, 'seo': { diff --git a/core/gather/navigation-runner.js b/core/gather/navigation-runner.js index 1d6a29126b51..c6209d138452 100644 --- a/core/gather/navigation-runner.js +++ b/core/gather/navigation-runner.js @@ -187,9 +187,15 @@ async function _computeNavigationResult( /** @type {Partial} */ const artifacts = {}; - const pageLoadErrorId = `pageLoadError-${navigationContext.navigation.id}`; - if (debugData.devtoolsLog) artifacts.devtoolsLogs = {[pageLoadErrorId]: debugData.devtoolsLog}; - if (debugData.trace) artifacts.traces = {[pageLoadErrorId]: debugData.trace}; + const navigationId = navigationContext.navigation.id; + if (debugData.devtoolsLog) { + artifacts.DevtoolsLog = debugData.devtoolsLog; + artifacts.devtoolsLogs = {[navigationId]: debugData.devtoolsLog}; + } + if (debugData.trace) { + artifacts.Trace = debugData.trace; + artifacts.traces = {[navigationId]: debugData.trace}; + } return { pageLoadError, diff --git a/core/runner.js b/core/runner.js index 2ce8a5fef0e2..fe6796b1abbe 100644 --- a/core/runner.js +++ b/core/runner.js @@ -367,6 +367,8 @@ class Runner { let auditResult; try { + if (artifacts.PageLoadError) throw artifacts.PageLoadError; + // Return an early error if an artifact required for the audit is missing or an error. for (const artifactName of audit.meta.requiredArtifacts) { const noArtifact = artifacts[artifactName] === undefined; diff --git a/core/test/gather/navigation-runner-test.js b/core/test/gather/navigation-runner-test.js index 2767ce3489a1..d414a3280f32 100644 --- a/core/test/gather/navigation-runner-test.js +++ b/core/test/gather/navigation-runner-test.js @@ -472,8 +472,10 @@ describe('NavigationRunner', () => { const {artifacts, pageLoadError} = await run(navigation); expect(pageLoadError).toBeInstanceOf(LighthouseError); expect(artifacts).toEqual({ - devtoolsLogs: {'pageLoadError-default': expect.any(Array)}, - traces: {'pageLoadError-default': {traceEvents: []}}, + DevtoolsLog: expect.any(Array), + Trace: {traceEvents: []}, + devtoolsLogs: {defaultPass: expect.any(Array)}, + traces: {defaultPass: {traceEvents: []}}, }); }); From 96218223d7e4cb7db15a059a24229ea1e1ac0263 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Wed, 26 Jul 2023 13:26:12 -0700 Subject: [PATCH 2/4] ope --- core/config/default-config.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/config/default-config.js b/core/config/default-config.js index e588e3f5fd61..f2f2b07f018c 100644 --- a/core/config/default-config.js +++ b/core/config/default-config.js @@ -593,7 +593,7 @@ const defaultConfig = { supportedModes: ['navigation', 'timespan', 'snapshot'], auditRefs: [ // Trust & Safety - {id: 'is-on-https', weight: 5, group: 'best-practices-trust-safety'}, + {id: 'is-on-https', weight: 1, group: 'best-practices-trust-safety'}, {id: 'geolocation-on-start', weight: 1, group: 'best-practices-trust-safety'}, {id: 'notification-on-start', weight: 1, group: 'best-practices-trust-safety'}, {id: 'csp-xss', weight: 0, group: 'best-practices-trust-safety'}, @@ -608,10 +608,10 @@ const defaultConfig = { // General Group {id: 'no-unload-listeners', weight: 1, group: 'best-practices-general'}, {id: 'js-libraries', weight: 0, group: 'best-practices-general'}, - {id: 'deprecations', weight: 5, group: 'best-practices-general'}, - {id: 'errors-in-console', weight: 3, group: 'best-practices-general'}, + {id: 'deprecations', weight: 1, group: 'best-practices-general'}, + {id: 'errors-in-console', weight: 1, group: 'best-practices-general'}, {id: 'valid-source-maps', weight: 0, group: 'best-practices-general'}, - {id: 'inspector-issues', weight: 3, group: 'best-practices-general'}, + {id: 'inspector-issues', weight: 1, group: 'best-practices-general'}, ], }, 'seo': { From 7e394a4149533b287dfde5c0f8c8f4998590c80d Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Wed, 26 Jul 2023 17:59:41 -0700 Subject: [PATCH 3/4] test --- core/test/config/config-test.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/core/test/config/config-test.js b/core/test/config/config-test.js index 2a3e2ab4fbd2..f2c286d18291 100644 --- a/core/test/config/config-test.js +++ b/core/test/config/config-test.js @@ -253,9 +253,10 @@ describe('Fraggle Rock Config', () => { expect(resolvedConfig).toMatchObject({ artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}], - navigations: [ - {id: 'default', artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}]}, - ], + navigations: [{ + id: 'defaultPass', + artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}], + }], }); }); @@ -269,7 +270,7 @@ describe('Fraggle Rock Config', () => { expect(resolvedConfig).toMatchObject({ navigations: [ { - id: 'default', + id: 'defaultPass', blankPage: 'about:blank', artifacts: [{id: 'Accessibility', gatherer: {path: 'accessibility'}}], loadFailureMode: 'fatal', @@ -366,7 +367,7 @@ describe('Fraggle Rock Config', () => { {id: 'Accessibility'}, ], navigations: [ - {id: 'default', artifacts: [{id: 'Accessibility'}]}, + {id: 'defaultPass', artifacts: [{id: 'Accessibility'}]}, ], }); }); From 0b8fa263a3d9f1577e86617befc0f06bc0ef2a79 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 27 Jul 2023 13:04:24 -0700 Subject: [PATCH 4/4] new error versions --- .../smokehouse/test-definitions/errors-expired-ssl.js | 4 ++-- .../test-definitions/errors-infinite-loop.js | 4 ++-- core/gather/navigation-runner.js | 10 +++++----- core/lib/asset-saver.js | 6 ++++++ core/test/gather/navigation-runner-test.js | 8 ++++---- types/artifacts.d.ts | 4 ++++ 6 files changed, 23 insertions(+), 13 deletions(-) diff --git a/cli/test/smokehouse/test-definitions/errors-expired-ssl.js b/cli/test/smokehouse/test-definitions/errors-expired-ssl.js index 76e4faa3c0dc..d1e3b117e2e1 100644 --- a/cli/test/smokehouse/test-definitions/errors-expired-ssl.js +++ b/cli/test/smokehouse/test-definitions/errors-expired-ssl.js @@ -46,8 +46,8 @@ const expectations = { }, artifacts: { PageLoadError: {code: 'INSECURE_DOCUMENT_REQUEST'}, - DevtoolsLog: NONEMPTY_ARRAY, - Trace: {traceEvents: NONEMPTY_ARRAY}, + DevtoolsLogError: NONEMPTY_ARRAY, + TraceError: {traceEvents: NONEMPTY_ARRAY}, }, }; diff --git a/cli/test/smokehouse/test-definitions/errors-infinite-loop.js b/cli/test/smokehouse/test-definitions/errors-infinite-loop.js index af4a83bf55c4..0c9780380d19 100644 --- a/cli/test/smokehouse/test-definitions/errors-infinite-loop.js +++ b/cli/test/smokehouse/test-definitions/errors-infinite-loop.js @@ -43,8 +43,8 @@ const expectations = { }, artifacts: { PageLoadError: {code: 'PAGE_HUNG'}, - DevtoolsLog: NONEMPTY_ARRAY, - Trace: {traceEvents: NONEMPTY_ARRAY}, + DevtoolsLogError: NONEMPTY_ARRAY, + TraceError: {traceEvents: NONEMPTY_ARRAY}, }, }; diff --git a/core/gather/navigation-runner.js b/core/gather/navigation-runner.js index c6209d138452..c4cf1136a25c 100644 --- a/core/gather/navigation-runner.js +++ b/core/gather/navigation-runner.js @@ -187,14 +187,14 @@ async function _computeNavigationResult( /** @type {Partial} */ const artifacts = {}; - const navigationId = navigationContext.navigation.id; + const pageLoadErrorId = `pageLoadError-${navigationContext.navigation.id}`; if (debugData.devtoolsLog) { - artifacts.DevtoolsLog = debugData.devtoolsLog; - artifacts.devtoolsLogs = {[navigationId]: debugData.devtoolsLog}; + artifacts.DevtoolsLogError = debugData.devtoolsLog; + artifacts.devtoolsLogs = {[pageLoadErrorId]: debugData.devtoolsLog}; } if (debugData.trace) { - artifacts.Trace = debugData.trace; - artifacts.traces = {[navigationId]: debugData.trace}; + artifacts.TraceError = debugData.trace; + artifacts.traces = {[pageLoadErrorId]: debugData.trace}; } return { diff --git a/core/lib/asset-saver.js b/core/lib/asset-saver.js index 277773bde4ef..a6a5317bb521 100644 --- a/core/lib/asset-saver.js +++ b/core/lib/asset-saver.js @@ -62,6 +62,9 @@ function loadArtifacts(basePath) { if (passName === 'defaultPass') { artifacts.DevtoolsLog = devtoolsLog; } + if (passName === 'pageLoadError-defaultPass') { + artifacts.DevtoolsLogError = devtoolsLog; + } }); // load traces @@ -74,6 +77,9 @@ function loadArtifacts(basePath) { if (passName === 'defaultPass') { artifacts.Trace = artifacts.traces[passName]; } + if (passName === 'pageLoadError-defaultPass') { + artifacts.TraceError = artifacts.traces[passName]; + } }); if (Array.isArray(artifacts.Timing)) { diff --git a/core/test/gather/navigation-runner-test.js b/core/test/gather/navigation-runner-test.js index d414a3280f32..c61b8a7bd672 100644 --- a/core/test/gather/navigation-runner-test.js +++ b/core/test/gather/navigation-runner-test.js @@ -472,10 +472,10 @@ describe('NavigationRunner', () => { const {artifacts, pageLoadError} = await run(navigation); expect(pageLoadError).toBeInstanceOf(LighthouseError); expect(artifacts).toEqual({ - DevtoolsLog: expect.any(Array), - Trace: {traceEvents: []}, - devtoolsLogs: {defaultPass: expect.any(Array)}, - traces: {defaultPass: {traceEvents: []}}, + DevtoolsLogError: expect.any(Array), + TraceError: {traceEvents: []}, + devtoolsLogs: {'pageLoadError-defaultPass': expect.any(Array)}, + traces: {'pageLoadError-defaultPass': {traceEvents: []}}, }); }); diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 6e3f5d1c02a8..06db596f8ae1 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -108,6 +108,8 @@ export interface GathererArtifacts extends PublicGathererArtifacts { CSSUsage: {rules: Crdp.CSS.RuleUsage[], stylesheets: Artifacts.CSSStyleSheetInfo[]}; /** The primary log of devtools protocol activity. */ DevtoolsLog: DevtoolsLog; + /** The log of devtools protocol activity if there was a page load error and Chrome navigated to a `chrome-error://` page. */ + DevtoolsLogError: DevtoolsLog; /** Information on the document's doctype(or null if not present), specifically the name, publicId, and systemId. All properties default to an empty string if not present */ Doctype: Artifacts.Doctype | null; @@ -152,6 +154,8 @@ export interface GathererArtifacts extends PublicGathererArtifacts { TapTargets: Artifacts.TapTarget[]; /** The primary trace taken over the entire run. */ Trace: Trace; + /** The trace if there was a page load error and Chrome navigated to a `chrome-error://` page. */ + TraceError: Trace; /** Elements associated with metrics (ie: Largest Contentful Paint element). */ TraceElements: Artifacts.TraceElement[]; /** Parsed version of the page's Web App Manifest, or null if none found. */