From 64b015e2f265011e8601b0cb9083fa323830e44c Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 11 Jul 2017 16:10:21 -0700 Subject: [PATCH] refactor(StartUrl): switch from error to debugString object (#2549) * refactor(StartUrl): switch from error to debugString object * feedback * allow debugString during passing * fix test * pre-existing bug fix * more feedback * more more feedback --- lighthouse-core/audits/multi-check-audit.js | 8 +++- .../audits/webapp-install-banner.js | 33 ++++++++------- lighthouse-core/gather/driver.js | 29 +++++--------- lighthouse-core/gather/gatherers/manifest.js | 11 ++--- lighthouse-core/gather/gatherers/start-url.js | 40 ++++++++++++++----- lighthouse-core/lib/url-shim.js | 2 +- .../test/audits/webapp-install-banner-test.js | 16 ++++++-- .../test/gather/gatherers/manifest-test.js | 2 +- .../test/gather/gatherers/start-url-test.js | 30 ++++++-------- 9 files changed, 99 insertions(+), 72 deletions(-) diff --git a/lighthouse-core/audits/multi-check-audit.js b/lighthouse-core/audits/multi-check-audit.js index 0e2e98cb4ec6..43ddab901f4c 100644 --- a/lighthouse-core/audits/multi-check-audit.js +++ b/lighthouse-core/audits/multi-check-audit.js @@ -38,10 +38,16 @@ class MultiCheckAudit extends Audit { }; } + let debugString; + if (result.warnings && result.warnings.length > 0) { + debugString = `Warnings: ${result.warnings.join(', ')}`; + } + // Otherwise, we pass return { rawValue: true, - extendedInfo + extendedInfo, + debugString, }; } diff --git a/lighthouse-core/audits/webapp-install-banner.js b/lighthouse-core/audits/webapp-install-banner.js index 28ac2169d98c..62b1d0b289bc 100644 --- a/lighthouse-core/audits/webapp-install-banner.js +++ b/lighthouse-core/audits/webapp-install-banner.js @@ -46,7 +46,8 @@ class WebappInstallBanner extends MultiCheckAudit { }; } - static assessManifest(manifestValues, failures) { + static assessManifest(artifacts, result) { + const {manifestValues, failures} = result; if (manifestValues.isParseFailure) { failures.push(manifestValues.parseFailureReason); return; @@ -69,32 +70,36 @@ class WebappInstallBanner extends MultiCheckAudit { } - static assessServiceWorker(artifacts, failures) { + static assessServiceWorker(artifacts, result) { const hasServiceWorker = SWAudit.audit(artifacts).rawValue; if (!hasServiceWorker) { - failures.push('Site does not register a Service Worker'); + result.failures.push('Site does not register a Service Worker'); } } - static assessOfflineStartUrl(artifacts, failures) { - const hasOfflineStartUrl = artifacts.StartUrl === 200; + static assessOfflineStartUrl(artifacts, result) { + const hasOfflineStartUrl = artifacts.StartUrl.statusCode === 200; + if (!hasOfflineStartUrl) { - failures.push('Manifest start_url is not cached by a Service Worker'); + result.failures.push('Manifest start_url is not cached by a Service Worker'); + } + + if (artifacts.StartUrl.debugString) { + result.warnings.push(artifacts.StartUrl.debugString); } } static audit_(artifacts) { const failures = []; + const warnings = []; return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => { - WebappInstallBanner.assessManifest(manifestValues, failures); - WebappInstallBanner.assessServiceWorker(artifacts, failures); - WebappInstallBanner.assessOfflineStartUrl(artifacts, failures); - - return { - failures, - manifestValues - }; + const result = {warnings, failures, manifestValues}; + WebappInstallBanner.assessManifest(artifacts, result); + WebappInstallBanner.assessServiceWorker(artifacts, result); + WebappInstallBanner.assessOfflineStartUrl(artifacts, result); + + return result; }); } } diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 7a4ec381129e..0bc42941b812 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -265,25 +265,18 @@ class Driver { } getAppManifest() { - return new Promise((resolve, reject) => { - this.sendCommand('Page.getAppManifest') - .then(response => { - // We're not reading `response.errors` however it may contain critical and noncritical - // errors from Blink's manifest parser: - // https://chromedevtools.github.io/debugger-protocol-viewer/tot/Page/#type-AppManifestError - if (!response.data) { - if (response.url) { - return reject(new Error(`Unable to retrieve manifest at ${response.url}.`)); - } - - // If both the data and the url are empty strings, the page had no manifest. - return reject('No web app manifest found.'); - } + return this.sendCommand('Page.getAppManifest') + .then(response => { + // We're not reading `response.errors` however it may contain critical and noncritical + // errors from Blink's manifest parser: + // https://chromedevtools.github.io/debugger-protocol-viewer/tot/Page/#type-AppManifestError + if (!response.data) { + // If the data is empty, the page had no manifest. + return null; + } - resolve(response); - }) - .catch(err => reject(err)); - }); + return response; + }); } getSecurityState() { diff --git a/lighthouse-core/gather/gatherers/manifest.js b/lighthouse-core/gather/gatherers/manifest.js index ff6612dc8af5..131ef7dd15f3 100644 --- a/lighthouse-core/gather/gatherers/manifest.js +++ b/lighthouse-core/gather/gatherers/manifest.js @@ -27,19 +27,16 @@ class Manifest extends Gatherer { afterPass(options) { return options.driver.getAppManifest() .then(response => { + if (!response) { + return null; + } + const isBomEncoded = response.data.charCodeAt(0) === BOM_FIRSTCHAR; if (isBomEncoded) { response.data = Buffer.from(response.data).slice(BOM_LENGTH).toString(); } return manifestParser(response.data, response.url, options.url); - }) - .catch(err => { - if (err === 'No web app manifest found.') { - return null; - } - - return Promise.reject(err); }); } } diff --git a/lighthouse-core/gather/gatherers/start-url.js b/lighthouse-core/gather/gatherers/start-url.js index fa2d283abeef..4548c51763c6 100644 --- a/lighthouse-core/gather/gatherers/start-url.js +++ b/lighthouse-core/gather/gatherers/start-url.js @@ -49,26 +49,29 @@ class StartUrl extends Gatherer { pass(options) { return options.driver.getAppManifest() .then(response => { - return manifestParser(response.data, response.url, options.url); + return response && manifestParser(response.data, response.url, options.url); }) .then(manifest => { - if (!manifest.value.start_url || !manifest.value.start_url.raw) { - return Promise.reject(new Error(`No web app manifest found on page ${options.url}`)); + if (!manifest || !manifest.value) { + const detailedMsg = manifest && manifest.debugString; + this.debugString = detailedMsg ? + `Error fetching web app manifest: ${detailedMsg}` : + `No usable web app manifest found on page ${options.url}`; + return; } if (manifest.value.start_url.debugString) { - return Promise.reject(new Error(manifest.value.start_url.debugString)); + // Even if the start URL had an error, the browser will still supply a fallback URL. + // Therefore, we only set the debugString here and continue with the fetch. + this.debugString = manifest.value.start_url.debugString; } this.startUrl = manifest.value.start_url.value; - }).then(_ => this.executeFetchRequest(options.driver, this.startUrl)); + return this.executeFetchRequest(options.driver, this.startUrl); + }); } afterPass(options, tracingData) { - if (!this.startUrl) { - return Promise.reject(new Error('No start_url found inside the manifest')); - } - const networkRecords = tracingData.networkRecords; const navigationRecord = networkRecords.filter(record => { return URL.equalWithExcludedFragments(record._url, this.startUrl) && @@ -76,7 +79,24 @@ class StartUrl extends Gatherer { }).pop(); // Take the last record that matches. return options.driver.goOnline(options) - .then(_ => navigationRecord ? navigationRecord.statusCode : -1); + .then(_ => { + if (!this.startUrl) { + return { + statusCode: -1, + debugString: this.debugString || 'No start URL to fetch', + }; + } else if (!navigationRecord) { + return { + statusCode: -1, + debugString: this.debugString || 'Did not fetch start URL from service worker', + }; + } else { + return { + statusCode: navigationRecord.statusCode, + debugString: this.debugString, + }; + } + }); } } diff --git a/lighthouse-core/lib/url-shim.js b/lighthouse-core/lib/url-shim.js index f78898bbb390..e33b5af4d1fc 100644 --- a/lighthouse-core/lib/url-shim.js +++ b/lighthouse-core/lib/url-shim.js @@ -103,7 +103,7 @@ URL.elideDataURI = function elideDataURI(url) { // Why? Special handling was added by Chrome team to allow a pushState transition between chrome:// pages. // As a result, the network URL (chrome://chrome/settings/) doesn't match the final document URL (chrome://settings/). function rewriteChromeInternalUrl(url) { - if (!url.startsWith('chrome://')) return url; + if (!url || !url.startsWith('chrome://')) return url; return url.replace(/^chrome:\/\/chrome\//, 'chrome://'); } diff --git a/lighthouse-core/test/audits/webapp-install-banner-test.js b/lighthouse-core/test/audits/webapp-install-banner-test.js index 58763b7bb011..8ff037f10450 100644 --- a/lighthouse-core/test/audits/webapp-install-banner-test.js +++ b/lighthouse-core/test/audits/webapp-install-banner-test.js @@ -26,7 +26,7 @@ function generateMockArtifacts() { scriptURL: 'https://example.com/sw.js' }] }, - StartUrl: 200, + StartUrl: {statusCode: 200}, URL: {finalUrl: 'https://example.com'} })); const mockArtifacts = Object.assign({}, computedArtifacts, clonedArtifacts); @@ -128,7 +128,7 @@ describe('PWA: webapp install banner audit', () => { it('fails if page had no SW', () => { const artifacts = generateMockArtifacts(); artifacts.ServiceWorker.versions = []; - artifacts.StartUrl = -1; + artifacts.StartUrl = {statusCode: -1}; return WebappInstallBannerAudit.audit(artifacts).then(result => { assert.strictEqual(result.rawValue, false); @@ -141,7 +141,7 @@ describe('PWA: webapp install banner audit', () => { it('fails if start_url is not cached', () => { const artifacts = generateMockArtifacts(); - artifacts.StartUrl = -1; + artifacts.StartUrl = {statusCode: -1}; return WebappInstallBannerAudit.audit(artifacts).then(result => { assert.strictEqual(result.rawValue, false); @@ -150,4 +150,14 @@ describe('PWA: webapp install banner audit', () => { assert.strictEqual(failures.length, 1, failures); }); }); + + it('includes debugString from start_url', () => { + const artifacts = generateMockArtifacts(); + artifacts.StartUrl = {statusCode: 200, debugString: 'Warning!'}; + + return WebappInstallBannerAudit.audit(artifacts).then(result => { + assert.strictEqual(result.rawValue, true); + assert.equal(result.debugString, 'Warnings: Warning!'); + }); + }); }); diff --git a/lighthouse-core/test/gather/gatherers/manifest-test.js b/lighthouse-core/test/gather/gatherers/manifest-test.js index 1b48e051f136..ccf0acc722db 100644 --- a/lighthouse-core/test/gather/gatherers/manifest-test.js +++ b/lighthouse-core/test/gather/gatherers/manifest-test.js @@ -97,7 +97,7 @@ describe('Manifest gatherer', () => { return manifestGather.afterPass({ driver: { getAppManifest() { - return Promise.reject('No web app manifest found.'); + return Promise.resolve(null); } } }).then(artifact => { diff --git a/lighthouse-core/test/gather/gatherers/start-url-test.js b/lighthouse-core/test/gather/gatherers/start-url-test.js index efbe07b68a76..49b89e6084ce 100644 --- a/lighthouse-core/test/gather/gatherers/start-url-test.js +++ b/lighthouse-core/test/gather/gatherers/start-url-test.js @@ -90,8 +90,10 @@ describe('Start-url gatherer', () => { startUrlGathererWithQueryString.pass(optionsWithQueryString) .then(_ => startUrlGathererWithQueryString.afterPass(optionsWithQueryString, tracingData)) ]).then(([artifact, artifactWithQueryString]) => { - assert.strictEqual(artifact, -1); - assert.strictEqual(artifactWithQueryString, -1); + assert.equal(artifact.statusCode, -1); + assert.ok(artifact.debugString, 'did not set debug string'); + assert.equal(artifactWithQueryString.statusCode, -1); + assert.ok(artifactWithQueryString.debugString, 'did not set debug string'); }); }); @@ -113,25 +115,22 @@ describe('Start-url gatherer', () => { startUrlGathererWithFragment.pass(optionsWithQueryString) .then(_ => startUrlGathererWithFragment.afterPass(optionsWithQueryString, tracingData)) ]).then(([artifact, artifactWithFragment]) => { - assert.strictEqual(artifact, 200); - assert.strictEqual(artifactWithFragment, 200); + assert.equal(artifact.statusCode, 200); + assert.equal(artifactWithFragment.statusCode, 200); }); }); - it('returns an error when manifest cannot be found', () => { + it('returns a debugString when manifest cannot be found', () => { const startUrlGatherer = new StartUrlGatherer(); const options = { url: 'https://ifixit-pwa.appspot.com/', driver: wrapSendCommand(mockDriver, '') }; - startUrlGatherer.pass(options) + return startUrlGatherer.pass(options) .then(_ => startUrlGatherer.afterPass(options, tracingData)) - .then(_ => { - assert.ok(false, 'should fail because manifest is empty'); - }) - .catch(err => { - assert.strictEqual(err.message, `No web app manifest found on page ${options.url}`); + .then(artifact => { + assert.equal(artifact.debugString, 'ERROR: start_url string empty'); }); }); @@ -142,13 +141,10 @@ describe('Start-url gatherer', () => { driver: wrapSendCommand(mockDriver, 'https://not-same-origin.com/') }; - startUrlGatherer.pass(options) + return startUrlGatherer.pass(options) .then(_ => startUrlGatherer.afterPass(options, tracingData)) - .then(_ => { - assert.ok(false, 'should fail because origin is not the same'); - }) - .catch(err => { - assert.strictEqual(err.message, 'ERROR: start_url must be same-origin as document'); + .then(artifact => { + assert.equal(artifact.debugString, 'ERROR: start_url must be same-origin as document'); }); }); });