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.

Sign up for GitHub

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

Jump to bottom

core(runner): audits can only use requiredArtifacts #8760

Merged
merged 4 commits into from
May 2, 2019
Merged

core(runner): audits can only use requiredArtifacts #8760

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd40e7b
core(runner): only pass requiredArtifacts
patrickhulce May 1, 2019
bfe007a
add test case
patrickhulce May 1, 2019
3c2df7f
semicolon
patrickhulce May 1, 2019
97b392c
smokes
patrickhulce May 2, 2019
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
2 changes: 1 addition & 1 deletion lighthouse-core/audits/bootup-time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class BootupTime extends Audit {
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
scoreDisplayMode: Audit.SCORING_MODES.NUMERIC,
requiredArtifacts: ['traces'],
requiredArtifacts: ['traces', 'devtoolsLogs'],
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/byte-efficiency/render-blocking-resources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class RenderBlockingResources extends Audit {
// This audit also looks at CSSUsage but has a graceful fallback if it failed, so do not mark
Copy link
Member

Choose a reason for hiding this comment

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

do you want to change anything about CSSUsage for this audit, since after this PR it won't ever see it unless we get around to optionalArtifacts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh yeah nice catch!

// it as a "requiredArtifact".
// TODO: look into adding an `optionalArtifacts` property that captures this
requiredArtifacts: ['URL', 'TagsBlockingFirstPaint', 'traces'],
requiredArtifacts: ['URL', 'TagsBlockingFirstPaint', 'traces', 'devtoolsLogs'],
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/byte-efficiency/unminified-css.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class UnminifiedCSS extends ByteEfficiencyAudit {
title: str_(UIStrings.title),
description: str_(UIStrings.description),
scoreDisplayMode: ByteEfficiencyAudit.SCORING_MODES.NUMERIC,
requiredArtifacts: ['CSSUsage', 'devtoolsLogs', 'traces'],
requiredArtifacts: ['CSSUsage', 'devtoolsLogs', 'traces', 'URL'],
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/byte-efficiency/uses-optimized-images.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class UsesOptimizedImages extends ByteEfficiencyAudit {
title: str_(UIStrings.title),
description: str_(UIStrings.description),
scoreDisplayMode: ByteEfficiencyAudit.SCORING_MODES.NUMERIC,
requiredArtifacts: ['OptimizedImages', 'ImageElements', 'devtoolsLogs', 'traces'],
requiredArtifacts: ['OptimizedImages', 'ImageElements', 'devtoolsLogs', 'traces', 'URL'],
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/byte-efficiency/uses-webp-images.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class UsesWebPImages extends ByteEfficiencyAudit {
title: str_(UIStrings.title),
description: str_(UIStrings.description),
scoreDisplayMode: ByteEfficiencyAudit.SCORING_MODES.NUMERIC,
requiredArtifacts: ['OptimizedImages', 'devtoolsLogs', 'traces'],
requiredArtifacts: ['OptimizedImages', 'devtoolsLogs', 'traces', 'URL', 'ImageElements'],
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/metrics/estimated-input-latency.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class EstimatedInputLatency extends Audit {
title: str_(UIStrings.title),
description: str_(UIStrings.description),
scoreDisplayMode: Audit.SCORING_MODES.NUMERIC,
requiredArtifacts: ['traces'],
requiredArtifacts: ['traces', 'devtoolsLogs'],
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/metrics/first-cpu-idle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class FirstCPUIdle extends Audit {
title: str_(UIStrings.title),
description: str_(UIStrings.description),
scoreDisplayMode: Audit.SCORING_MODES.NUMERIC,
requiredArtifacts: ['traces'],
requiredArtifacts: ['traces', 'devtoolsLogs'],
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/metrics/first-meaningful-paint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class FirstMeaningfulPaint extends Audit {
title: str_(UIStrings.title),
description: str_(UIStrings.description),
scoreDisplayMode: Audit.SCORING_MODES.NUMERIC,
requiredArtifacts: ['traces'],
requiredArtifacts: ['traces', 'devtoolsLogs'],
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/metrics/max-potential-fid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class MaxPotentialFID extends Audit {
title: str_(UIStrings.title),
description: str_(UIStrings.description),
scoreDisplayMode: Audit.SCORING_MODES.NUMERIC,
requiredArtifacts: ['traces'],
requiredArtifacts: ['traces', 'devtoolsLogs'],
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/seo/canonical.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class Canonical extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['LinkElements', 'URL'],
requiredArtifacts: ['LinkElements', 'URL', 'devtoolsLogs'],
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/seo/font-size.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ class FontSize extends Audit {
};
}

const viewportMeta = await ComputedViewportMeta.request(artifacts, context);
const viewportMeta = await ComputedViewportMeta.request(artifacts.MetaElements, context);
if (!viewportMeta.isMobileOptimized) {
return {
score: 0,
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/seo/is-crawlable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class IsCrawlable extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['MetaElements', 'RobotsTxt', 'URL'],
requiredArtifacts: ['MetaElements', 'RobotsTxt', 'URL', 'devtoolsLogs'],
};
}

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/seo/tap-targets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ class TapTargets extends Audit {
};
}

const viewportMeta = await ComputedViewportMeta.request(artifacts, context);
const viewportMeta = await ComputedViewportMeta.request(artifacts.MetaElements, context);
if (!viewportMeta.isMobileOptimized) {
return {
score: 0,
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/audits/viewport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class Viewport extends Audit {
* @return {Promise<LH.Audit.Product>}
*/
static async audit(artifacts, context) {
const viewportMeta = await ComputedViewportMeta.request(artifacts, context);
const viewportMeta = await ComputedViewportMeta.request(artifacts.MetaElements, context);

if (!viewportMeta.hasViewportTag) {
return {
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/computed/viewport-meta.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ const makeComputedArtifact = require('./computed-artifact.js');

class ViewportMeta {
/**
* @param {{MetaElements: LH.GathererArtifacts['MetaElements']}} artifacts
* @param {LH.GathererArtifacts['MetaElements']} MetaElements
* @return {Promise<ViewportMetaResult>}
*/
static async compute_({MetaElements}) {
static async compute_(MetaElements) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a necessary refactor to ensure that they're still cached, also this seems like a bad idea to cache on the entire artifacts object anyway :)

Copy link
Member

Choose a reason for hiding this comment

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

lol whoops

const viewportMeta = MetaElements.find(meta => meta.name === 'viewport');

if (!viewportMeta) {
Expand Down
7 changes: 6 additions & 1 deletion lighthouse-core/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,12 @@ class Runner {
...sharedAuditContext,
};

const product = await audit.audit(artifacts, auditContext);
const requiredArtifacts = audit.meta.requiredArtifacts
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the magic

patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
.reduce((requiredArtifacts, artifactName) => {
requiredArtifacts[artifactName] = artifacts[artifactName];
return requiredArtifacts;
}, /** @type {LH.Artifacts} */ ({}));
Copy link
Member

Choose a reason for hiding this comment

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

this is unfortunate, but fine enough. Maybe add a line about the masquerading type to the above comment as well?

I was hoping at one point that we could type the artifacts input of an audit based on its declared requiredArtifacts, but it would require not having Array<string> for the requiredArtifacts type, was finicky because it was nested on the meta getter, and didn't gain us all that much, but maybe in some future version of typescript...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I thought about it for about 5 seconds before thinking there was no way it was going to be usable 😆

hopefully one day typescript will let you work your magic here :)

const product = await audit.audit(requiredArtifacts, auditContext);
auditResult = Audit.generateAuditResult(audit, product);
} catch (err) {
log.warn(audit.meta.id, `Caught exception: ${err.message}`);
Expand Down
14 changes: 7 additions & 7 deletions lighthouse-core/test/computed/viewport-meta-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ describe('ViewportMeta computed artifact', () => {
const makeMetaElements = viewport => [{name: 'viewport', content: viewport}];

it('is not mobile optimized when page does not contain a viewport meta tag', async () => {
const {hasViewportTag, isMobileOptimized} = await ViewportMeta.compute_({MetaElements: []});
const {hasViewportTag, isMobileOptimized} = await ViewportMeta.compute_([]);
assert.equal(hasViewportTag, false);
assert.equal(isMobileOptimized, false);
});
Expand All @@ -23,23 +23,23 @@ describe('ViewportMeta computed artifact', () => {
it('is not mobile optimized when HTML contains a non-mobile friendly viewport meta tag', async () => {
const viewport = 'maximum-scale=1';
const {hasViewportTag, isMobileOptimized} =
await ViewportMeta.compute_({MetaElements: makeMetaElements(viewport)});
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(hasViewportTag, true);
assert.equal(isMobileOptimized, false);
});

it('is not mobile optimized when HTML contains an invalid viewport meta tag key', async () => {
const viewport = 'nonsense=true';
const {hasViewportTag, isMobileOptimized} =
await ViewportMeta.compute_({MetaElements: makeMetaElements(viewport)});
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(hasViewportTag, true);
assert.equal(isMobileOptimized, false);
});

it('is not mobile optimized when HTML contains an invalid viewport meta tag value', async () => {
const viewport = 'initial-scale=microscopic';
const {isMobileOptimized, parserWarnings} =
await ViewportMeta.compute_({MetaElements: makeMetaElements(viewport)});
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(isMobileOptimized, false);
assert.equal(parserWarnings[0], 'Invalid values found: {"initial-scale":"microscopic"}');
});
Expand All @@ -48,7 +48,7 @@ describe('ViewportMeta computed artifact', () => {
it('is not mobile optimized when HTML contains an invalid viewport meta tag key and value', async () => {
const viewport = 'nonsense=true, initial-scale=microscopic';
const {isMobileOptimized, parserWarnings} =
await ViewportMeta.compute_({MetaElements: makeMetaElements(viewport)});
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(isMobileOptimized, false);
assert.equal(parserWarnings[0], 'Invalid properties found: {"nonsense":"true"}');
assert.equal(parserWarnings[1], 'Invalid values found: {"initial-scale":"microscopic"}');
Expand All @@ -64,7 +64,7 @@ describe('ViewportMeta computed artifact', () => {

await Promise.all(viewports.map(async viewport => {
const {isMobileOptimized} =
await ViewportMeta.compute_({MetaElements: makeMetaElements(viewport)});
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(isMobileOptimized, true);
}));
});
Expand All @@ -76,7 +76,7 @@ describe('ViewportMeta computed artifact', () => {
];
await Promise.all(viewports.map(async viewport => {
const {isMobileOptimized, parserWarnings} =
await ViewportMeta.compute_({MetaElements: makeMetaElements(viewport)});
await ViewportMeta.compute_(makeMetaElements(viewport));
assert.equal(isMobileOptimized, true);
assert.equal(parserWarnings[0], undefined);
}));
Expand Down