From 5c79506354f496d209708eb60539e805086395e1 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 21 Mar 2024 14:02:07 -0700 Subject: [PATCH] core: remove `ScriptElements` artifact --- CONTRIBUTING.md | 4 +- .../test-definitions/byte-efficiency.js | 67 ---------- .../test-definitions/oopif-requests.js | 3 - .../test-definitions/oopif-scripts.js | 24 +--- core/audits/script-elements-test-audit.js | 29 ----- core/config/default-config.js | 1 - core/gather/gatherers/script-elements.js | 100 --------------- core/test/config/config-test.js | 2 +- .../gather/gatherers/script-elements-test.js | 117 ------------------ core/test/index-test.js | 2 +- core/test/lib/asset-saver-test.js | 16 +-- core/test/scripts/run-mocha-tests.js | 1 - types/artifacts.d.ts | 6 +- 13 files changed, 16 insertions(+), 356 deletions(-) delete mode 100644 core/audits/script-elements-test-audit.js delete mode 100644 core/gather/gatherers/script-elements.js delete mode 100644 core/test/gather/gatherers/script-elements-test.js diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e7f4b8f112d2..414908d83c9e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -79,8 +79,8 @@ A PR adding or changing a gatherer almost always needs to include the following: 1. **Golden artifacts**: `sample_v2.json` is generated from a set of artifacts that come from running LH against `dbw_tester.html`. Those artifacts likely need to be updated after gatherer changes with `yarn update:sample-artifacts`, but limit to just the artifact being altered if possible. For example: ```sh - # update just the ScriptElements artifact - yarn update:sample-artifacts ScriptElements + # update just the Scripts artifact + yarn update:sample-artifacts Scripts ``` This command works for updating `yarn update:sample-artifacts DevtoolsLog` or `Trace` as well, but the resulting `sample_v2.json` churn may be extensive and you might be better off editing manually. diff --git a/cli/test/smokehouse/test-definitions/byte-efficiency.js b/cli/test/smokehouse/test-definitions/byte-efficiency.js index 641005ea4061..ae62437cf9f4 100644 --- a/cli/test/smokehouse/test-definitions/byte-efficiency.js +++ b/cli/test/smokehouse/test-definitions/byte-efficiency.js @@ -30,7 +30,6 @@ const config = { // unsized-images is not a byte-efficiency audit but can easily leverage the variety of images present in // byte-efficiency tests & thus makes sense to test together. 'unsized-images', - 'script-elements-test-audit', ], throttlingMethod: 'devtools', }, @@ -40,7 +39,6 @@ const config = { // Lower the threshold so we don't need huge resources to make a test. unusedThreshold: 2000, }}, - 'script-elements-test-audit', ], }; @@ -50,71 +48,6 @@ const config = { */ const expectations = { artifacts: { - ScriptElements: [ - { - type: null, - src: null, - async: false, - defer: false, - source: 'head', - // Only do a single assertion for `devtoolsNodePath`: this can be flaky for elements - // deep in the DOM, and the sample LHR test has plenty of places that would catch - // a regression in `devtoolsNodePath` calculation. Keep just one for the benefit - // of other smoke test runners. - node: { - devtoolsNodePath: '2,HTML,0,HEAD,3,SCRIPT', - }, - }, - { - type: 'application/javascript', - src: 'http://localhost:10200/byte-efficiency/script.js', - async: false, - defer: false, - source: 'head', - }, - { - type: null, - src: 'http://localhost:10200/byte-efficiency/bundle.js', - async: false, - defer: false, - source: 'head', - }, - { - type: null, - src: null, - async: false, - defer: false, - source: 'body', - }, - { - type: null, - src: null, - async: false, - defer: false, - source: 'body', - }, - { - type: null, - src: null, - async: false, - defer: false, - source: 'body', - }, - { - type: null, - src: null, - async: false, - defer: false, - source: 'body', - }, - { - type: null, - src: 'http://localhost:10200/byte-efficiency/delay-complete.js?delay=8000', - async: true, - defer: false, - source: 'body', - }, - ], Scripts: { _includes: [ { diff --git a/cli/test/smokehouse/test-definitions/oopif-requests.js b/cli/test/smokehouse/test-definitions/oopif-requests.js index 2994470fa14b..3520581dcd22 100644 --- a/cli/test/smokehouse/test-definitions/oopif-requests.js +++ b/cli/test/smokehouse/test-definitions/oopif-requests.js @@ -12,14 +12,12 @@ const config = { title: 'Performance', auditRefs: [ {id: 'oopif-iframe-test-audit', weight: 0}, - {id: 'script-elements-test-audit', weight: 0}, ], }, }, audits: [ // Include an audit that *forces* the IFrameElements artifact to be used for our test. {path: 'oopif-iframe-test-audit'}, - {path: 'script-elements-test-audit'}, ], settings: { // This test runs in CI and hits the outside network of a live site. @@ -86,7 +84,6 @@ const expectations = { isPositionFixed: false, }, ], - ScriptElements: [], Scripts: [], }, }; diff --git a/cli/test/smokehouse/test-definitions/oopif-scripts.js b/cli/test/smokehouse/test-definitions/oopif-scripts.js index 5c3d2b401240..090ad1188302 100644 --- a/cli/test/smokehouse/test-definitions/oopif-scripts.js +++ b/cli/test/smokehouse/test-definitions/oopif-scripts.js @@ -12,14 +12,12 @@ const config = { title: 'Performance', auditRefs: [ {id: 'oopif-iframe-test-audit', weight: 0}, - {id: 'script-elements-test-audit', weight: 0}, ], }, }, audits: [ // Include an audit that *forces* the IFrameElements artifact to be used for our test. {path: 'oopif-iframe-test-audit'}, - {path: 'script-elements-test-audit'}, ], settings: { // This test runs in CI and hits the outside network of a live site. @@ -99,27 +97,9 @@ const expectations = { isPositionFixed: true, }, ], - // Only `:10200/oopif-simple-page.html`'s inclusion of `simple-script.js` shows here. + // Only `:10200/oopif-simple-page.html`'s inclusion of `simple-script.js` shows here, + // as well as inline scripts of the iframe. // All other scripts are filtered out because of our "OOPIF" filter. - ScriptElements: [ - { - src: 'http://localhost:10200/simple-script.js', - source: 'network', - }, - { - // Worker requests emitted on the worker's parent target since M123. - _minChromiumVersion: '123', - src: 'http://localhost:10200/simple-worker.mjs', - source: 'network', - }, - { - // Worker requests emitted on the worker's parent target since M123. - _minChromiumVersion: '123', - src: 'http://localhost:10200/simple-worker.js', - source: 'network', - }, - ], - // Same here, except we get inline scripts of the iframe. Scripts: { _includes: [ { diff --git a/core/audits/script-elements-test-audit.js b/core/audits/script-elements-test-audit.js deleted file mode 100644 index b41a10405fb0..000000000000 --- a/core/audits/script-elements-test-audit.js +++ /dev/null @@ -1,29 +0,0 @@ -/** - * @license - * Copyright 2022 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -/** - * @fileoverview This is a fake audit used exclusively in smoke tests to force inclusion of ScriptElements artifact. - * It is included here for complex reasons in the way the bundled smoketests work. - * - * The smokehouse configs are evaluated first in the node CLI side (which requires an absolute path using LH_ROOT). - * The smokehouse configs are then *re-evaluated* in the bundled context for execution by Lighthouse (which *cannot* use an absolute path using LH_ROOT). - * - * This mismatch in environment demands that the audit path in the config must be context-aware, - * yet the require-graph for the config is included before even the CLI knows that it will be using - * a bundled runner. Rather than force a massive smoketest architecture change, we include a harmless, - * test-only audit in our core list instead. - */ - -export default { - meta: { - id: 'script-elements-test-audit', - title: 'ScriptElements', - failureTitle: 'ScriptElements', - description: 'Audit to force the inclusion of ScriptElements artifact', - requiredArtifacts: ['ScriptElements'], - }, - audit: () => ({score: 1}), -}; diff --git a/core/config/default-config.js b/core/config/default-config.js index c98f0b5693d8..5fedcf0ce7c5 100644 --- a/core/config/default-config.js +++ b/core/config/default-config.js @@ -155,7 +155,6 @@ const defaultConfig = { {id: 'ResponseCompression', gatherer: 'dobetterweb/response-compression'}, {id: 'RobotsTxt', gatherer: 'seo/robots-txt'}, {id: 'ServiceWorker', gatherer: 'service-worker'}, - {id: 'ScriptElements', gatherer: 'script-elements'}, {id: 'Scripts', gatherer: 'scripts'}, {id: 'SourceMaps', gatherer: 'source-maps'}, {id: 'Stacks', gatherer: 'stacks'}, diff --git a/core/gather/gatherers/script-elements.js b/core/gather/gatherers/script-elements.js deleted file mode 100644 index 83ef33d8683c..000000000000 --- a/core/gather/gatherers/script-elements.js +++ /dev/null @@ -1,100 +0,0 @@ -/** - * @license - * Copyright 2017 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import BaseGatherer from '../base-gatherer.js'; -import {NetworkRecords} from '../../computed/network-records.js'; -import {NetworkRequest} from '../../lib/network-request.js'; -import {pageFunctions} from '../../lib/page-functions.js'; -import DevtoolsLog from './devtools-log.js'; - -/* global getNodeDetails */ - -/** - * @return {LH.Artifacts['ScriptElements']} - */ -/* c8 ignore start */ -function collectAllScriptElements() { - /** @type {HTMLScriptElement[]} */ - // @ts-expect-error - getElementsInDocument put into scope via stringification - const scripts = getElementsInDocument('script'); // eslint-disable-line no-undef - - return scripts.map(script => { - return { - type: script.type || null, - src: script.src || null, - id: script.id || null, - async: script.async, - defer: script.defer, - source: script.closest('head') ? 'head' : 'body', - // @ts-expect-error - getNodeDetails put into scope via stringification - node: getNodeDetails(script), - }; - }); -} -/* c8 ignore stop */ - -/** - * @fileoverview Gets JavaScript file contents. - */ -class ScriptElements extends BaseGatherer { - /** @type {LH.Gatherer.GathererMeta<'DevtoolsLog'>} */ - meta = { - supportedModes: ['timespan', 'navigation'], - dependencies: {DevtoolsLog: DevtoolsLog.symbol}, - }; - - /** - * @param {LH.Gatherer.Context} context - * @param {LH.Artifacts.NetworkRequest[]} networkRecords - * @return {Promise} - */ - async _getArtifact(context, networkRecords) { - const executionContext = context.driver.executionContext; - - const scripts = await executionContext.evaluate(collectAllScriptElements, { - args: [], - useIsolation: true, - deps: [ - pageFunctions.getNodeDetails, - pageFunctions.getElementsInDocument, - ], - }); - - const scriptRecords = networkRecords - .filter(record => record.resourceType === NetworkRequest.TYPES.Script) - .filter(record => record.sessionTargetType === 'page'); - - for (let i = 0; i < scriptRecords.length; i++) { - const record = scriptRecords[i]; - - const matchedScriptElement = scripts.find(script => script.src === record.url); - if (!matchedScriptElement) { - scripts.push({ - type: null, - src: record.url, - id: null, - async: false, - defer: false, - source: 'network', - node: null, - }); - } - } - - return scripts; - } - - /** - * @param {LH.Gatherer.Context<'DevtoolsLog'>} context - */ - async getArtifact(context) { - const devtoolsLog = context.dependencies.DevtoolsLog; - const networkRecords = await NetworkRecords.request(devtoolsLog, context); - return this._getArtifact(context, networkRecords); - } -} - -export default ScriptElements; diff --git a/core/test/config/config-test.js b/core/test/config/config-test.js index 50aa2a765306..afe4ba39b886 100644 --- a/core/test/config/config-test.js +++ b/core/test/config/config-test.js @@ -394,7 +394,7 @@ describe('getConfigDisplayString', () => { passes: [{ passName: 'defaultPass', gatherers: [ - {path: 'script-elements'}, + {path: 'scripts'}, ], }], audits: [ diff --git a/core/test/gather/gatherers/script-elements-test.js b/core/test/gather/gatherers/script-elements-test.js deleted file mode 100644 index a51d0819351d..000000000000 --- a/core/test/gather/gatherers/script-elements-test.js +++ /dev/null @@ -1,117 +0,0 @@ -/** - * @license - * Copyright 2021 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import {createMockContext, mockDriverSubmodules} from '../mock-driver.js'; - -const mocks = await mockDriverSubmodules(); - -// Some imports needs to be done dynamically, so that their dependencies will be mocked. -// https://github.com/GoogleChrome/lighthouse/blob/main/docs/hacking-tips.md#mocking-modules-with-testdouble -/** @typedef {import('../../../gather/gatherers/script-elements.js').default} ScriptElements */ -const ScriptElements = (await import('../../../gather/gatherers/script-elements.js')).default; -const {NetworkRequest} = await import('../../../lib/network-request.js'); - -/** - * @param {Partial=} partial - * @return {LH.Artifacts.NetworkRequest} - */ -function mockRecord(partial) { - const request = new NetworkRequest(); - request.resourceType = NetworkRequest.TYPES.Script; - request.sessionTargetType = 'page'; - return Object.assign(request, partial); -} - -/** - * @param {Partial=} partial - * @return {LH.Artifacts.ScriptElement} - */ -function mockElement(partial) { - return { - type: null, - src: null, - id: null, - async: false, - defer: false, - source: 'head', - node: null, - ...partial, - }; -} - -describe('_getArtifact', () => { - let mockContext = createMockContext(); - /** @type {ScriptElements} */ - let gatherer; - /** @type {LH.Artifacts.ScriptElement[]} */ - let scriptElements; - /** @type {LH.Artifacts.NetworkRequest[]} */ - let networkRecords; - /** @type {LH.Artifacts.NetworkRequest} */ - let mainDocument; - - beforeEach(() => { - mocks.reset(); - mockContext = createMockContext(); - gatherer = new ScriptElements(); - scriptElements = []; - mainDocument = mockRecord({resourceType: NetworkRequest.TYPES.Document, requestId: '0'}); - networkRecords = [mainDocument]; - mockContext.driver._executionContext.evaluate.mockImplementation(() => scriptElements); - }); - - it('collects script elements', async () => { - networkRecords = [ - mainDocument, - mockRecord({url: 'https://example.com/script.js', requestId: '1'}), - ]; - scriptElements = [ - mockElement({src: 'https://example.com/script.js'}), - mockElement({src: null}), - ]; - - const artifact = await gatherer._getArtifact(mockContext.asContext(), networkRecords); - - expect(artifact).toEqual([ - mockElement({src: 'https://example.com/script.js'}), - mockElement({src: null}), - ]); - }); - - it('ignore OOPIF and worker records', async () => { - networkRecords = [ - mainDocument, - mockRecord({url: 'https://example.com/script.js', requestId: '1'}), - mockRecord({url: 'https://oopif.com/script.js', requestId: '2', sessionTargetType: 'iframe'}), - mockRecord({url: 'https://oopif.com/worker.js', requestId: '2', sessionTargetType: 'worker'}), - ]; - // OOPIF would not produce script element - scriptElements = [ - mockElement({src: 'https://example.com/script.js'}), - mockElement({src: null}), - ]; - - const artifact = await gatherer._getArtifact(mockContext.asContext(), networkRecords); - - expect(artifact).toEqual([ - mockElement({src: 'https://example.com/script.js'}), - mockElement({src: null}), - ]); - }); - - it('create element if none found', async () => { - networkRecords = [ - mainDocument, - mockRecord({url: 'https://example.com/script.js', requestId: '1'}), - ]; - - const artifact = await gatherer._getArtifact(mockContext.asContext(), networkRecords); - - expect(artifact).toEqual([ - mockElement({src: 'https://example.com/script.js', source: 'network'}), - ]); - }); -}); diff --git a/core/test/index-test.js b/core/test/index-test.js index 8b50d9f2d645..ef064678bead 100644 --- a/core/test/index-test.js +++ b/core/test/index-test.js @@ -67,7 +67,7 @@ describe('Module Tests', function() { const resultPromise = lighthouse('chrome://version', {}, { passes: [{ gatherers: [ - 'script-elements', + 'scripts', ], }], audits: [ diff --git a/core/test/lib/asset-saver-test.js b/core/test/lib/asset-saver-test.js index 2cdbec0ab898..d1dfc9c2c902 100644 --- a/core/test/lib/asset-saver-test.js +++ b/core/test/lib/asset-saver-test.js @@ -402,21 +402,21 @@ describe('asset-saver helper', () => { {cause: new Error('the cause')}); const artifacts = { - ScriptElements: lhError, + Scripts: lhError, }; await assetSaver.saveArtifacts(artifacts, outputPath); const roundTripArtifacts = await assetSaver.loadArtifacts(outputPath); expect(roundTripArtifacts).toStrictEqual(artifacts); - expect(roundTripArtifacts.ScriptElements).toBeInstanceOf(LighthouseError); - expect(roundTripArtifacts.ScriptElements.code).toEqual('PROTOCOL_TIMEOUT'); - expect(roundTripArtifacts.ScriptElements.protocolMethod).toEqual(protocolMethod); - expect(roundTripArtifacts.ScriptElements.cause).toBeInstanceOf(Error); - expect(roundTripArtifacts.ScriptElements.cause.message).toEqual('the cause'); - expect(roundTripArtifacts.ScriptElements.stack).toMatch( + expect(roundTripArtifacts.Scripts).toBeInstanceOf(LighthouseError); + expect(roundTripArtifacts.Scripts.code).toEqual('PROTOCOL_TIMEOUT'); + expect(roundTripArtifacts.Scripts.protocolMethod).toEqual(protocolMethod); + expect(roundTripArtifacts.Scripts.cause).toBeInstanceOf(Error); + expect(roundTripArtifacts.Scripts.cause.message).toEqual('the cause'); + expect(roundTripArtifacts.Scripts.stack).toMatch( /^LighthouseError: PROTOCOL_TIMEOUT.*test[\\/]lib[\\/]asset-saver-test\.js/s); - expect(roundTripArtifacts.ScriptElements.friendlyMessage) + expect(roundTripArtifacts.Scripts.friendlyMessage) .toBeDisplayString(/\(Method: Page\.getFastness\)/); }); diff --git a/core/test/scripts/run-mocha-tests.js b/core/test/scripts/run-mocha-tests.js index fc5bd253ceeb..c457cfe0a0be 100644 --- a/core/test/scripts/run-mocha-tests.js +++ b/core/test/scripts/run-mocha-tests.js @@ -79,7 +79,6 @@ const testsToIsolate = new Set([ 'core/test/gather/timespan-runner-test.js', 'core/test/user-flow-test.js', 'core/test/gather/gatherers/dobetterweb/response-compression-test.js', - 'core/test/gather/gatherers/script-elements-test.js', 'core/test/runner-test.js', // These tend to timeout in puppeteer when run in parallel with other tests. diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index f601eadd2a28..d93432ab0a4c 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -89,8 +89,8 @@ interface PublicGathererArtifacts { LinkElements: Artifacts.LinkElement[]; /** The values of the elements in the head. */ MetaElements: Array<{name?: string, content?: string, property?: string, httpEquiv?: string, charset?: string, node: Artifacts.NodeDetails}>; - /** Information on all script elements in the page. Also contains the content of all requested scripts and the networkRecord requestId that contained their content. Note, HTML documents will have one entry per script tag, all with the same requestId. */ - ScriptElements: Array; + /** Information on all scripts in the page. */ + Scripts: Artifacts.Script[]; /** The dimensions and devicePixelRatio of the loaded viewport. */ ViewportDimensions: Artifacts.ViewportDimensions; } @@ -146,8 +146,6 @@ export interface GathererArtifacts extends PublicGathererArtifacts { RobotsTxt: {status: number|null, content: string|null, errorMessage?: string}; /** The result of calling the shared trace engine root cause analysis. */ RootCauses: Artifacts.TraceEngineRootCauses; - /** Information on all scripts in the page. */ - Scripts: Artifacts.Script[]; /** Version information for all ServiceWorkers active after the first page load. */ ServiceWorker: {versions: Crdp.ServiceWorker.ServiceWorkerVersion[], registrations: Crdp.ServiceWorker.ServiceWorkerRegistration[]}; /** Source maps of scripts executed in the page. */