From edbca2a36f24a6d0dfaf7ccdbbb94abfcd14a5a9 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 11 Jun 2018 11:48:28 -0700 Subject: [PATCH] core(multi-check): expose manifest checks in details (#5405) --- .../test/smokehouse/pwa-expectations.js | 26 +++++++++--- .../test/smokehouse/pwa2-expectations.js | 16 ++++--- .../test/smokehouse/pwa3-expectations.js | 10 +++-- lighthouse-core/audits/multi-check-audit.js | 21 ++++++++-- .../gather/computed/manifest-values.js | 2 +- .../test/audits/splash-screen-test.js | 2 +- .../test/audits/webapp-install-banner-test.js | 14 +++---- lighthouse-core/test/results/sample_v2.json | 42 +++++++++++++++++-- typings/artifacts.d.ts | 4 +- typings/audit.d.ts | 11 +++++ 10 files changed, 116 insertions(+), 32 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/pwa-expectations.js b/lighthouse-cli/test/smokehouse/pwa-expectations.js index c470b50ec95c..e8042e40618c 100644 --- a/lighthouse-cli/test/smokehouse/pwa-expectations.js +++ b/lighthouse-cli/test/smokehouse/pwa-expectations.js @@ -5,6 +5,18 @@ */ 'use strict'; +const pwaDetailsExpectations = { + isParseFailure: false, + hasStartUrl: true, + hasIconsAtLeast192px: true, + hasIconsAtLeast512px: true, + hasPWADisplayValue: true, + hasBackgroundColor: true, + hasThemeColor: true, + hasShortName: true, + hasName: true, +}; + /** * Expected Lighthouse audit values for various sites with stable(ish) PWA * results. @@ -36,16 +48,16 @@ module.exports = [ // Ignore speed test; just verify that it ran. }, 'webapp-install-banner': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [pwaDetailsExpectations]}, }, 'splash-screen': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [pwaDetailsExpectations]}, }, 'themed-omnibox': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [{...pwaDetailsExpectations, themeColor: '#2196F3'}]}, }, 'content-width': { score: 1, @@ -93,16 +105,16 @@ module.exports = [ // Ignore speed test; just verify that it ran. }, 'webapp-install-banner': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [pwaDetailsExpectations]}, }, 'splash-screen': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [pwaDetailsExpectations]}, }, 'themed-omnibox': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [pwaDetailsExpectations]}, }, 'content-width': { score: 1, @@ -124,3 +136,5 @@ module.exports = [ }, }, ]; + +module.exports.PWA_DETAILS_EXPECTATIONS = pwaDetailsExpectations; diff --git a/lighthouse-cli/test/smokehouse/pwa2-expectations.js b/lighthouse-cli/test/smokehouse/pwa2-expectations.js index e87e7736a87b..36be890b3c4b 100644 --- a/lighthouse-cli/test/smokehouse/pwa2-expectations.js +++ b/lighthouse-cli/test/smokehouse/pwa2-expectations.js @@ -5,6 +5,10 @@ */ 'use strict'; +const pwaDetailsExpectations = require('./pwa-expectations').PWA_DETAILS_EXPECTATIONS; + +const jakeExpectations = {...pwaDetailsExpectations, hasShortName: false}; + /** * Expected Lighthouse audit values for various sites with stable(ish) PWA * results. @@ -38,16 +42,16 @@ module.exports = [ // Ignore speed test; just verify that it ran. }, 'webapp-install-banner': { - // TODO(phulce): assert the checks when we put them in details score: 0, + details: {items: [jakeExpectations]}, }, 'splash-screen': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [jakeExpectations]}, }, 'themed-omnibox': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [jakeExpectations]}, }, 'content-width': { score: 1, @@ -95,16 +99,16 @@ module.exports = [ // Ignore speed test; just verify that it ran. }, 'webapp-install-banner': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [pwaDetailsExpectations]}, }, 'splash-screen': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [pwaDetailsExpectations]}, }, 'themed-omnibox': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [pwaDetailsExpectations]}, }, 'content-width': { score: 1, diff --git a/lighthouse-cli/test/smokehouse/pwa3-expectations.js b/lighthouse-cli/test/smokehouse/pwa3-expectations.js index 3e99887162ef..f28cdbc5fa87 100644 --- a/lighthouse-cli/test/smokehouse/pwa3-expectations.js +++ b/lighthouse-cli/test/smokehouse/pwa3-expectations.js @@ -5,6 +5,10 @@ */ 'use strict'; +const pwaDetailsExpectations = require('./pwa-expectations').PWA_DETAILS_EXPECTATIONS; + +const pwaRocksExpectations = {...pwaDetailsExpectations, hasIconsAtLeast512px: false}; + /** * Expected Lighthouse audit values for various sites with stable(ish) PWA * results. @@ -36,16 +40,16 @@ module.exports = [ // Ignore speed test; just verify that it ran . }, 'webapp-install-banner': { - // TODO(phulce): assert the checks when we put them in details score: 1, + details: {items: [pwaRocksExpectations]}, }, 'splash-screen': { - // TODO(phulce): assert the checks when we put them in details score: 0, + details: {items: [pwaRocksExpectations]}, }, 'themed-omnibox': { - // TODO(phulce): assert the checks when we put them in details score: 0, + details: {items: [pwaRocksExpectations]}, }, 'content-width': { score: 1, diff --git a/lighthouse-core/audits/multi-check-audit.js b/lighthouse-core/audits/multi-check-audit.js index 4776b7704337..70c724fec26d 100644 --- a/lighthouse-core/audits/multi-check-audit.js +++ b/lighthouse-core/audits/multi-check-audit.js @@ -25,23 +25,36 @@ class MultiCheckAudit extends Audit { * @return {LH.Audit.Product} */ static createAuditProduct(result) { - const extendedInfo = { - value: result, + /** @type {LH.Audit.MultiCheckAuditDetails} */ + const detailsItem = { + ...result, + ...result.manifestValues, + manifestValues: undefined, + warnings: undefined, + allChecks: undefined, }; + if (result.manifestValues && result.manifestValues.allChecks) { + result.manifestValues.allChecks.forEach(check => { + detailsItem[check.id] = check.passing; + }); + } + + const details = {items: [detailsItem]}; + // If we fail, share the failures if (result.failures.length > 0) { return { rawValue: false, explanation: `Failures: ${result.failures.join(',\n')}.`, - extendedInfo, + details, }; } // Otherwise, we pass return { rawValue: true, - extendedInfo, + details, warnings: result.warnings, }; } diff --git a/lighthouse-core/gather/computed/manifest-values.js b/lighthouse-core/gather/computed/manifest-values.js index 1519e03813ad..11b71b91d540 100644 --- a/lighthouse-core/gather/computed/manifest-values.js +++ b/lighthouse-core/gather/computed/manifest-values.js @@ -26,7 +26,7 @@ class ManifestValues extends ComputedArtifact { /** @typedef {(val: NonNullable) => boolean} Validator */ /** - * @return {Array<{id: string, failureText: string, validate: Validator}>} + * @return {Array<{id: LH.Artifacts.ManifestValueCheckID, failureText: string, validate: Validator}>} */ static get manifestChecks() { return [ diff --git a/lighthouse-core/test/audits/splash-screen-test.js b/lighthouse-core/test/audits/splash-screen-test.js index 6e7bf957ec75..e6359de1b6db 100644 --- a/lighthouse-core/test/audits/splash-screen-test.js +++ b/lighthouse-core/test/audits/splash-screen-test.js @@ -62,7 +62,7 @@ describe('PWA: splash screen audit', () => { return SplashScreenAudit.audit(artifacts).then(result => { assert.strictEqual(result.rawValue, false); assert.ok(result.explanation); - assert.strictEqual(result.extendedInfo.value.failures.length, 4); + assert.strictEqual(result.details.items[0].failures.length, 4); }); }); diff --git a/lighthouse-core/test/audits/webapp-install-banner-test.js b/lighthouse-core/test/audits/webapp-install-banner-test.js index cba8734ba045..00247cfd0a34 100644 --- a/lighthouse-core/test/audits/webapp-install-banner-test.js +++ b/lighthouse-core/test/audits/webapp-install-banner-test.js @@ -61,7 +61,7 @@ describe('PWA: webapp install banner audit', () => { return WebappInstallBannerAudit.audit(artifacts).then(result => { assert.strictEqual(result.rawValue, false); assert.ok(result.explanation); - assert.strictEqual(result.extendedInfo.value.failures.length, 4); + assert.strictEqual(result.details.items[0].failures.length, 4); }); }); @@ -82,7 +82,7 @@ describe('PWA: webapp install banner audit', () => { return WebappInstallBannerAudit.audit(artifacts).then(result => { assert.strictEqual(result.rawValue, false); assert.ok(result.explanation.includes('start_url'), result.explanation); - const failures = result.extendedInfo.value.failures; + const failures = result.details.items[0].failures; assert.strictEqual(failures.length, 1, failures); }); }); @@ -95,7 +95,7 @@ describe('PWA: webapp install banner audit', () => { return WebappInstallBannerAudit.audit(artifacts).then(result => { assert.strictEqual(result.rawValue, false); assert.ok(result.explanation.includes('short_name'), result.explanation); - const failures = result.extendedInfo.value.failures; + const failures = result.details.items[0].failures; assert.strictEqual(failures.length, 1, failures); }); }); @@ -107,7 +107,7 @@ describe('PWA: webapp install banner audit', () => { return WebappInstallBannerAudit.audit(artifacts).then(result => { assert.strictEqual(result.rawValue, false); assert.ok(result.explanation.includes('name'), result.explanation); - const failures = result.extendedInfo.value.failures; + const failures = result.details.items[0].failures; assert.strictEqual(failures.length, 1, failures); }); }); @@ -119,7 +119,7 @@ describe('PWA: webapp install banner audit', () => { return WebappInstallBannerAudit.audit(artifacts).then(result => { assert.strictEqual(result.rawValue, false); assert.ok(result.explanation.includes('icons'), result.explanation); - const failures = result.extendedInfo.value.failures; + const failures = result.details.items[0].failures; assert.strictEqual(failures.length, 1, failures); }); }); @@ -133,7 +133,7 @@ describe('PWA: webapp install banner audit', () => { return WebappInstallBannerAudit.audit(artifacts).then(result => { assert.strictEqual(result.rawValue, false); assert.ok(result.explanation.includes('service worker'), result.explanation); - const failures = result.extendedInfo.value.failures; + const failures = result.details.items[0].failures; assert.strictEqual(failures.length, 1, failures); }); }); @@ -145,7 +145,7 @@ describe('PWA: webapp install banner audit', () => { return WebappInstallBannerAudit.audit(artifacts).then(result => { assert.strictEqual(result.rawValue, false); assert.ok(result.explanation.includes('start_url'), result.explanation); - const failures = result.extendedInfo.value.failures; + const failures = result.details.items[0].failures; assert.strictEqual(failures.length, 1, failures); }); }); diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json index 171b40d3015a..79e4235aa6d0 100644 --- a/lighthouse-core/test/results/sample_v2.json +++ b/lighthouse-core/test/results/sample_v2.json @@ -486,7 +486,19 @@ "score": 0, "scoreDisplayMode": "binary", "rawValue": false, - "explanation": "Failures: No manifest was fetched,\nSite does not register a service worker." + "explanation": "Failures: No manifest was fetched,\nSite does not register a service worker.", + "details": { + "items": [ + { + "failures": [ + "No manifest was fetched", + "Site does not register a service worker" + ], + "isParseFailure": true, + "parseFailureReason": "No manifest was fetched" + } + ] + } }, "splash-screen": { "id": "splash-screen", @@ -495,7 +507,18 @@ "score": 0, "scoreDisplayMode": "binary", "rawValue": false, - "explanation": "Failures: No manifest was fetched." + "explanation": "Failures: No manifest was fetched.", + "details": { + "items": [ + { + "failures": [ + "No manifest was fetched" + ], + "isParseFailure": true, + "parseFailureReason": "No manifest was fetched" + } + ] + } }, "themed-omnibox": { "id": "themed-omnibox", @@ -504,7 +527,20 @@ "score": 0, "scoreDisplayMode": "binary", "rawValue": false, - "explanation": "Failures: No manifest was fetched,\nNo `` tag found." + "explanation": "Failures: No manifest was fetched,\nNo `` tag found.", + "details": { + "items": [ + { + "failures": [ + "No manifest was fetched", + "No `` tag found" + ], + "themeColor": null, + "isParseFailure": true, + "parseFailureReason": "No manifest was fetched" + } + ] + } }, "manifest-short-name-length": { "id": "manifest-short-name-length", diff --git a/typings/artifacts.d.ts b/typings/artifacts.d.ts index 53035b0813b4..2223138fd9cf 100644 --- a/typings/artifacts.d.ts +++ b/typings/artifacts.d.ts @@ -307,11 +307,13 @@ declare global { bottomUpGroupBy(grouping: string): DevtoolsTimelineModelNode; } + export type ManifestValueCheckID = 'hasStartUrl'|'hasIconsAtLeast192px'|'hasIconsAtLeast512px'|'hasPWADisplayValue'|'hasBackgroundColor'|'hasThemeColor'|'hasShortName'|'hasName'|'shortNameLength'; + export interface ManifestValues { isParseFailure: boolean; parseFailureReason: string | undefined; allChecks: { - id: string; + id: ManifestValueCheckID; failureText: string; passing: boolean; }[]; diff --git a/typings/audit.d.ts b/typings/audit.d.ts index a0a6c804a562..bf3f006bb67a 100644 --- a/typings/audit.d.ts +++ b/typings/audit.d.ts @@ -145,6 +145,17 @@ declare global { children: SimpleCriticalRequestNode; } } + + type MultiCheckAuditP1 = Partial>; + type MultiCheckAuditP2 = Partial; + interface MultiCheckAuditP3 { + failures: Array; + warnings?: undefined; + manifestValues?: undefined; + allChecks?: undefined; + } + + export type MultiCheckAuditDetails = MultiCheckAuditP1 & MultiCheckAuditP2 & MultiCheckAuditP3; } }