From 1494ebbe3b5d85ad907cb6e3da2ddcb3c697fe17 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 13 May 2021 13:03:04 -0400 Subject: [PATCH 01/17] core(fr): convert image-elements gatherer --- .../gather/gatherers/image-elements.js | 132 +++++--- .../gather/gatherers/image-elements-test.js | 282 ++++++++++++++++++ 2 files changed, 370 insertions(+), 44 deletions(-) create mode 100644 lighthouse-core/test/gather/gatherers/image-elements-test.js diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 4d999374781f..ab3b860f109d 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -10,10 +10,11 @@ 'use strict'; const log = require('lighthouse-logger'); -const Gatherer = require('./gatherer.js'); +const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js'); const pageFunctions = require('../../lib/page-functions.js'); -const Driver = require('../driver.js'); // eslint-disable-line no-unused-vars +const DevtoolsLog = require('./devtools-log.js'); const FontSize = require('./seo/font-size.js'); +const NetworkRecords = require('../../computed/network-records.js'); /* global window, getElementsInDocument, Image, getNodeDetails, ShadowRoot */ @@ -221,7 +222,13 @@ function getEffectiveSizingRule({attributesStyle, inlineStyle, matchedCSSRules}, return null; } -class ImageElements extends Gatherer { +class ImageElements extends FRGatherer { + /** @type {LH.Gatherer.GathererMeta<'DevtoolsLog'>} */ + meta = { + supportedModes: ['snapshot', 'navigation'], + dependencies: {DevtoolsLog: DevtoolsLog.symbol}, + }; + constructor() { super(); /** @type {Map} */ @@ -229,7 +236,7 @@ class ImageElements extends Gatherer { } /** - * @param {Driver} driver + * @param {LH.Gatherer.FRTransitionalDriver} driver * @param {LH.Artifacts.ImageElement} element */ async fetchElementWithSizeInformation(driver, element) { @@ -240,7 +247,7 @@ class ImageElements extends Gatherer { try { // We don't want this to take forever, 250ms should be enough for images that are cached - driver.setNextProtocolTimeout(250); + driver.defaultSession.setNextProtocolTimeout(250); const size = await driver.executionContext.evaluate(determineNaturalSize, { args: [url], }); @@ -256,18 +263,18 @@ class ImageElements extends Gatherer { * Images might be sized via CSS. In order to compute unsized-images failures, we need to collect * matched CSS rules to see if this is the case. * @url http://go/dwoqq (googlers only) - * @param {Driver} driver + * @param {LH.Gatherer.FRProtocolSession} session * @param {string} devtoolsNodePath * @param {LH.Artifacts.ImageElement} element */ - async fetchSourceRules(driver, devtoolsNodePath, element) { + async fetchSourceRules(session, devtoolsNodePath, element) { try { - const {nodeId} = await driver.sendCommand('DOM.pushNodeByPathToFrontend', { + const {nodeId} = await session.sendCommand('DOM.pushNodeByPathToFrontend', { path: devtoolsNodePath, }); if (!nodeId) return; - const matchedRules = await driver.sendCommand('CSS.getMatchedStylesForNode', { + const matchedRules = await session.sendCommand('CSS.getMatchedStylesForNode', { nodeId: nodeId, }); const width = getEffectiveSizingRule(matchedRules, 'width'); @@ -284,13 +291,10 @@ class ImageElements extends Gatherer { } /** - * @param {LH.Gatherer.PassContext} passContext - * @param {LH.Gatherer.LoadData} loadData - * @return {Promise} + * @param {LH.Artifacts.NetworkRequest[]} networkRecords */ - async afterPass(passContext, loadData) { - const driver = passContext.driver; - const indexedNetworkRecords = loadData.networkRecords.reduce((map, record) => { + indexNetworkRecords(networkRecords) { + return networkRecords.reduce((map, record) => { // An image response in newer formats is sometimes incorrectly marked as "application/octet-stream", // so respect the extension too. const isImage = /^image/.test(record.mimeType) || /\.(avif|webp)$/i.test(record.url); @@ -302,33 +306,15 @@ class ImageElements extends Gatherer { return map; }, /** @type {Object} */ ({})); + } - const elements = await driver.executionContext.evaluate(collectImageElementInfo, { - args: [], - deps: [ - pageFunctions.getElementsInDocumentString, - pageFunctions.getBoundingClientRectString, - pageFunctions.getNodeDetailsString, - getClientRect, - getPosition, - getHTMLImages, - getCSSImages, - ], - }); - - await Promise.all([ - driver.sendCommand('DOM.enable'), - driver.sendCommand('CSS.enable'), - driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true}), - ]); - - // Sort (in-place) as largest images descending - elements.sort((a, b) => { - const aRecord = indexedNetworkRecords[a.src] || {}; - const bRecord = indexedNetworkRecords[b.src] || {}; - return bRecord.resourceSize - aRecord.resourceSize; - }); - + /** + * + * @param {LH.Gatherer.FRTransitionalDriver} driver + * @param {LH.Artifacts.ImageElement[]} elements + * @param {Record} indexedNetworkRecords + */ + async collectExtraDetails(driver, elements, indexedNetworkRecords) { // Don't do more than 5s of this expensive devtools protocol work. See #11289 let reachedGatheringBudget = false; setTimeout(_ => (reachedGatheringBudget = true), 5000); @@ -346,7 +332,7 @@ class ImageElements extends Gatherer { // Need source rules to determine if sized via CSS (for unsized-images). if (!element.isInShadowDOM && !element.isCss) { - await this.fetchSourceRules(driver, element.node.devtoolsNodePath, element); + await this.fetchSourceRules(driver.defaultSession, element.node.devtoolsNodePath, element); } // Images within `picture` behave strangely and natural size information isn't accurate, // CSS images have no natural size information at all. Try to get the actual size if we can. @@ -359,14 +345,72 @@ class ImageElements extends Gatherer { if (reachedGatheringBudget) { log.warn('ImageElements', `Reached gathering budget of 5s. Skipped extra details for ${skippedCount}/${elements.length}`); // eslint-disable-line max-len } + } + + /** + * @param {LH.Gatherer.FRTransitionalContext} context + * @param {LH.Artifacts.NetworkRequest[]} networkRecords + * @return {Promise} + */ + async _getArtifact(context, networkRecords) { + const session = context.driver.defaultSession; + const executionContext = context.driver.executionContext; + const indexedNetworkRecords = this.indexNetworkRecords(networkRecords); + + const elements = await executionContext.evaluate(collectImageElementInfo, { + args: [], + deps: [ + pageFunctions.getElementsInDocumentString, + pageFunctions.getBoundingClientRectString, + pageFunctions.getNodeDetailsString, + getClientRect, + getPosition, + getHTMLImages, + getCSSImages, + ], + }); + + await Promise.all([ + session.sendCommand('DOM.enable'), + session.sendCommand('CSS.enable'), + session.sendCommand('DOM.getDocument', {depth: -1, pierce: true}), + ]); + + // Sort (in-place) as largest images descending + elements.sort((a, b) => { + const aRecord = indexedNetworkRecords[a.src] || {}; + const bRecord = indexedNetworkRecords[b.src] || {}; + return bRecord.resourceSize - aRecord.resourceSize; + }); + + await this.collectExtraDetails(context.driver, elements, indexedNetworkRecords); await Promise.all([ - driver.sendCommand('DOM.disable'), - driver.sendCommand('CSS.disable'), + session.sendCommand('DOM.disable'), + session.sendCommand('CSS.disable'), ]); return elements; } + + /** + * @param {LH.Gatherer.FRTransitionalContext<'DevtoolsLog'>} context + * @return {Promise} + */ + async getArtifact(context) { + const devtoolsLog = context.dependencies.DevtoolsLog; + const networkRecords = await NetworkRecords.request(devtoolsLog, context); + return this._getArtifact(context, networkRecords); + } + + /** + * @param {LH.Gatherer.PassContext} passContext + * @param {LH.Gatherer.LoadData} loadData + * @return {Promise} + */ + async afterPass(passContext, loadData) { + return this._getArtifact({...passContext, dependencies: {}}, loadData.networkRecords); + } } module.exports = ImageElements; diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js new file mode 100644 index 000000000000..ee6e6d224d4d --- /dev/null +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -0,0 +1,282 @@ +/** + * @license Copyright 2021 The Lighthouse Authors. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +/* eslint-env jest */ + +const ImageElements = require('../../../gather/gatherers/image-elements.js'); +const NetworkRecorder = require('../../../lib/network-recorder.js'); +const {createMockContext} = require('../../fraggle-rock/gather/mock-driver.js'); +const {makeParamsOptional} = require('../../test-utils.js'); + +const devtoolsLog = /** @type {LH.DevtoolsLog} */ (require('../../fixtures/traces/lcp-m78.devtools.log.json')); // eslint-disable-line max-len +const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); + +/** @type {LH.Artifacts.ImageElement} */ +const imageElement = { + src: 'https://www.paulirish.com/avatar150.jpg', + srcset: '', + displayedWidth: 200, + displayedHeight: 200, + clientRect: { + top: 50, + bottom: 250, + left: 50, + right: 250, + }, + attributeWidth: '', + attributeHeight: '', + cssWidth: undefined, + cssHeight: undefined, + _privateCssSizing: undefined, + cssComputedPosition: 'absolute', + isCss: false, + isPicture: false, + isInShadowDOM: false, + cssComputedObjectFit: '', + cssComputedImageRendering: '', + node: { + lhId: '__nodeid__', + devtoolsNodePath: '1,HTML,1,BODY,1,DIV,1,IMG', + selector: 'body > img', + nodeLabel: 'img', + snippet: '', + boundingRect: { + top: 50, + bottom: 250, + left: 50, + right: 250, + width: 200, + height: 200, + }, + }, +}; + +jest.useFakeTimers(); + +function mockImageElements() { + const gatherer = new ImageElements(); + return { + ...gatherer, + _getArtifact: jest.fn(makeParamsOptional(gatherer._getArtifact)), + collectExtraDetails: jest.fn(makeParamsOptional(gatherer.collectExtraDetails)), + indexNetworkRecords: jest.fn(makeParamsOptional(gatherer.indexNetworkRecords)), + fetchSourceRules: jest.fn(makeParamsOptional(gatherer.fetchSourceRules)), + fetchElementWithSizeInformation: jest.fn( + makeParamsOptional(gatherer.fetchElementWithSizeInformation) + ), + }; +} + +describe('.indexNetworkRecords', () => { + it('maps image urls to network records', () => { + const gatherer = mockImageElements(); + const networkRecords = [ + { + mimeType: 'image/png', + url: 'https://example.com/img.png', + finished: true, + statusCode: 200, + }, + { + mimeType: 'application/octect-stream', + url: 'https://example.com/img.webp', + finished: true, + statusCode: 200, + }, + { + mimeType: 'application/octect-stream', + url: 'https://example.com/img.avif', + finished: true, + statusCode: 200, + }, + ]; + + const index = gatherer.indexNetworkRecords(networkRecords); + + expect(index).toEqual({ + 'https://example.com/img.avif': { + finished: true, + mimeType: 'application/octect-stream', + statusCode: 200, + url: 'https://example.com/img.avif', + }, + 'https://example.com/img.png': { + finished: true, + mimeType: 'image/png', + statusCode: 200, + url: 'https://example.com/img.png', + }, + 'https://example.com/img.webp': { + finished: true, + mimeType: 'application/octect-stream', + statusCode: 200, + url: 'https://example.com/img.webp', + }, + }); + }); + + it('ignores bad status codes', () => { + const gatherer = mockImageElements(); + const networkRecords = [ + { + mimeType: 'image/png', + url: 'https://example.com/img.png', + finished: true, + statusCode: 200, + }, + { + mimeType: 'application/octect-stream', + url: 'https://example.com/img.webp', + finished: false, + }, + { + mimeType: 'application/octect-stream', + url: 'https://example.com/img.avif', + finished: true, + statusCode: 404, + }, + ]; + + const index = gatherer.indexNetworkRecords(networkRecords); + + expect(index).toEqual({ + 'https://example.com/img.png': { + finished: true, + mimeType: 'image/png', + statusCode: 200, + url: 'https://example.com/img.png', + }, + }); + }); +}); + +describe('.collectExtraDetails', () => { + let gatherer = mockImageElements(); + + beforeEach(() => { + gatherer = mockImageElements(); + gatherer.fetchSourceRules.mockImplementation(); + gatherer.fetchElementWithSizeInformation.mockImplementation(); + }); + + it('times out', () => { + const elements = [ + {node: {}, isInShadowDOM: false, isCss: false}, + {node: {}, isInShadowDOM: false, isCss: false}, + {node: {}, isInShadowDOM: false, isCss: false}, + ]; + gatherer.fetchSourceRules.mockImplementation(async () => { + jest.advanceTimersByTime(6000); + }); + + gatherer.collectExtraDetails({}, elements, {}); + + expect(gatherer.fetchSourceRules).toHaveBeenCalledTimes(1); + }); + + it('fetch source rules for unsized images', () => { + const elements = [ + {node: {}, isInShadowDOM: false, isCss: false}, + {node: {}, isInShadowDOM: true, isCss: false}, + {node: {}, isInShadowDOM: false, isCss: true}, + ]; + + gatherer.collectExtraDetails({}, elements, {}); + + expect(gatherer.fetchSourceRules).toHaveBeenCalledTimes(1); + expect(gatherer.fetchSourceRules).toHaveBeenCalledWith(undefined, undefined, elements[0]); + }); + + it('fetch size information for image with picture', () => { + const elements = [ + {node: {}, src: 'https://example.com/a.png', isPicture: true, isCss: true, srcset: 'src'}, + {node: {}, src: 'https://example.com/b.png', isPicture: false, isCss: true, srcset: 'src'}, + {node: {}, src: 'https://example.com/c.png', isPicture: true, isCss: false, srcset: 'src'}, + {node: {}, src: 'https://example.com/d.png', isPicture: true, isCss: true}, + {node: {}, src: 'https://example.com/e.png', isPicture: true, isCss: true, srcset: 'src'}, + ]; + const indexedNetworkRecords = { + 'https://example.com/a.png': {}, + 'https://example.com/b.png': {}, + 'https://example.com/c.png': {}, + 'https://example.com/d.png': {}, + }; + + gatherer.collectExtraDetails({}, elements, indexedNetworkRecords); + + expect(gatherer.fetchElementWithSizeInformation).toHaveBeenCalledTimes(1); + expect(gatherer.fetchElementWithSizeInformation).toHaveBeenCalledWith({}, elements[0]); + }); +}); + +describe('FR compat', () => { + it('uses loadData in legacy mode', async () => { + const gatherer = new ImageElements(); + const mockContext = createMockContext(); + mockContext.driver.defaultSession.sendCommand + .mockResponse('DOM.enable') + .mockResponse('CSS.enable') + .mockResponse('DOM.getDocument') + .mockResponse('DOM.pushNodeByPathToFrontend', {nodeId: 1}) + .mockResponse('CSS.getMatchedStylesForNode', {attributesStyle: {cssProperties: [ + {name: 'width', value: '200px'}, + {name: 'height', value: '200px'}, + ]}}) + .mockResponse('CSS.disable') + .mockResponse('DOM.disable'); + mockContext.driver._executionContext.evaluate.mockReturnValue([imageElement]); + + const artifact = await gatherer.afterPass(mockContext.asLegacyContext(), { + devtoolsLog, + networkRecords, + }); + + expect(artifact).toEqual([{ + ...imageElement, + cssWidth: '200px', + cssHeight: '200px', + _privateCssSizing: { + width: '200px', + height: '200px', + aspectRatio: null, + }, + }]); + }); + + it('uses dependencies in legacy mode', async () => { + const gatherer = new ImageElements(); + const mockContext = createMockContext(); + mockContext.driver.defaultSession.sendCommand + .mockResponse('DOM.enable') + .mockResponse('CSS.enable') + .mockResponse('DOM.getDocument') + .mockResponse('DOM.pushNodeByPathToFrontend', {nodeId: 1}) + .mockResponse('CSS.getMatchedStylesForNode', {attributesStyle: {cssProperties: [ + {name: 'width', value: '200px'}, + {name: 'height', value: '200px'}, + ]}}) + .mockResponse('CSS.disable') + .mockResponse('DOM.disable'); + mockContext.driver._executionContext.evaluate.mockReturnValue([imageElement]); + + const artifact = await gatherer.getArtifact({ + ...mockContext.asContext(), + dependencies: {DevtoolsLog: devtoolsLog}, + }); + + expect(artifact).toEqual([{ + ...imageElement, + cssWidth: '200px', + cssHeight: '200px', + _privateCssSizing: { + width: '200px', + height: '200px', + aspectRatio: null, + }, + }]); + }); +}); From 80af0f0612ef7009b559e3f4fc23d3b939f9ef2a Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 13 May 2021 13:11:04 -0400 Subject: [PATCH 02/17] add to config --- lighthouse-core/fraggle-rock/config/default-config.js | 3 +++ lighthouse-core/gather/gatherers/image-elements.js | 2 +- lighthouse-core/test/fraggle-rock/api-test-pptr.js | 2 +- types/artifacts.d.ts | 1 - 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/fraggle-rock/config/default-config.js b/lighthouse-core/fraggle-rock/config/default-config.js index 5774c4f981e0..8fc7ea931d07 100644 --- a/lighthouse-core/fraggle-rock/config/default-config.js +++ b/lighthouse-core/fraggle-rock/config/default-config.js @@ -25,6 +25,7 @@ const artifacts = { FormElements: '', GlobalListeners: '', IFrameElements: '', + ImageElements: '', InstallabilityErrors: '', JsUsage: '', LinkElements: '', @@ -68,6 +69,7 @@ const defaultConfig = { {id: artifacts.FormElements, gatherer: 'form-elements'}, {id: artifacts.GlobalListeners, gatherer: 'global-listeners'}, {id: artifacts.IFrameElements, gatherer: 'iframe-elements'}, + {id: artifacts.ImageElements, gatherer: 'image-elements'}, {id: artifacts.InstallabilityErrors, gatherer: 'installability-errors'}, {id: artifacts.JsUsage, gatherer: 'js-usage'}, {id: artifacts.LinkElements, gatherer: 'link-elements'}, @@ -109,6 +111,7 @@ const defaultConfig = { artifacts.FormElements, artifacts.GlobalListeners, artifacts.IFrameElements, + artifacts.ImageElements, artifacts.InstallabilityErrors, artifacts.JsUsage, artifacts.LinkElements, diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index ab3b860f109d..6544eeba2f64 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -225,7 +225,7 @@ function getEffectiveSizingRule({attributesStyle, inlineStyle, matchedCSSRules}, class ImageElements extends FRGatherer { /** @type {LH.Gatherer.GathererMeta<'DevtoolsLog'>} */ meta = { - supportedModes: ['snapshot', 'navigation'], + supportedModes: ['navigation'], dependencies: {DevtoolsLog: DevtoolsLog.symbol}, }; diff --git a/lighthouse-core/test/fraggle-rock/api-test-pptr.js b/lighthouse-core/test/fraggle-rock/api-test-pptr.js index 88066dc081c6..cfa55deee7a2 100644 --- a/lighthouse-core/test/fraggle-rock/api-test-pptr.js +++ b/lighthouse-core/test/fraggle-rock/api-test-pptr.js @@ -159,7 +159,7 @@ describe('Fraggle Rock API', () => { const {lhr} = result; const {auditResults, failedAudits, erroredAudits} = getAuditsBreakdown(lhr); // TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached. - expect(auditResults.length).toMatchInlineSnapshot(`107`); + expect(auditResults.length).toMatchInlineSnapshot(`112`); expect(erroredAudits).toHaveLength(0); const failedAuditIds = failedAudits.map(audit => audit.id); diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 6eeae7b75d8b..9d6e5cdd7acb 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -23,7 +23,6 @@ declare global { | 'Fonts' | 'FullPageScreenshot' | 'HTTPRedirect' - | 'ImageElements' | 'InspectorIssues' | 'Manifest' | 'MixedContent' From 04d4911cc90057d075e344e116db54a39f04288d Mon Sep 17 00:00:00 2001 From: Adam Raine <6752989+adamraine@users.noreply.github.com> Date: Thu, 13 May 2021 14:23:50 -0400 Subject: [PATCH 03/17] Update lighthouse-core/test/gather/gatherers/image-elements-test.js Co-authored-by: Patrick Hulce --- lighthouse-core/test/gather/gatherers/image-elements-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index ee6e6d224d4d..f99597d9de5b 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -163,7 +163,7 @@ describe('.collectExtraDetails', () => { gatherer.fetchElementWithSizeInformation.mockImplementation(); }); - it('times out', () => { + it('respects the overall time budget for source rules', () => { const elements = [ {node: {}, isInShadowDOM: false, isCss: false}, {node: {}, isInShadowDOM: false, isCss: false}, From 947a26066214ae22c55a172fed3063ef95514a02 Mon Sep 17 00:00:00 2001 From: Adam Raine <6752989+adamraine@users.noreply.github.com> Date: Thu, 13 May 2021 14:23:56 -0400 Subject: [PATCH 04/17] Update lighthouse-core/test/gather/gatherers/image-elements-test.js Co-authored-by: Patrick Hulce --- lighthouse-core/test/gather/gatherers/image-elements-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index f99597d9de5b..51c2196ba827 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -178,7 +178,7 @@ describe('.collectExtraDetails', () => { expect(gatherer.fetchSourceRules).toHaveBeenCalledTimes(1); }); - it('fetch source rules for unsized images', () => { + it('fetch source rules to determine sizing for non-shadow DOM/non-CSS images', () => { const elements = [ {node: {}, isInShadowDOM: false, isCss: false}, {node: {}, isInShadowDOM: true, isCss: false}, From 5db0fbd07e3718b0cae90037c402de1389959877 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 13 May 2021 14:34:34 -0400 Subject: [PATCH 05/17] mutiple source rules --- .../gather/gatherers/image-elements-test.js | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index 51c2196ba827..f045c085464b 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -163,7 +163,7 @@ describe('.collectExtraDetails', () => { gatherer.fetchElementWithSizeInformation.mockImplementation(); }); - it('respects the overall time budget for source rules', () => { + it('respects the overall time budget for source rules', async () => { const elements = [ {node: {}, isInShadowDOM: false, isCss: false}, {node: {}, isInShadowDOM: false, isCss: false}, @@ -173,25 +173,38 @@ describe('.collectExtraDetails', () => { jest.advanceTimersByTime(6000); }); - gatherer.collectExtraDetails({}, elements, {}); + await gatherer.collectExtraDetails({}, elements, {}); expect(gatherer.fetchSourceRules).toHaveBeenCalledTimes(1); }); - it('fetch source rules to determine sizing for non-shadow DOM/non-CSS images', () => { + it('fetch source rules to determine sizing for non-shadow DOM/non-CSS images', async () => { const elements = [ {node: {}, isInShadowDOM: false, isCss: false}, {node: {}, isInShadowDOM: true, isCss: false}, {node: {}, isInShadowDOM: false, isCss: true}, ]; - gatherer.collectExtraDetails({}, elements, {}); + await gatherer.collectExtraDetails({}, elements, {}); expect(gatherer.fetchSourceRules).toHaveBeenCalledTimes(1); expect(gatherer.fetchSourceRules).toHaveBeenCalledWith(undefined, undefined, elements[0]); }); - it('fetch size information for image with picture', () => { + it('fetch multiple source rules to determine sizing for non-shadow DOM/non-CSS images', async () => { + const elements = [ + {node: {}, isInShadowDOM: false, isCss: false}, + {node: {}, isInShadowDOM: false, isCss: false}, + ]; + + await gatherer.collectExtraDetails({}, elements, {}); + + expect(gatherer.fetchSourceRules).toHaveBeenCalledTimes(2); + expect(gatherer.fetchSourceRules).toHaveBeenNthCalledWith(1, undefined, undefined, elements[0]); + expect(gatherer.fetchSourceRules).toHaveBeenNthCalledWith(2, undefined, undefined, elements[1]); + }); + + it('fetch size information for image with picture', async () => { const elements = [ {node: {}, src: 'https://example.com/a.png', isPicture: true, isCss: true, srcset: 'src'}, {node: {}, src: 'https://example.com/b.png', isPicture: false, isCss: true, srcset: 'src'}, @@ -206,7 +219,7 @@ describe('.collectExtraDetails', () => { 'https://example.com/d.png': {}, }; - gatherer.collectExtraDetails({}, elements, indexedNetworkRecords); + await gatherer.collectExtraDetails({}, elements, indexedNetworkRecords); expect(gatherer.fetchElementWithSizeInformation).toHaveBeenCalledTimes(1); expect(gatherer.fetchElementWithSizeInformation).toHaveBeenCalledWith({}, elements[0]); From 9168e6d79a41d5b79dfadc126c68a3b358c3382f Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 13 May 2021 14:55:22 -0400 Subject: [PATCH 06/17] update expectation --- .../gather/gatherers/image-elements.js | 4 ++-- .../test/gather/gatherers/image-elements-test.js | 16 +++++++++------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 6544eeba2f64..4f3a6f48b47c 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -322,8 +322,8 @@ class ImageElements extends FRGatherer { for (const element of elements) { // Pull some of our information directly off the network record. - const networkRecord = indexedNetworkRecords[element.src] || {}; - element.mimeType = networkRecord.mimeType; + const networkRecord = indexedNetworkRecords[element.src]; + element.mimeType = networkRecord && networkRecord.mimeType; if (reachedGatheringBudget) { skippedCount++; diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index f045c085464b..e4035c93bc28 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -206,11 +206,11 @@ describe('.collectExtraDetails', () => { it('fetch size information for image with picture', async () => { const elements = [ - {node: {}, src: 'https://example.com/a.png', isPicture: true, isCss: true, srcset: 'src'}, - {node: {}, src: 'https://example.com/b.png', isPicture: false, isCss: true, srcset: 'src'}, - {node: {}, src: 'https://example.com/c.png', isPicture: true, isCss: false, srcset: 'src'}, - {node: {}, src: 'https://example.com/d.png', isPicture: true, isCss: true}, - {node: {}, src: 'https://example.com/e.png', isPicture: true, isCss: true, srcset: 'src'}, + {node: {}, src: 'https://example.com/a.png', isPicture: false, isCss: true, srcset: 'src'}, + {node: {}, src: 'https://example.com/b.png', isPicture: true, isCss: false, srcset: 'src'}, + {node: {}, src: 'https://example.com/c.png', isPicture: false, isCss: true}, + {node: {}, src: 'https://example.com/d.png', isPicture: false, isCss: false}, + {node: {}, src: 'https://example.com/e.png', isPicture: false, isCss: true, srcset: 'src'}, ]; const indexedNetworkRecords = { 'https://example.com/a.png': {}, @@ -221,8 +221,10 @@ describe('.collectExtraDetails', () => { await gatherer.collectExtraDetails({}, elements, indexedNetworkRecords); - expect(gatherer.fetchElementWithSizeInformation).toHaveBeenCalledTimes(1); - expect(gatherer.fetchElementWithSizeInformation).toHaveBeenCalledWith({}, elements[0]); + expect(gatherer.fetchElementWithSizeInformation).toHaveBeenCalledTimes(3); + expect(gatherer.fetchElementWithSizeInformation).toHaveBeenNthCalledWith(1, {}, elements[0]); + expect(gatherer.fetchElementWithSizeInformation).toHaveBeenNthCalledWith(2, {}, elements[1]); + expect(gatherer.fetchElementWithSizeInformation).toHaveBeenNthCalledWith(3, {}, elements[2]); }); }); From cc5ca1321460ddee61809f166dbea958b2f31979 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 13 May 2021 15:19:43 -0400 Subject: [PATCH 07/17] no optional params --- .../gather/gatherers/image-elements-test.js | 117 +++++++++--------- 1 file changed, 60 insertions(+), 57 deletions(-) diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index e4035c93bc28..1327a0141dec 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -9,14 +9,25 @@ const ImageElements = require('../../../gather/gatherers/image-elements.js'); const NetworkRecorder = require('../../../lib/network-recorder.js'); -const {createMockContext} = require('../../fraggle-rock/gather/mock-driver.js'); -const {makeParamsOptional} = require('../../test-utils.js'); +const NetworkRequest = require('../../../lib/network-request.js'); +const {createMockContext, createMockDriver} = require('../../fraggle-rock/gather/mock-driver.js'); const devtoolsLog = /** @type {LH.DevtoolsLog} */ (require('../../fixtures/traces/lcp-m78.devtools.log.json')); // eslint-disable-line max-len const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); +jest.useFakeTimers(); + +/** + * @param {Partial=} partial + * @return {LH.Artifacts.NetworkRequest} + */ +function mockRequest(partial = {}) { + const request = new NetworkRequest(); + return Object.assign(request, partial); +} + /** @type {LH.Artifacts.ImageElement} */ -const imageElement = { +const mockEl = { src: 'https://www.paulirish.com/avatar150.jpg', srcset: '', displayedWidth: 200, @@ -55,19 +66,15 @@ const imageElement = { }, }; -jest.useFakeTimers(); - function mockImageElements() { const gatherer = new ImageElements(); return { ...gatherer, - _getArtifact: jest.fn(makeParamsOptional(gatherer._getArtifact)), - collectExtraDetails: jest.fn(makeParamsOptional(gatherer.collectExtraDetails)), - indexNetworkRecords: jest.fn(makeParamsOptional(gatherer.indexNetworkRecords)), - fetchSourceRules: jest.fn(makeParamsOptional(gatherer.fetchSourceRules)), - fetchElementWithSizeInformation: jest.fn( - makeParamsOptional(gatherer.fetchElementWithSizeInformation) - ), + _getArtifact: jest.fn(gatherer._getArtifact), + collectExtraDetails: jest.fn(gatherer.collectExtraDetails), + indexNetworkRecords: jest.fn(gatherer.indexNetworkRecords), + fetchSourceRules: jest.fn(gatherer.fetchSourceRules), + fetchElementWithSizeInformation: jest.fn(gatherer.fetchElementWithSizeInformation), }; } @@ -75,29 +82,29 @@ describe('.indexNetworkRecords', () => { it('maps image urls to network records', () => { const gatherer = mockImageElements(); const networkRecords = [ - { + mockRequest({ mimeType: 'image/png', url: 'https://example.com/img.png', finished: true, statusCode: 200, - }, - { + }), + mockRequest({ mimeType: 'application/octect-stream', url: 'https://example.com/img.webp', finished: true, statusCode: 200, - }, - { + }), + mockRequest({ mimeType: 'application/octect-stream', url: 'https://example.com/img.avif', finished: true, statusCode: 200, - }, + }), ]; const index = gatherer.indexNetworkRecords(networkRecords); - expect(index).toEqual({ + expect(index).toMatchObject({ 'https://example.com/img.avif': { finished: true, mimeType: 'application/octect-stream', @@ -122,28 +129,28 @@ describe('.indexNetworkRecords', () => { it('ignores bad status codes', () => { const gatherer = mockImageElements(); const networkRecords = [ - { + mockRequest({ mimeType: 'image/png', url: 'https://example.com/img.png', finished: true, statusCode: 200, - }, - { + }), + mockRequest({ mimeType: 'application/octect-stream', url: 'https://example.com/img.webp', finished: false, - }, - { + }), + mockRequest({ mimeType: 'application/octect-stream', url: 'https://example.com/img.avif', finished: true, statusCode: 404, - }, + }), ]; const index = gatherer.indexNetworkRecords(networkRecords); - expect(index).toEqual({ + expect(index).toMatchObject({ 'https://example.com/img.png': { finished: true, mimeType: 'image/png', @@ -156,8 +163,10 @@ describe('.indexNetworkRecords', () => { describe('.collectExtraDetails', () => { let gatherer = mockImageElements(); + let driver = createMockDriver().asDriver(); beforeEach(() => { + driver = createMockDriver().asDriver(); gatherer = mockImageElements(); gatherer.fetchSourceRules.mockImplementation(); gatherer.fetchElementWithSizeInformation.mockImplementation(); @@ -165,66 +174,60 @@ describe('.collectExtraDetails', () => { it('respects the overall time budget for source rules', async () => { const elements = [ - {node: {}, isInShadowDOM: false, isCss: false}, - {node: {}, isInShadowDOM: false, isCss: false}, - {node: {}, isInShadowDOM: false, isCss: false}, + {...mockEl, isInShadowDOM: false, isCss: false}, + {...mockEl, isInShadowDOM: false, isCss: false}, + {...mockEl, isInShadowDOM: false, isCss: false}, ]; gatherer.fetchSourceRules.mockImplementation(async () => { jest.advanceTimersByTime(6000); }); - await gatherer.collectExtraDetails({}, elements, {}); + await gatherer.collectExtraDetails(driver, elements, {}); expect(gatherer.fetchSourceRules).toHaveBeenCalledTimes(1); }); it('fetch source rules to determine sizing for non-shadow DOM/non-CSS images', async () => { const elements = [ - {node: {}, isInShadowDOM: false, isCss: false}, - {node: {}, isInShadowDOM: true, isCss: false}, - {node: {}, isInShadowDOM: false, isCss: true}, + {...mockEl, isInShadowDOM: false, isCss: false}, + {...mockEl, isInShadowDOM: true, isCss: false}, + {...mockEl, isInShadowDOM: false, isCss: true}, ]; - await gatherer.collectExtraDetails({}, elements, {}); + await gatherer.collectExtraDetails(driver, elements, {}); expect(gatherer.fetchSourceRules).toHaveBeenCalledTimes(1); - expect(gatherer.fetchSourceRules).toHaveBeenCalledWith(undefined, undefined, elements[0]); }); it('fetch multiple source rules to determine sizing for non-shadow DOM/non-CSS images', async () => { const elements = [ - {node: {}, isInShadowDOM: false, isCss: false}, - {node: {}, isInShadowDOM: false, isCss: false}, + {...mockEl, isInShadowDOM: false, isCss: false}, + {...mockEl, isInShadowDOM: false, isCss: false}, ]; - await gatherer.collectExtraDetails({}, elements, {}); + await gatherer.collectExtraDetails(driver, elements, {}); expect(gatherer.fetchSourceRules).toHaveBeenCalledTimes(2); - expect(gatherer.fetchSourceRules).toHaveBeenNthCalledWith(1, undefined, undefined, elements[0]); - expect(gatherer.fetchSourceRules).toHaveBeenNthCalledWith(2, undefined, undefined, elements[1]); }); it('fetch size information for image with picture', async () => { const elements = [ - {node: {}, src: 'https://example.com/a.png', isPicture: false, isCss: true, srcset: 'src'}, - {node: {}, src: 'https://example.com/b.png', isPicture: true, isCss: false, srcset: 'src'}, - {node: {}, src: 'https://example.com/c.png', isPicture: false, isCss: true}, - {node: {}, src: 'https://example.com/d.png', isPicture: false, isCss: false}, - {node: {}, src: 'https://example.com/e.png', isPicture: false, isCss: true, srcset: 'src'}, + {...mockEl, src: 'https://example.com/a.png', isPicture: false, isCss: true, srcset: 'src'}, + {...mockEl, src: 'https://example.com/b.png', isPicture: true, isCss: false, srcset: 'src'}, + {...mockEl, src: 'https://example.com/c.png', isPicture: false, isCss: true}, + {...mockEl, src: 'https://example.com/d.png', isPicture: false, isCss: false}, + {...mockEl, src: 'https://example.com/e.png', isPicture: false, isCss: true, srcset: 'src'}, ]; const indexedNetworkRecords = { - 'https://example.com/a.png': {}, - 'https://example.com/b.png': {}, - 'https://example.com/c.png': {}, - 'https://example.com/d.png': {}, + 'https://example.com/a.png': mockRequest(), + 'https://example.com/b.png': mockRequest(), + 'https://example.com/c.png': mockRequest(), + 'https://example.com/d.png': mockRequest(), }; - await gatherer.collectExtraDetails({}, elements, indexedNetworkRecords); + await gatherer.collectExtraDetails(driver, elements, indexedNetworkRecords); expect(gatherer.fetchElementWithSizeInformation).toHaveBeenCalledTimes(3); - expect(gatherer.fetchElementWithSizeInformation).toHaveBeenNthCalledWith(1, {}, elements[0]); - expect(gatherer.fetchElementWithSizeInformation).toHaveBeenNthCalledWith(2, {}, elements[1]); - expect(gatherer.fetchElementWithSizeInformation).toHaveBeenNthCalledWith(3, {}, elements[2]); }); }); @@ -243,7 +246,7 @@ describe('FR compat', () => { ]}}) .mockResponse('CSS.disable') .mockResponse('DOM.disable'); - mockContext.driver._executionContext.evaluate.mockReturnValue([imageElement]); + mockContext.driver._executionContext.evaluate.mockReturnValue([mockEl]); const artifact = await gatherer.afterPass(mockContext.asLegacyContext(), { devtoolsLog, @@ -251,7 +254,7 @@ describe('FR compat', () => { }); expect(artifact).toEqual([{ - ...imageElement, + ...mockEl, cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { @@ -276,7 +279,7 @@ describe('FR compat', () => { ]}}) .mockResponse('CSS.disable') .mockResponse('DOM.disable'); - mockContext.driver._executionContext.evaluate.mockReturnValue([imageElement]); + mockContext.driver._executionContext.evaluate.mockReturnValue([mockEl]); const artifact = await gatherer.getArtifact({ ...mockContext.asContext(), @@ -284,7 +287,7 @@ describe('FR compat', () => { }); expect(artifact).toEqual([{ - ...imageElement, + ...mockEl, cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { From 85cce15edd8b26606978e238cedcdb68875c1424 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 13 May 2021 15:22:52 -0400 Subject: [PATCH 08/17] spy on --- .../gather/gatherers/image-elements-test.js | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index 1327a0141dec..4b5f308d5d79 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -68,14 +68,12 @@ const mockEl = { function mockImageElements() { const gatherer = new ImageElements(); - return { - ...gatherer, - _getArtifact: jest.fn(gatherer._getArtifact), - collectExtraDetails: jest.fn(gatherer.collectExtraDetails), - indexNetworkRecords: jest.fn(gatherer.indexNetworkRecords), - fetchSourceRules: jest.fn(gatherer.fetchSourceRules), - fetchElementWithSizeInformation: jest.fn(gatherer.fetchElementWithSizeInformation), - }; + jest.spyOn(gatherer, '_getArtifact'); + jest.spyOn(gatherer, 'collectExtraDetails'); + jest.spyOn(gatherer, 'indexNetworkRecords'); + jest.spyOn(gatherer, 'fetchSourceRules'); + jest.spyOn(gatherer, 'fetchElementWithSizeInformation'); + return gatherer; } describe('.indexNetworkRecords', () => { @@ -168,8 +166,8 @@ describe('.collectExtraDetails', () => { beforeEach(() => { driver = createMockDriver().asDriver(); gatherer = mockImageElements(); - gatherer.fetchSourceRules.mockImplementation(); - gatherer.fetchElementWithSizeInformation.mockImplementation(); + gatherer.fetchSourceRules = jest.fn(); + gatherer.fetchElementWithSizeInformation = jest.fn(); }); it('respects the overall time budget for source rules', async () => { @@ -178,7 +176,7 @@ describe('.collectExtraDetails', () => { {...mockEl, isInShadowDOM: false, isCss: false}, {...mockEl, isInShadowDOM: false, isCss: false}, ]; - gatherer.fetchSourceRules.mockImplementation(async () => { + gatherer.fetchSourceRules = jest.fn().mockImplementation(async () => { jest.advanceTimersByTime(6000); }); From c4d74803cc77b1cb50086123f483b4f6a04e999f Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 13 May 2021 16:52:16 -0400 Subject: [PATCH 09/17] fetchSourceRules test --- .../gather/gatherers/image-elements-test.js | 145 +++++++++++++++--- 1 file changed, 124 insertions(+), 21 deletions(-) diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index 4b5f308d5d79..efe976734c33 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -10,7 +10,7 @@ const ImageElements = require('../../../gather/gatherers/image-elements.js'); const NetworkRecorder = require('../../../lib/network-recorder.js'); const NetworkRequest = require('../../../lib/network-request.js'); -const {createMockContext, createMockDriver} = require('../../fraggle-rock/gather/mock-driver.js'); +const {createMockContext, createMockDriver, createMockSession} = require('../../fraggle-rock/gather/mock-driver.js'); const devtoolsLog = /** @type {LH.DevtoolsLog} */ (require('../../fixtures/traces/lcp-m78.devtools.log.json')); // eslint-disable-line max-len const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); @@ -26,8 +26,8 @@ function mockRequest(partial = {}) { return Object.assign(request, partial); } -/** @type {LH.Artifacts.ImageElement} */ -const mockEl = { +/** @return {LH.Artifacts.ImageElement} */ +const mockEl = () => ({ src: 'https://www.paulirish.com/avatar150.jpg', srcset: '', displayedWidth: 200, @@ -64,7 +64,7 @@ const mockEl = { height: 200, }, }, -}; +}); function mockImageElements() { const gatherer = new ImageElements(); @@ -76,6 +76,109 @@ function mockImageElements() { return gatherer; } +describe('.fetchSourceRules', () => { + let gatherer = mockImageElements(); + let session = createMockSession(); + + beforeEach(() => { + gatherer = mockImageElements(); + session = createMockSession(); + }) + + it('handles no node found error', async () => { + session.sendCommand.mockImplementationOnce(() => { + throw Error('No node found'); + }) + + const returnPromise = gatherer.fetchSourceRules(session.asSession(), '1,HTML,1,BODY,1,IMG', mockEl()); + await expect(returnPromise).resolves.not.toThrow(); + }); + + it('gets sizes from inline style', async () => { + session.sendCommand + .mockResponse('DOM.pushNodeByPathToFrontend', {nodeId: 1}) + .mockResponse('CSS.getMatchedStylesForNode', {inlineStyle: {cssProperties: [ + {name: 'width', value: '200px'}, + {name: 'height', value: '200px'}, + {name: 'aspect-ratio', value: '1 / 1'}, + ]}}); + + const element = mockEl(); + await gatherer.fetchSourceRules(session.asSession(), '1,HTML,1,BODY,1,IMG', element); + + expect(element).toMatchObject({ + cssWidth: '200px', + cssHeight: '200px', + _privateCssSizing: { + width: '200px', + height: '200px', + aspectRatio: '1 / 1', + } + }); + }); + + it('gets sizes from attributes', async () => { + session.sendCommand + .mockResponse('DOM.pushNodeByPathToFrontend', {nodeId: 1}) + .mockResponse('CSS.getMatchedStylesForNode', {attributesStyle: {cssProperties: [ + {name: 'width', value: '200px'}, + {name: 'height', value: '200px'}, + ]}}); + + const element = mockEl(); + await gatherer.fetchSourceRules(session.asSession(), '1,HTML,1,BODY,1,IMG', element); + + expect(element).toMatchObject({ + cssWidth: '200px', + cssHeight: '200px', + _privateCssSizing: { + width: '200px', + height: '200px', + aspectRatio: null, + } + }); + }); + + it('gets sizes from matching CSS rules', async () => { + session.sendCommand + .mockResponse('DOM.pushNodeByPathToFrontend', {nodeId: 1}) + .mockResponse('CSS.getMatchedStylesForNode', {matchedCSSRules: [ + { + rule: { + selectorList: {selectors: [{text: 'img'}]}, + style: {cssProperties: [ + {name: 'width', value: '200px'}, + {name: 'height', value: '200px'}, + ]} + }, + matchingSelectors: [0], + }, + { + rule: { + selectorList: {selectors: [{text: '.classy'}]}, + style: {cssProperties: [ + {name: 'aspect-ratio', value: '1 / 1'}, + ]} + }, + matchingSelectors: [0], + }, + ]}); + + const element = mockEl(); + await gatherer.fetchSourceRules(session.asSession(), '1,HTML,1,BODY,1,IMG', element); + + expect(element).toMatchObject({ + cssWidth: '200px', + cssHeight: '200px', + _privateCssSizing: { + width: '200px', + height: '200px', + aspectRatio: '1 / 1', + } + }); + }); +}); + describe('.indexNetworkRecords', () => { it('maps image urls to network records', () => { const gatherer = mockImageElements(); @@ -172,9 +275,9 @@ describe('.collectExtraDetails', () => { it('respects the overall time budget for source rules', async () => { const elements = [ - {...mockEl, isInShadowDOM: false, isCss: false}, - {...mockEl, isInShadowDOM: false, isCss: false}, - {...mockEl, isInShadowDOM: false, isCss: false}, + {...mockEl(), isInShadowDOM: false, isCss: false}, + {...mockEl(), isInShadowDOM: false, isCss: false}, + {...mockEl(), isInShadowDOM: false, isCss: false}, ]; gatherer.fetchSourceRules = jest.fn().mockImplementation(async () => { jest.advanceTimersByTime(6000); @@ -187,9 +290,9 @@ describe('.collectExtraDetails', () => { it('fetch source rules to determine sizing for non-shadow DOM/non-CSS images', async () => { const elements = [ - {...mockEl, isInShadowDOM: false, isCss: false}, - {...mockEl, isInShadowDOM: true, isCss: false}, - {...mockEl, isInShadowDOM: false, isCss: true}, + {...mockEl(), isInShadowDOM: false, isCss: false}, + {...mockEl(), isInShadowDOM: true, isCss: false}, + {...mockEl(), isInShadowDOM: false, isCss: true}, ]; await gatherer.collectExtraDetails(driver, elements, {}); @@ -199,8 +302,8 @@ describe('.collectExtraDetails', () => { it('fetch multiple source rules to determine sizing for non-shadow DOM/non-CSS images', async () => { const elements = [ - {...mockEl, isInShadowDOM: false, isCss: false}, - {...mockEl, isInShadowDOM: false, isCss: false}, + {...mockEl(), isInShadowDOM: false, isCss: false}, + {...mockEl(), isInShadowDOM: false, isCss: false}, ]; await gatherer.collectExtraDetails(driver, elements, {}); @@ -210,11 +313,11 @@ describe('.collectExtraDetails', () => { it('fetch size information for image with picture', async () => { const elements = [ - {...mockEl, src: 'https://example.com/a.png', isPicture: false, isCss: true, srcset: 'src'}, - {...mockEl, src: 'https://example.com/b.png', isPicture: true, isCss: false, srcset: 'src'}, - {...mockEl, src: 'https://example.com/c.png', isPicture: false, isCss: true}, - {...mockEl, src: 'https://example.com/d.png', isPicture: false, isCss: false}, - {...mockEl, src: 'https://example.com/e.png', isPicture: false, isCss: true, srcset: 'src'}, + {...mockEl(), src: 'https://example.com/a.png', isPicture: false, isCss: true, srcset: 'src'}, + {...mockEl(), src: 'https://example.com/b.png', isPicture: true, isCss: false, srcset: 'src'}, + {...mockEl(), src: 'https://example.com/c.png', isPicture: false, isCss: true}, + {...mockEl(), src: 'https://example.com/d.png', isPicture: false, isCss: false}, + {...mockEl(), src: 'https://example.com/e.png', isPicture: false, isCss: true, srcset: 'src'}, ]; const indexedNetworkRecords = { 'https://example.com/a.png': mockRequest(), @@ -244,14 +347,14 @@ describe('FR compat', () => { ]}}) .mockResponse('CSS.disable') .mockResponse('DOM.disable'); - mockContext.driver._executionContext.evaluate.mockReturnValue([mockEl]); + mockContext.driver._executionContext.evaluate.mockReturnValue([mockEl()]); const artifact = await gatherer.afterPass(mockContext.asLegacyContext(), { devtoolsLog, networkRecords, }); - expect(artifact).toEqual([{ + expect(artifact).toMatchObject([{ ...mockEl, cssWidth: '200px', cssHeight: '200px', @@ -277,14 +380,14 @@ describe('FR compat', () => { ]}}) .mockResponse('CSS.disable') .mockResponse('DOM.disable'); - mockContext.driver._executionContext.evaluate.mockReturnValue([mockEl]); + mockContext.driver._executionContext.evaluate.mockReturnValue([mockEl()]); const artifact = await gatherer.getArtifact({ ...mockContext.asContext(), dependencies: {DevtoolsLog: devtoolsLog}, }); - expect(artifact).toEqual([{ + expect(artifact).toMatchObject([{ ...mockEl, cssWidth: '200px', cssHeight: '200px', From 3d8c71cdae71c593389fe9b6a986ac11d2202a59 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 13 May 2021 17:13:59 -0400 Subject: [PATCH 10/17] more tests --- .../gather/gatherers/image-elements.js | 1 + .../gather/gatherers/image-elements-test.js | 54 +++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 4f3a6f48b47c..3ff108f98521 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -243,6 +243,7 @@ class ImageElements extends FRGatherer { const url = element.src; if (this._naturalSizeCache.has(url)) { Object.assign(element, this._naturalSizeCache.get(url)); + return; } try { diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index efe976734c33..aea888bffbdc 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -76,6 +76,60 @@ function mockImageElements() { return gatherer; } +describe('.fetchElementsWithSizingInformation', () => { + let gatherer = mockImageElements(); + let driver = createMockDriver(); + beforeEach(() => { + gatherer = mockImageElements(); + driver = createMockDriver(); + }) + + it('uses natural dimensions from cache if possible', async () => { + const element = mockEl(); + gatherer._naturalSizeCache.set(element.src, { + naturalWidth: 200, + naturalHeight: 200, + }); + + await gatherer.fetchElementWithSizeInformation(driver.asDriver(), element); + + expect(driver._executionContext.evaluate).not.toHaveBeenCalled(); + expect(element).toMatchObject({ + naturalWidth: 200, + naturalHeight: 200, + }); + }); + + it('evaluates natural dimensions if not in cache', async () => { + const element = mockEl(); + driver._executionContext.evaluate.mockReturnValue({ + naturalWidth: 200, + naturalHeight: 200, + }); + + await gatherer.fetchElementWithSizeInformation(driver.asDriver(), element); + + expect(gatherer._naturalSizeCache.get(element.src)).toEqual({ + naturalWidth: 200, + naturalHeight: 200, + }); + expect(element).toMatchObject({ + naturalWidth: 200, + naturalHeight: 200, + }); + }); + + it('handles error when calculating natural dimensions', async () => { + const element = mockEl(); + driver._executionContext.evaluate.mockRejectedValue(new Error()); + + const returnPromise = gatherer.fetchElementWithSizeInformation(driver.asDriver(), element); + + await expect(returnPromise).resolves.not.toThrow(); + expect(element).toEqual(mockEl()); // Element unchanged + }); +}); + describe('.fetchSourceRules', () => { let gatherer = mockImageElements(); let session = createMockSession(); From f3ef859a92910729523f10c85f29dc1bc403c43a Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 13 May 2021 17:16:46 -0400 Subject: [PATCH 11/17] lint --- .../gather/gatherers/image-elements-test.js | 30 ++++++++++++------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index aea888bffbdc..bda2f8080302 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -10,7 +10,11 @@ const ImageElements = require('../../../gather/gatherers/image-elements.js'); const NetworkRecorder = require('../../../lib/network-recorder.js'); const NetworkRequest = require('../../../lib/network-request.js'); -const {createMockContext, createMockDriver, createMockSession} = require('../../fraggle-rock/gather/mock-driver.js'); +const { + createMockContext, + createMockDriver, + createMockSession, +} = require('../../fraggle-rock/gather/mock-driver.js'); const devtoolsLog = /** @type {LH.DevtoolsLog} */ (require('../../fixtures/traces/lcp-m78.devtools.log.json')); // eslint-disable-line max-len const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); @@ -82,7 +86,7 @@ describe('.fetchElementsWithSizingInformation', () => { beforeEach(() => { gatherer = mockImageElements(); driver = createMockDriver(); - }) + }); it('uses natural dimensions from cache if possible', async () => { const element = mockEl(); @@ -137,14 +141,18 @@ describe('.fetchSourceRules', () => { beforeEach(() => { gatherer = mockImageElements(); session = createMockSession(); - }) + }); it('handles no node found error', async () => { session.sendCommand.mockImplementationOnce(() => { throw Error('No node found'); - }) + }); - const returnPromise = gatherer.fetchSourceRules(session.asSession(), '1,HTML,1,BODY,1,IMG', mockEl()); + const returnPromise = gatherer.fetchSourceRules( + session.asSession(), + '1,HTML,1,BODY,1,IMG', + mockEl() + ); await expect(returnPromise).resolves.not.toThrow(); }); @@ -167,7 +175,7 @@ describe('.fetchSourceRules', () => { width: '200px', height: '200px', aspectRatio: '1 / 1', - } + }, }); }); @@ -189,7 +197,7 @@ describe('.fetchSourceRules', () => { width: '200px', height: '200px', aspectRatio: null, - } + }, }); }); @@ -203,7 +211,7 @@ describe('.fetchSourceRules', () => { style: {cssProperties: [ {name: 'width', value: '200px'}, {name: 'height', value: '200px'}, - ]} + ]}, }, matchingSelectors: [0], }, @@ -212,7 +220,7 @@ describe('.fetchSourceRules', () => { selectorList: {selectors: [{text: '.classy'}]}, style: {cssProperties: [ {name: 'aspect-ratio', value: '1 / 1'}, - ]} + ]}, }, matchingSelectors: [0], }, @@ -228,7 +236,7 @@ describe('.fetchSourceRules', () => { width: '200px', height: '200px', aspectRatio: '1 / 1', - } + }, }); }); }); @@ -354,7 +362,7 @@ describe('.collectExtraDetails', () => { expect(gatherer.fetchSourceRules).toHaveBeenCalledTimes(1); }); - it('fetch multiple source rules to determine sizing for non-shadow DOM/non-CSS images', async () => { + it('fetch multiple source rules for non-shadow DOM/non-CSS images', async () => { const elements = [ {...mockEl(), isInShadowDOM: false, isCss: false}, {...mockEl(), isInShadowDOM: false, isCss: false}, From b6a03881bf23b235c26e2bfebe4d4a305e155b0d Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 13 May 2021 17:20:12 -0400 Subject: [PATCH 12/17] config test --- lighthouse-core/test/fraggle-rock/config/config-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/test/fraggle-rock/config/config-test.js b/lighthouse-core/test/fraggle-rock/config/config-test.js index 68242f2d789f..9875bc0040b1 100644 --- a/lighthouse-core/test/fraggle-rock/config/config-test.js +++ b/lighthouse-core/test/fraggle-rock/config/config-test.js @@ -63,8 +63,8 @@ describe('Fraggle Rock Config', () => { }); it('should throw on invalid artifact definitions', () => { - const configJson = {artifacts: [{id: 'ImageElements', gatherer: 'image-elements'}]}; - expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/ImageElements gatherer/); + const configJson = {artifacts: [{id: 'FullPageScreenshot', gatherer: 'full-page-screenshot'}]}; + expect(() => initializeConfig(configJson, {gatherMode})).toThrow(/FullPageScreenshot gatherer/); }); it('should resolve navigation definitions', () => { From 1fe02b44166519d998b16ae41e32bd4abb632891 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Fri, 14 May 2021 11:14:11 -0400 Subject: [PATCH 13/17] sometimes my genius is almost frightening --- lighthouse-core/test/gather/gatherers/image-elements-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index bda2f8080302..55362355ad77 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -417,7 +417,7 @@ describe('FR compat', () => { }); expect(artifact).toMatchObject([{ - ...mockEl, + ...mockEl(), cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { @@ -450,7 +450,7 @@ describe('FR compat', () => { }); expect(artifact).toMatchObject([{ - ...mockEl, + ...mockEl(), cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { From 2f01f63d428df48c02513e85329cd3dc5123640d Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Fri, 14 May 2021 11:22:05 -0400 Subject: [PATCH 14/17] stricter tests --- .../gather/gatherers/image-elements-test.js | 41 +++++++++++-------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index 55362355ad77..72769cdc419d 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -98,7 +98,8 @@ describe('.fetchElementsWithSizingInformation', () => { await gatherer.fetchElementWithSizeInformation(driver.asDriver(), element); expect(driver._executionContext.evaluate).not.toHaveBeenCalled(); - expect(element).toMatchObject({ + expect(element).toEqual({ + ...mockEl(), naturalWidth: 200, naturalHeight: 200, }); @@ -117,7 +118,8 @@ describe('.fetchElementsWithSizingInformation', () => { naturalWidth: 200, naturalHeight: 200, }); - expect(element).toMatchObject({ + expect(element).toEqual({ + ...mockEl(), naturalWidth: 200, naturalHeight: 200, }); @@ -168,7 +170,8 @@ describe('.fetchSourceRules', () => { const element = mockEl(); await gatherer.fetchSourceRules(session.asSession(), '1,HTML,1,BODY,1,IMG', element); - expect(element).toMatchObject({ + expect(element).toEqual({ + ...mockEl(), cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { @@ -190,7 +193,8 @@ describe('.fetchSourceRules', () => { const element = mockEl(); await gatherer.fetchSourceRules(session.asSession(), '1,HTML,1,BODY,1,IMG', element); - expect(element).toMatchObject({ + expect(element).toEqual({ + ...mockEl(), cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { @@ -229,7 +233,8 @@ describe('.fetchSourceRules', () => { const element = mockEl(); await gatherer.fetchSourceRules(session.asSession(), '1,HTML,1,BODY,1,IMG', element); - expect(element).toMatchObject({ + expect(element).toEqual({ + ...mockEl(), cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { @@ -267,25 +272,25 @@ describe('.indexNetworkRecords', () => { const index = gatherer.indexNetworkRecords(networkRecords); - expect(index).toMatchObject({ - 'https://example.com/img.avif': { + expect(index).toEqual({ + 'https://example.com/img.avif': mockRequest({ finished: true, mimeType: 'application/octect-stream', statusCode: 200, url: 'https://example.com/img.avif', - }, - 'https://example.com/img.png': { + }), + 'https://example.com/img.png': mockRequest({ finished: true, mimeType: 'image/png', statusCode: 200, url: 'https://example.com/img.png', - }, - 'https://example.com/img.webp': { + }), + 'https://example.com/img.webp': mockRequest({ finished: true, mimeType: 'application/octect-stream', statusCode: 200, url: 'https://example.com/img.webp', - }, + }), }); }); @@ -313,13 +318,13 @@ describe('.indexNetworkRecords', () => { const index = gatherer.indexNetworkRecords(networkRecords); - expect(index).toMatchObject({ - 'https://example.com/img.png': { + expect(index).toEqual({ + 'https://example.com/img.png': mockRequest({ finished: true, mimeType: 'image/png', statusCode: 200, url: 'https://example.com/img.png', - }, + }), }); }); }); @@ -416,8 +421,9 @@ describe('FR compat', () => { networkRecords, }); - expect(artifact).toMatchObject([{ + expect(artifact).toEqual([{ ...mockEl(), + mimeType: 'image/jpeg', cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { @@ -449,8 +455,9 @@ describe('FR compat', () => { dependencies: {DevtoolsLog: devtoolsLog}, }); - expect(artifact).toMatchObject([{ + expect(artifact).toEqual([{ ...mockEl(), + mimeType: 'image/jpeg', cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { From c4c39c02d7a51c3798b6b5fee754f5421334fcd5 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Fri, 14 May 2021 12:07:15 -0400 Subject: [PATCH 15/17] cleanup --- .../gather/gatherers/image-elements-test.js | 217 +++++++++--------- 1 file changed, 111 insertions(+), 106 deletions(-) diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index 72769cdc419d..28256e0963e7 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -30,47 +30,55 @@ function mockRequest(partial = {}) { return Object.assign(request, partial); } -/** @return {LH.Artifacts.ImageElement} */ -const mockEl = () => ({ - src: 'https://www.paulirish.com/avatar150.jpg', - srcset: '', - displayedWidth: 200, - displayedHeight: 200, - clientRect: { - top: 50, - bottom: 250, - left: 50, - right: 250, - }, - attributeWidth: '', - attributeHeight: '', - cssWidth: undefined, - cssHeight: undefined, - _privateCssSizing: undefined, - cssComputedPosition: 'absolute', - isCss: false, - isPicture: false, - isInShadowDOM: false, - cssComputedObjectFit: '', - cssComputedImageRendering: '', - node: { - lhId: '__nodeid__', - devtoolsNodePath: '1,HTML,1,BODY,1,DIV,1,IMG', - selector: 'body > img', - nodeLabel: 'img', - snippet: '', - boundingRect: { +/** + * @param {Partial=} partial + * @return {LH.Artifacts.ImageElement} + */ +function mockElement(partial = {}) { + return { + src: 'https://www.paulirish.com/avatar150.jpg', + srcset: '', + displayedWidth: 200, + displayedHeight: 200, + clientRect: { top: 50, bottom: 250, left: 50, right: 250, - width: 200, - height: 200, }, - }, -}); + attributeWidth: '', + attributeHeight: '', + naturalWidth: undefined, + naturalHeight: undefined, + cssWidth: undefined, + cssHeight: undefined, + _privateCssSizing: undefined, + cssComputedPosition: 'absolute', + isCss: false, + isPicture: false, + isInShadowDOM: false, + cssComputedObjectFit: '', + cssComputedImageRendering: '', + node: { + lhId: '__nodeid__', + devtoolsNodePath: '1,HTML,1,BODY,1,DIV,1,IMG', + selector: 'body > div > img', + nodeLabel: 'img', + snippet: '', + boundingRect: { + top: 50, + bottom: 250, + left: 50, + right: 250, + width: 200, + height: 200, + }, + }, + ...partial, + }; +} -function mockImageElements() { +function makeImageElements() { const gatherer = new ImageElements(); jest.spyOn(gatherer, '_getArtifact'); jest.spyOn(gatherer, 'collectExtraDetails'); @@ -81,15 +89,15 @@ function mockImageElements() { } describe('.fetchElementsWithSizingInformation', () => { - let gatherer = mockImageElements(); + let gatherer = makeImageElements(); let driver = createMockDriver(); beforeEach(() => { - gatherer = mockImageElements(); + gatherer = makeImageElements(); driver = createMockDriver(); }); it('uses natural dimensions from cache if possible', async () => { - const element = mockEl(); + const element = mockElement(); gatherer._naturalSizeCache.set(element.src, { naturalWidth: 200, naturalHeight: 200, @@ -98,15 +106,14 @@ describe('.fetchElementsWithSizingInformation', () => { await gatherer.fetchElementWithSizeInformation(driver.asDriver(), element); expect(driver._executionContext.evaluate).not.toHaveBeenCalled(); - expect(element).toEqual({ - ...mockEl(), + expect(element).toEqual(mockElement({ naturalWidth: 200, naturalHeight: 200, - }); + })); }); it('evaluates natural dimensions if not in cache', async () => { - const element = mockEl(); + const element = mockElement(); driver._executionContext.evaluate.mockReturnValue({ naturalWidth: 200, naturalHeight: 200, @@ -118,30 +125,29 @@ describe('.fetchElementsWithSizingInformation', () => { naturalWidth: 200, naturalHeight: 200, }); - expect(element).toEqual({ - ...mockEl(), + expect(element).toEqual(mockElement({ naturalWidth: 200, naturalHeight: 200, - }); + })); }); it('handles error when calculating natural dimensions', async () => { - const element = mockEl(); + const element = mockElement(); driver._executionContext.evaluate.mockRejectedValue(new Error()); const returnPromise = gatherer.fetchElementWithSizeInformation(driver.asDriver(), element); await expect(returnPromise).resolves.not.toThrow(); - expect(element).toEqual(mockEl()); // Element unchanged + expect(element).toEqual(mockElement()); // Element unchanged }); }); describe('.fetchSourceRules', () => { - let gatherer = mockImageElements(); + let gatherer = makeImageElements(); let session = createMockSession(); beforeEach(() => { - gatherer = mockImageElements(); + gatherer = makeImageElements(); session = createMockSession(); }); @@ -153,7 +159,7 @@ describe('.fetchSourceRules', () => { const returnPromise = gatherer.fetchSourceRules( session.asSession(), '1,HTML,1,BODY,1,IMG', - mockEl() + mockElement() ); await expect(returnPromise).resolves.not.toThrow(); }); @@ -167,11 +173,10 @@ describe('.fetchSourceRules', () => { {name: 'aspect-ratio', value: '1 / 1'}, ]}}); - const element = mockEl(); - await gatherer.fetchSourceRules(session.asSession(), '1,HTML,1,BODY,1,IMG', element); + const element = mockElement(); + await gatherer.fetchSourceRules(session.asSession(), element.node.devtoolsNodePath, element); - expect(element).toEqual({ - ...mockEl(), + expect(element).toEqual(mockElement({ cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { @@ -179,7 +184,7 @@ describe('.fetchSourceRules', () => { height: '200px', aspectRatio: '1 / 1', }, - }); + })); }); it('gets sizes from attributes', async () => { @@ -190,11 +195,10 @@ describe('.fetchSourceRules', () => { {name: 'height', value: '200px'}, ]}}); - const element = mockEl(); - await gatherer.fetchSourceRules(session.asSession(), '1,HTML,1,BODY,1,IMG', element); + const element = mockElement(); + await gatherer.fetchSourceRules(session.asSession(), element.node.devtoolsNodePath, element); - expect(element).toEqual({ - ...mockEl(), + expect(element).toEqual(mockElement({ cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { @@ -202,7 +206,7 @@ describe('.fetchSourceRules', () => { height: '200px', aspectRatio: null, }, - }); + })); }); it('gets sizes from matching CSS rules', async () => { @@ -230,11 +234,10 @@ describe('.fetchSourceRules', () => { }, ]}); - const element = mockEl(); - await gatherer.fetchSourceRules(session.asSession(), '1,HTML,1,BODY,1,IMG', element); + const element = mockElement(); + await gatherer.fetchSourceRules(session.asSession(), element.node.devtoolsNodePath, element); - expect(element).toEqual({ - ...mockEl(), + expect(element).toEqual(mockElement({ cssWidth: '200px', cssHeight: '200px', _privateCssSizing: { @@ -242,13 +245,13 @@ describe('.fetchSourceRules', () => { height: '200px', aspectRatio: '1 / 1', }, - }); + })); }); }); describe('.indexNetworkRecords', () => { it('maps image urls to network records', () => { - const gatherer = mockImageElements(); + const gatherer = makeImageElements(); const networkRecords = [ mockRequest({ mimeType: 'image/png', @@ -295,7 +298,7 @@ describe('.indexNetworkRecords', () => { }); it('ignores bad status codes', () => { - const gatherer = mockImageElements(); + const gatherer = makeImageElements(); const networkRecords = [ mockRequest({ mimeType: 'image/png', @@ -330,21 +333,21 @@ describe('.indexNetworkRecords', () => { }); describe('.collectExtraDetails', () => { - let gatherer = mockImageElements(); + let gatherer = makeImageElements(); let driver = createMockDriver().asDriver(); beforeEach(() => { driver = createMockDriver().asDriver(); - gatherer = mockImageElements(); + gatherer = makeImageElements(); gatherer.fetchSourceRules = jest.fn(); gatherer.fetchElementWithSizeInformation = jest.fn(); }); it('respects the overall time budget for source rules', async () => { const elements = [ - {...mockEl(), isInShadowDOM: false, isCss: false}, - {...mockEl(), isInShadowDOM: false, isCss: false}, - {...mockEl(), isInShadowDOM: false, isCss: false}, + mockElement({isInShadowDOM: false, isCss: false}), + mockElement({isInShadowDOM: false, isCss: false}), + mockElement({isInShadowDOM: false, isCss: false}), ]; gatherer.fetchSourceRules = jest.fn().mockImplementation(async () => { jest.advanceTimersByTime(6000); @@ -357,9 +360,9 @@ describe('.collectExtraDetails', () => { it('fetch source rules to determine sizing for non-shadow DOM/non-CSS images', async () => { const elements = [ - {...mockEl(), isInShadowDOM: false, isCss: false}, - {...mockEl(), isInShadowDOM: true, isCss: false}, - {...mockEl(), isInShadowDOM: false, isCss: true}, + mockElement({isInShadowDOM: false, isCss: false}), + mockElement({isInShadowDOM: true, isCss: false}), + mockElement({isInShadowDOM: false, isCss: true}), ]; await gatherer.collectExtraDetails(driver, elements, {}); @@ -369,8 +372,8 @@ describe('.collectExtraDetails', () => { it('fetch multiple source rules for non-shadow DOM/non-CSS images', async () => { const elements = [ - {...mockEl(), isInShadowDOM: false, isCss: false}, - {...mockEl(), isInShadowDOM: false, isCss: false}, + mockElement({isInShadowDOM: false, isCss: false}), + mockElement({isInShadowDOM: false, isCss: false}), ]; await gatherer.collectExtraDetails(driver, elements, {}); @@ -380,11 +383,11 @@ describe('.collectExtraDetails', () => { it('fetch size information for image with picture', async () => { const elements = [ - {...mockEl(), src: 'https://example.com/a.png', isPicture: false, isCss: true, srcset: 'src'}, - {...mockEl(), src: 'https://example.com/b.png', isPicture: true, isCss: false, srcset: 'src'}, - {...mockEl(), src: 'https://example.com/c.png', isPicture: false, isCss: true}, - {...mockEl(), src: 'https://example.com/d.png', isPicture: false, isCss: false}, - {...mockEl(), src: 'https://example.com/e.png', isPicture: false, isCss: true, srcset: 'src'}, + mockElement({src: 'https://example.com/a.png', isPicture: false, isCss: true, srcset: 'src'}), + mockElement({src: 'https://example.com/b.png', isPicture: true, isCss: false, srcset: 'src'}), + mockElement({src: 'https://example.com/c.png', isPicture: false, isCss: true}), + mockElement({src: 'https://example.com/d.png', isPicture: false, isCss: false}), + mockElement({src: 'https://example.com/e.png', isPicture: false, isCss: true, srcset: 'src'}), ]; const indexedNetworkRecords = { 'https://example.com/a.png': mockRequest(), @@ -414,24 +417,25 @@ describe('FR compat', () => { ]}}) .mockResponse('CSS.disable') .mockResponse('DOM.disable'); - mockContext.driver._executionContext.evaluate.mockReturnValue([mockEl()]); + mockContext.driver._executionContext.evaluate.mockReturnValue([mockElement()]); const artifact = await gatherer.afterPass(mockContext.asLegacyContext(), { devtoolsLog, networkRecords, }); - expect(artifact).toEqual([{ - ...mockEl(), - mimeType: 'image/jpeg', - cssWidth: '200px', - cssHeight: '200px', - _privateCssSizing: { - width: '200px', - height: '200px', - aspectRatio: null, - }, - }]); + expect(artifact).toEqual([ + mockElement({ + mimeType: 'image/jpeg', + cssWidth: '200px', + cssHeight: '200px', + _privateCssSizing: { + width: '200px', + height: '200px', + aspectRatio: null, + }, + }), + ]); }); it('uses dependencies in legacy mode', async () => { @@ -448,23 +452,24 @@ describe('FR compat', () => { ]}}) .mockResponse('CSS.disable') .mockResponse('DOM.disable'); - mockContext.driver._executionContext.evaluate.mockReturnValue([mockEl()]); + mockContext.driver._executionContext.evaluate.mockReturnValue([mockElement()]); const artifact = await gatherer.getArtifact({ ...mockContext.asContext(), dependencies: {DevtoolsLog: devtoolsLog}, }); - expect(artifact).toEqual([{ - ...mockEl(), - mimeType: 'image/jpeg', - cssWidth: '200px', - cssHeight: '200px', - _privateCssSizing: { - width: '200px', - height: '200px', - aspectRatio: null, - }, - }]); + expect(artifact).toEqual([ + mockElement({ + mimeType: 'image/jpeg', + cssWidth: '200px', + cssHeight: '200px', + _privateCssSizing: { + width: '200px', + height: '200px', + aspectRatio: null, + }, + }), + ]); }); }); From 2ba7a68913bf206639ad7b3f6c881e468723bb45 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Fri, 14 May 2021 13:50:27 -0400 Subject: [PATCH 16/17] typo --- lighthouse-core/test/gather/gatherers/image-elements-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/test/gather/gatherers/image-elements-test.js b/lighthouse-core/test/gather/gatherers/image-elements-test.js index 28256e0963e7..ccbe9571ad88 100644 --- a/lighthouse-core/test/gather/gatherers/image-elements-test.js +++ b/lighthouse-core/test/gather/gatherers/image-elements-test.js @@ -438,7 +438,7 @@ describe('FR compat', () => { ]); }); - it('uses dependencies in legacy mode', async () => { + it('uses dependencies in FR', async () => { const gatherer = new ImageElements(); const mockContext = createMockContext(); mockContext.driver.defaultSession.sendCommand From d91e945821d82a45883eefd8df1a34faf1ab3c03 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Fri, 14 May 2021 15:17:30 -0400 Subject: [PATCH 17/17] comments --- lighthouse-core/gather/gatherers/image-elements.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 3ff108f98521..50cb0f0943f4 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -306,7 +306,7 @@ class ImageElements extends FRGatherer { } return map; - }, /** @type {Object} */ ({})); + }, /** @type {Record} */ ({})); } /** @@ -377,7 +377,7 @@ class ImageElements extends FRGatherer { session.sendCommand('DOM.getDocument', {depth: -1, pierce: true}), ]); - // Sort (in-place) as largest images descending + // Sort (in-place) as largest images descending. elements.sort((a, b) => { const aRecord = indexedNetworkRecords[a.src] || {}; const bRecord = indexedNetworkRecords[b.src] || {};