From 8232811555f1a51a441eb54057fb57f940595105 Mon Sep 17 00:00:00 2001 From: adrianaixba Date: Wed, 26 Aug 2020 13:41:55 -0500 Subject: [PATCH 1/5] modified manifest artifact, added test, exposing url in audit --- lighthouse-core/audits/installable-manifest.js | 5 ++++- lighthouse-core/gather/gather-runner.js | 11 ++++++++--- lighthouse-core/lib/manifest-parser.js | 1 + .../test/audits/installable-manifest-test.js | 10 ++++++++++ lighthouse-core/test/gather/gather-runner-test.js | 5 +++-- 5 files changed, 26 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/audits/installable-manifest.js b/lighthouse-core/audits/installable-manifest.js index 4f2d87813519..da063a3fead5 100644 --- a/lighthouse-core/audits/installable-manifest.js +++ b/lighthouse-core/audits/installable-manifest.js @@ -89,17 +89,20 @@ class InstallableManifest extends MultiCheckAudit { /** * @param {LH.Artifacts} artifacts * @param {LH.Audit.Context} context - * @return {Promise<{failures: Array, manifestValues: LH.Artifacts.ManifestValues}>} + * @return {Promise<{failures: Array, manifestValues: LH.Artifacts.ManifestValues, manifestUrl: string | undefined}>} */ static async audit_(artifacts, context) { const manifestValues = await ManifestValues.request(artifacts, context); const manifestFailures = InstallableManifest.assessManifest(manifestValues); + const manifestUrl = artifacts.WebAppManifest ? artifacts.WebAppManifest.manifestUrl : undefined; + return { failures: [ ...manifestFailures, ], manifestValues, + manifestUrl, }; } } diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 6efc7eeba59e..b9d734ddac4b 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -584,7 +584,9 @@ class GatherRunner { } // Fetch the manifest, if it exists. - baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext); + const manifestResult = await GatherRunner.getWebAppManifest(passContext); + // manifestResult could be null + baseArtifacts.WebAppManifest = manifestResult ? manifestResult.parsedManifest : null; if (baseArtifacts.WebAppManifest) { baseArtifacts.InstallabilityErrors = await GatherRunner.getInstallabilityErrors(passContext); @@ -626,12 +628,15 @@ class GatherRunner { * will have the reason. See manifest-parser.js for more information. * * @param {LH.Gatherer.PassContext} passContext - * @return {Promise} + * @return {Promise<{parsedManifest: LH.Artifacts.Manifest|null, manifestUrl: string|null} | null>} */ static async getWebAppManifest(passContext) { const response = await passContext.driver.getAppManifest(); if (!response) return null; - return manifestParser(response.data, response.url, passContext.url); + const parsedManifest = manifestParser(response.data, response.url, passContext.url); + const manifestUrl = response.url; + const manifestResult = {parsedManifest, manifestUrl}; + return manifestResult; } /** diff --git a/lighthouse-core/lib/manifest-parser.js b/lighthouse-core/lib/manifest-parser.js index df3620b64ea8..7c0c09aa444d 100644 --- a/lighthouse-core/lib/manifest-parser.js +++ b/lighthouse-core/lib/manifest-parser.js @@ -505,6 +505,7 @@ function parse(string, manifestUrl, documentUrl) { raw: string, value: manifest, warning: manifestUrlWarning, + manifestUrl: manifestUrl, }; } diff --git a/lighthouse-core/test/audits/installable-manifest-test.js b/lighthouse-core/test/audits/installable-manifest-test.js index f4ea6a84a306..0b5749f0e5b4 100644 --- a/lighthouse-core/test/audits/installable-manifest-test.js +++ b/lighthouse-core/test/audits/installable-manifest-test.js @@ -44,6 +44,16 @@ describe('PWA: webapp install banner audit', () => { }); }); + it('passes when manifestUrl matches', () => { + const artifacts = generateMockArtifacts(); + const context = generateMockAuditContext(); + + return InstallableManifestAudit.audit(artifacts, context).then(result => { + assert.strictEqual(artifacts.WebAppManifest.manifestUrl, EXAMPLE_MANIFEST_URL); + assert.strictEqual(result.details.items[0].manifestUrl, EXAMPLE_MANIFEST_URL); + }); + }); + it('fails with a non-parsable manifest', () => { const artifacts = generateMockArtifacts('{,:}'); const context = generateMockAuditContext(); diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 0120527eebd2..346a2d7ba826 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -1798,8 +1798,9 @@ describe('GatherRunner', function() { .mockResponse('Page.getInstallabilityErrors', {installabilityErrors: []}); const result = await GatherRunner.getWebAppManifest(passContext); - expect(result).toHaveProperty('raw', JSON.stringify(manifest)); - expect(result && result.value).toMatchObject({ + const parsedManifest = result ? result.parsedManifest : null; + expect(parsedManifest).toHaveProperty('raw', JSON.stringify(manifest)); + expect(parsedManifest && parsedManifest.value).toMatchObject({ name: {value: 'App', raw: 'App'}, start_url: {value: passContext.url, raw: undefined}, }); From 7d35259459f16a3de5bb04701bfb42237d747457 Mon Sep 17 00:00:00 2001 From: adrianaixba Date: Fri, 28 Aug 2020 14:18:02 -0500 Subject: [PATCH 2/5] reverting things on gather-runner --- lighthouse-core/audits/installable-manifest.js | 6 +++--- lighthouse-core/gather/gather-runner.js | 11 +++-------- lighthouse-core/lib/manifest-parser.js | 3 ++- .../test/audits/installable-manifest-test.js | 6 +++--- lighthouse-core/test/gather/gather-runner-test.js | 6 +++--- 5 files changed, 14 insertions(+), 18 deletions(-) diff --git a/lighthouse-core/audits/installable-manifest.js b/lighthouse-core/audits/installable-manifest.js index da063a3fead5..8f2d0fad19d5 100644 --- a/lighthouse-core/audits/installable-manifest.js +++ b/lighthouse-core/audits/installable-manifest.js @@ -89,20 +89,20 @@ class InstallableManifest extends MultiCheckAudit { /** * @param {LH.Artifacts} artifacts * @param {LH.Audit.Context} context - * @return {Promise<{failures: Array, manifestValues: LH.Artifacts.ManifestValues, manifestUrl: string | undefined}>} + * @return {Promise<{failures: Array, manifestValues: LH.Artifacts.ManifestValues, url: string | null}>} */ static async audit_(artifacts, context) { const manifestValues = await ManifestValues.request(artifacts, context); const manifestFailures = InstallableManifest.assessManifest(manifestValues); - const manifestUrl = artifacts.WebAppManifest ? artifacts.WebAppManifest.manifestUrl : undefined; + const manifestUrl = artifacts.WebAppManifest ? artifacts.WebAppManifest.url : null; return { failures: [ ...manifestFailures, ], manifestValues, - manifestUrl, + url: manifestUrl, }; } } diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index b9d734ddac4b..6efc7eeba59e 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -584,9 +584,7 @@ class GatherRunner { } // Fetch the manifest, if it exists. - const manifestResult = await GatherRunner.getWebAppManifest(passContext); - // manifestResult could be null - baseArtifacts.WebAppManifest = manifestResult ? manifestResult.parsedManifest : null; + baseArtifacts.WebAppManifest = await GatherRunner.getWebAppManifest(passContext); if (baseArtifacts.WebAppManifest) { baseArtifacts.InstallabilityErrors = await GatherRunner.getInstallabilityErrors(passContext); @@ -628,15 +626,12 @@ class GatherRunner { * will have the reason. See manifest-parser.js for more information. * * @param {LH.Gatherer.PassContext} passContext - * @return {Promise<{parsedManifest: LH.Artifacts.Manifest|null, manifestUrl: string|null} | null>} + * @return {Promise} */ static async getWebAppManifest(passContext) { const response = await passContext.driver.getAppManifest(); if (!response) return null; - const parsedManifest = manifestParser(response.data, response.url, passContext.url); - const manifestUrl = response.url; - const manifestResult = {parsedManifest, manifestUrl}; - return manifestResult; + return manifestParser(response.data, response.url, passContext.url); } /** diff --git a/lighthouse-core/lib/manifest-parser.js b/lighthouse-core/lib/manifest-parser.js index 7c0c09aa444d..01f1cba9e08d 100644 --- a/lighthouse-core/lib/manifest-parser.js +++ b/lighthouse-core/lib/manifest-parser.js @@ -472,6 +472,7 @@ function parse(string, manifestUrl, documentUrl) { raw: string, value: undefined, warning: 'ERROR: file isn\'t valid JSON: ' + e, + url: manifestUrl, }; } @@ -505,7 +506,7 @@ function parse(string, manifestUrl, documentUrl) { raw: string, value: manifest, warning: manifestUrlWarning, - manifestUrl: manifestUrl, + url: manifestUrl, }; } diff --git a/lighthouse-core/test/audits/installable-manifest-test.js b/lighthouse-core/test/audits/installable-manifest-test.js index 0b5749f0e5b4..7b7c2dc0511d 100644 --- a/lighthouse-core/test/audits/installable-manifest-test.js +++ b/lighthouse-core/test/audits/installable-manifest-test.js @@ -44,13 +44,13 @@ describe('PWA: webapp install banner audit', () => { }); }); - it('passes when manifestUrl matches', () => { + it('passes when manifest url matches', () => { const artifacts = generateMockArtifacts(); const context = generateMockAuditContext(); return InstallableManifestAudit.audit(artifacts, context).then(result => { - assert.strictEqual(artifacts.WebAppManifest.manifestUrl, EXAMPLE_MANIFEST_URL); - assert.strictEqual(result.details.items[0].manifestUrl, EXAMPLE_MANIFEST_URL); + assert.strictEqual(artifacts.WebAppManifest.url, EXAMPLE_MANIFEST_URL); + assert.strictEqual(result.details.items[0].url, EXAMPLE_MANIFEST_URL); }); }); diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 346a2d7ba826..498a86bd0584 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -1798,12 +1798,12 @@ describe('GatherRunner', function() { .mockResponse('Page.getInstallabilityErrors', {installabilityErrors: []}); const result = await GatherRunner.getWebAppManifest(passContext); - const parsedManifest = result ? result.parsedManifest : null; - expect(parsedManifest).toHaveProperty('raw', JSON.stringify(manifest)); - expect(parsedManifest && parsedManifest.value).toMatchObject({ + expect(result).toHaveProperty('raw', JSON.stringify(manifest)); + expect(result && result.value).toMatchObject({ name: {value: 'App', raw: 'App'}, start_url: {value: passContext.url, raw: undefined}, }); + expect(result && result.url).toMatch(MANIFEST_URL); }); }); }); From aff9f37bf010dc936d09f97576fcbcb2235a31fe Mon Sep 17 00:00:00 2001 From: adrianaixba Date: Fri, 28 Aug 2020 15:34:49 -0500 Subject: [PATCH 3/5] added smoke test pwa2 expectation --- .../test/smokehouse/test-definitions/pwa/pwa2-expectations.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa2-expectations.js b/lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa2-expectations.js index d5ec678ade65..00e50b80729a 100644 --- a/lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa2-expectations.js +++ b/lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa2-expectations.js @@ -116,7 +116,7 @@ module.exports = [ }, 'installable-manifest': { score: 1, - details: {items: [pwaDetailsExpectations]}, + details: {items: [{...pwaDetailsExpectations, url: 'https://caltrainschedule.io/manifest.json'}]}, }, 'splash-screen': { score: 1, From 1323a3958a86cb4b3f6ad25c9dc7aaaaebb577ad Mon Sep 17 00:00:00 2001 From: adrianaixba Date: Fri, 28 Aug 2020 15:54:50 -0500 Subject: [PATCH 4/5] manifestUrl --- .../test/smokehouse/test-definitions/pwa/pwa2-expectations.js | 2 +- lighthouse-core/audits/installable-manifest.js | 4 ++-- lighthouse-core/test/audits/installable-manifest-test.js | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa2-expectations.js b/lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa2-expectations.js index 00e50b80729a..d250ae839599 100644 --- a/lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa2-expectations.js +++ b/lighthouse-cli/test/smokehouse/test-definitions/pwa/pwa2-expectations.js @@ -116,7 +116,7 @@ module.exports = [ }, 'installable-manifest': { score: 1, - details: {items: [{...pwaDetailsExpectations, url: 'https://caltrainschedule.io/manifest.json'}]}, + details: {items: [{...pwaDetailsExpectations, manifestUrl: 'https://caltrainschedule.io/manifest.json'}]}, }, 'splash-screen': { score: 1, diff --git a/lighthouse-core/audits/installable-manifest.js b/lighthouse-core/audits/installable-manifest.js index 8f2d0fad19d5..84bed02d579f 100644 --- a/lighthouse-core/audits/installable-manifest.js +++ b/lighthouse-core/audits/installable-manifest.js @@ -89,7 +89,7 @@ class InstallableManifest extends MultiCheckAudit { /** * @param {LH.Artifacts} artifacts * @param {LH.Audit.Context} context - * @return {Promise<{failures: Array, manifestValues: LH.Artifacts.ManifestValues, url: string | null}>} + * @return {Promise<{failures: Array, manifestValues: LH.Artifacts.ManifestValues, manifestUrl: string | null}>} */ static async audit_(artifacts, context) { const manifestValues = await ManifestValues.request(artifacts, context); @@ -102,7 +102,7 @@ class InstallableManifest extends MultiCheckAudit { ...manifestFailures, ], manifestValues, - url: manifestUrl, + manifestUrl, }; } } diff --git a/lighthouse-core/test/audits/installable-manifest-test.js b/lighthouse-core/test/audits/installable-manifest-test.js index 7b7c2dc0511d..501bb986dc66 100644 --- a/lighthouse-core/test/audits/installable-manifest-test.js +++ b/lighthouse-core/test/audits/installable-manifest-test.js @@ -50,7 +50,7 @@ describe('PWA: webapp install banner audit', () => { return InstallableManifestAudit.audit(artifacts, context).then(result => { assert.strictEqual(artifacts.WebAppManifest.url, EXAMPLE_MANIFEST_URL); - assert.strictEqual(result.details.items[0].url, EXAMPLE_MANIFEST_URL); + assert.strictEqual(result.details.items[0].manifestUrl, EXAMPLE_MANIFEST_URL); }); }); From fb3183277c1e65586a345907fd2b0e680abecdf9 Mon Sep 17 00:00:00 2001 From: adrianaixba Date: Fri, 28 Aug 2020 16:20:53 -0500 Subject: [PATCH 5/5] sample_v2 fix --- lighthouse-core/test/results/sample_v2.json | 1 + 1 file changed, 1 insertion(+) diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json index 9dbfae8428e0..ea014b309db6 100644 --- a/lighthouse-core/test/results/sample_v2.json +++ b/lighthouse-core/test/results/sample_v2.json @@ -509,6 +509,7 @@ "failures": [ "No manifest was fetched" ], + "manifestUrl": null, "isParseFailure": true, "parseFailureReason": "No manifest was fetched" }