Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(fr): limit scope of audits to applicable modes #12764

Merged
merged 3 commits into from
Jul 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lighthouse-core/audits/apple-touch-icon.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class AppleTouchIcon extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['LinkElements'],
};
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/dobetterweb/geolocation-on-start.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class GeolocationOnStart extends ViolationAudit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['ConsoleMessages'],
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class NotificationOnStart extends ViolationAudit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['ConsoleMessages'],
};
}
Expand Down
6 changes: 5 additions & 1 deletion lighthouse-core/audits/final-screenshot.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class FinalScreenshot extends Audit {
scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE,
title: 'Final Screenshot',
description: 'The last screenshot captured of the pageload.',
requiredArtifacts: ['traces'],
requiredArtifacts: ['traces', 'GatherContext'],
};
}

Expand All @@ -37,6 +37,10 @@ class FinalScreenshot extends Audit {
const finalScreenshot = screenshots[screenshots.length - 1];

if (!finalScreenshot) {
// If a timespan didn't happen to contain frames, that's fine. Just mark not applicable.
if (artifacts.GatherContext.gatherMode === 'timespan') return {notApplicable: true, score: 1};

// If it was another mode, that's a fatal error.
throw new LHError(LHError.errors.NO_SCREENSHOTS);
}

Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/font-display.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class FontDisplay extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['devtoolsLogs', 'CSSUsage', 'URL'],
};
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/installable-manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ class InstallableManifest extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['URL', 'WebAppManifest', 'InstallabilityErrors'],
};
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/maskable-icon.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class MaskableIcon extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['WebAppManifest', 'InstallabilityErrors'],
};
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/metrics.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class Metrics extends Audit {
scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE,
title: 'Metrics',
description: 'Collects all available metrics.',
supportedModes: ['navigation'],
requiredArtifacts: ['traces', 'devtoolsLogs', 'GatherContext'],
};
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/performance-budget.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class ResourceBudget extends Audit {
title: str_(UIStrings.title),
description: str_(UIStrings.description),
scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE,
supportedModes: ['navigation'],
requiredArtifacts: ['devtoolsLogs', 'URL'],
};
}
Expand Down
29 changes: 27 additions & 2 deletions lighthouse-core/audits/screenshot-thumbnails.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class ScreenshotThumbnails extends Audit {
scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE,
title: 'Screenshot Thumbnails',
description: 'This is what the load of your site looked like.',
requiredArtifacts: ['traces'],
requiredArtifacts: ['traces', 'GatherContext'],
};
}

Expand Down Expand Up @@ -70,7 +70,7 @@ class ScreenshotThumbnails extends Audit {
* @param {LH.Audit.Context} context
* @return {Promise<LH.Audit.Product>}
*/
static async audit(artifacts, context) {
static async _audit(artifacts, context) {
const trace = artifacts.traces[Audit.DEFAULT_PASS];
/** @type {Map<SpeedlineFrame, string>} */
const cachedThumbnails = new Map();
Expand Down Expand Up @@ -133,6 +133,31 @@ class ScreenshotThumbnails extends Audit {
},
};
}

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<LH.Audit.Product>}
*/
static async audit(artifacts, context) {
try {
return await this._audit(artifacts, context);
} catch (err) {
const noFramesErrors = new Set([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Set seems like overkill for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overkill to employ a data structure that matches the semantics? :)

is there a particular concern addressed by using an array?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it boils down to a style choice, so we can leave it as is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM!

just out of curiosity though, is there a particular concern you feel is addressed by using an array?

e.g. Sets are still too unfamiliar to most developers and take longer to understand the initialization than a plain array does

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think the code is easily understandable. I do think it would look cleaner if we used the array directly.

LHError.errors.NO_SCREENSHOTS.code,
LHError.errors.SPEEDINDEX_OF_ZERO.code,
LHError.errors.NO_SPEEDLINE_FRAMES.code,
LHError.errors.INVALID_SPEEDLINE.code,
]);

// If a timespan didn't happen to contain frames, that's fine. Just mark not applicable.
if (noFramesErrors.has(err.code) && artifacts.GatherContext.gatherMode === 'timespan') {
return {notApplicable: true, score: 1};
}

throw err;
}
}
}

module.exports = ScreenshotThumbnails;
1 change: 1 addition & 0 deletions lighthouse-core/audits/seo/canonical.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ class Canonical extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['LinkElements', 'URL', 'devtoolsLogs'],
};
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/seo/hreflang.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class Hreflang extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['LinkElements', 'URL'],
};
}
Expand Down
37 changes: 21 additions & 16 deletions lighthouse-core/audits/seo/http-status-code.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@
'use strict';

const Audit = require('../audit.js');
const MainResource = require('../../computed/main-resource.js');
const NetworkRecords = require('../../computed/network-records.js');
const HTTP_UNSUCCESSFUL_CODE_LOW = 400;
const HTTP_UNSUCCESSFUL_CODE_HIGH = 599;
const i18n = require('../../lib/i18n/i18n.js');
const NetworkAnalyzer = require('../../lib/dependency-graph/simulator/network-analyzer.js');

const UIStrings = {
/** Title of a Lighthouse audit that provides detail on the HTTP status code a page responds with. This descriptive title is shown when the page has responded with a valid HTTP status code. */
Expand All @@ -33,7 +34,7 @@ class HTTPStatusCode extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['devtoolsLogs', 'URL'],
requiredArtifacts: ['devtoolsLogs', 'URL', 'GatherContext'],
};
}

Expand All @@ -42,26 +43,30 @@ class HTTPStatusCode extends Audit {
* @param {LH.Audit.Context} context
* @return {Promise<LH.Audit.Product>}
*/
static audit(artifacts, context) {
static async audit(artifacts, context) {
const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
const URL = artifacts.URL;
const networkRecords = await NetworkRecords.request(devtoolsLog, context);
const mainResource = NetworkAnalyzer.findOptionalMainDocument(networkRecords, URL.finalUrl);

return MainResource.request({devtoolsLog, URL}, context)
.then(mainResource => {
const statusCode = mainResource.statusCode;
if (!mainResource) {
if (artifacts.GatherContext.gatherMode === 'timespan') return {notApplicable: true, score: 1};
throw new Error(`Unable to locate main resource`);
}

if (statusCode >= HTTP_UNSUCCESSFUL_CODE_LOW &&
const statusCode = mainResource.statusCode;

if (statusCode >= HTTP_UNSUCCESSFUL_CODE_LOW &&
statusCode <= HTTP_UNSUCCESSFUL_CODE_HIGH) {
return {
score: 0,
displayValue: `${statusCode}`,
};
}
return {
score: 0,
displayValue: `${statusCode}`,
};
}

return {
score: 1,
};
});
return {
score: 1,
};
}
}

Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/seo/is-crawlable.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class IsCrawlable extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['MetaElements', 'RobotsTxt', 'URL', 'devtoolsLogs'],
};
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/splash-screen.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class SplashScreen extends MultiCheckAudit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['WebAppManifest', 'InstallabilityErrors'],
};
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/themed-omnibox.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ class ThemedOmnibox extends MultiCheckAudit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['WebAppManifest', 'InstallabilityErrors', 'MetaElements'],
};
}
Expand Down
8 changes: 2 additions & 6 deletions lighthouse-core/computed/resource-summary.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ const makeComputedArtifact = require('./computed-artifact.js');
const NetworkRecords = require('./network-records.js');
const URL = require('../lib/url-shim.js');
const NetworkRequest = require('../lib/network-request.js');
const MainResource = require('./main-resource.js');
const Budget = require('../config/budget.js');
const Util = require('../../report/renderer/util.js');

Expand Down Expand Up @@ -107,11 +106,8 @@ class ResourceSummary {
* @return {Promise<Record<LH.Budget.ResourceType,ResourceEntry>>}
*/
static async compute_(data, context) {
const [networkRecords, mainResource] = await Promise.all([
NetworkRecords.request(data.devtoolsLog, context),
MainResource.request({devtoolsLog: data.devtoolsLog, URL: data.URL}, context),
]);
return ResourceSummary.summarize(networkRecords, mainResource.url, data.budgets);
const networkRecords = await NetworkRecords.request(data.devtoolsLog, context);
return ResourceSummary.summarize(networkRecords, data.URL.finalUrl, data.budgets);
}
}

Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/gather/gatherers/script-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class ScriptElements extends FRGatherer {
async _getArtifact(context, networkRecords, formFactor) {
const session = context.driver.defaultSession;
const executionContext = context.driver.executionContext;
const mainResource = NetworkAnalyzer.findMainDocument(networkRecords, context.url);
const mainResource = NetworkAnalyzer.findOptionalMainDocument(networkRecords, context.url);

const scripts = await executionContext.evaluate(collectAllScriptElements, {
args: [],
Expand All @@ -94,7 +94,7 @@ class ScriptElements extends FRGatherer {
});

for (const script of scripts) {
if (script.content) script.requestId = mainResource.requestId;
if (mainResource && script.content) script.requestId = mainResource.requestId;
}

const scriptRecords = networkRecords
Expand Down
12 changes: 12 additions & 0 deletions lighthouse-core/gather/gatherers/seo/tap-targets.js
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,18 @@ function gatherTapTargets(tapTargetsSelector, className) {
/* c8 ignore stop */

class TapTargets extends FRGatherer {
constructor() {
super();
/**
* This needs to be in the constructor.
* https://github.com/GoogleChrome/lighthouse/issues/12134
* @type {LH.Gatherer.GathererMeta}
*/
this.meta = {
supportedModes: ['snapshot', 'navigation'],
};
}

/**
* @param {LH.Gatherer.FRProtocolSession} session
* @param {string} className
Expand Down
12 changes: 11 additions & 1 deletion lighthouse-core/gather/gatherers/trace-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const Sentry = require('../../lib/sentry.js');
const Trace = require('./trace.js');
const ProcessedTrace = require('../../computed/processed-trace.js');
const ProcessedNavigation = require('../../computed/processed-navigation.js');
const LighthouseError = require('../../lib/lh-error.js');

/** @typedef {{nodeId: number, score?: number, animations?: {name?: string, failureReasonsMask?: number, unsupportedProperties?: string[]}[]}} TraceElementData */

Expand Down Expand Up @@ -257,7 +258,16 @@ class TraceElements extends FRGatherer {
}

const processedTrace = await ProcessedTrace.request(trace, context);
const {largestContentfulPaintEvt} = await ProcessedNavigation.request(processedTrace, context);
const {largestContentfulPaintEvt} = await ProcessedNavigation
.request(processedTrace, context)
.catch(err => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep! good catch

// If we were running in timespan mode and there was no paint, treat LCP as missing.
if (context.gatherMode === 'timespan' && err.code === LighthouseError.errors.NO_FCP.code) {
return {largestContentfulPaintEvt: undefined};
}

throw err;
});
const {mainThreadEvents} = processedTrace;

const lcpNodeId = TraceElements.getNodeIDFromTraceEvent(largestContentfulPaintEvt);
Expand Down
39 changes: 32 additions & 7 deletions lighthouse-core/test/audits/final-screenshot-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,48 @@
*/
'use strict';

const assert = require('assert').strict;

const FinalScreenshotAudit = require('../../audits/final-screenshot.js');
const pwaTrace = require('../fixtures/traces/progressive-app-m60.json');
const noScreenshotsTrace = {traceEvents: pwaTrace.traceEvents.filter(e => e.name !== 'Screenshot')};

/* eslint-env jest */

describe('Final screenshot', () => {
let context;

beforeEach(() => {
context = {computedCache: new Map()};
});

it('should extract a final screenshot from a trace', async () => {
const artifacts = Object.assign({
traces: {defaultPass: pwaTrace},
GatherContext: {gatherMode: 'timespan'},
});
const results = await FinalScreenshotAudit.audit(artifacts, {computedCache: new Map()});
const results = await FinalScreenshotAudit.audit(artifacts, context);

expect(results.score).toEqual(1);
expect(results.details.timing).toEqual(818);
expect(results.details.timestamp).toEqual(225414990064);
expect(results.details.data).toContain('data:image/jpeg;base64,/9j/4AAQSkZJRgABA');
});

it('should returns not applicable for missing screenshots in timespan mode', async () => {
const artifacts = {
traces: {defaultPass: noScreenshotsTrace},
GatherContext: {gatherMode: 'timespan'},
};

const results = await FinalScreenshotAudit.audit(artifacts, context);
expect(results.notApplicable).toEqual(true);
});

it('should throws for missing screenshots in navigation mode', async () => {
const artifacts = {
traces: {defaultPass: noScreenshotsTrace},
GatherContext: {gatherMode: 'navigation'},
};

assert.equal(results.score, 1);
assert.equal(results.details.timing, 818);
assert.equal(results.details.timestamp, 225414990064);
assert.ok(results.details.data.startsWith('data:image/jpeg;base64,/9j/4AAQSkZJRgABA'));
await expect(FinalScreenshotAudit.audit(artifacts, context)).rejects.toThrow();
});
});
Loading