core(installable-manifest): always run InstallabilityErrors in lega…

…cy mode (#13622)
GoogleChrome · May 9, 2022 · f88fd0a · f88fd0a
1 parent 95aba6d
commit f88fd0a
Show file tree

Hide file tree

Showing 10 changed files with 44 additions and 37 deletions.
diff --git a/.github/workflows/smoke.yml b/.github/workflows/smoke.yml
@@ -123,7 +123,7 @@ jobs:
 
     - run: sudo apt-get install xvfb
     - name: yarn smoke --fraggle-rock
-      run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2 --invert-match pwa offline
+      run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2
 
     # Fail if any changes were written to source files.
     - run: git diff --exit-code

diff --git a/lighthouse-cli/test/smokehouse/test-definitions/offline-online-only.js b/lighthouse-cli/test/smokehouse/test-definitions/offline-online-only.js
@@ -68,7 +68,7 @@ const expectations = {
       },
       'installable-manifest': {
         score: 0,
-        details: {items: [{reason: 'No manifest was fetched'}]},
+        details: {items: [{reason: 'Page has no manifest <link> URL'}]},
       },
       'splash-screen': {
         score: 0,

diff --git a/lighthouse-cli/test/smokehouse/test-definitions/pwa-chromestatus.js b/lighthouse-cli/test/smokehouse/test-definitions/pwa-chromestatus.js
@@ -30,7 +30,7 @@ const expectations = {
       },
       'installable-manifest': {
         score: 0,
-        details: {items: [{reason: 'No manifest was fetched'}]},
+        details: {items: [{reason: 'Page has no manifest <link> URL'}]},
       },
       'splash-screen': {
         score: 0,

diff --git a/lighthouse-core/audits/installable-manifest.js b/lighthouse-core/audits/installable-manifest.js
@@ -130,7 +130,7 @@ class InstallableManifest extends Audit {
       failureTitle: str_(UIStrings.failureTitle),
       description: str_(UIStrings.description),
       supportedModes: ['navigation'],
-      requiredArtifacts: ['URL', 'WebAppManifest', 'InstallabilityErrors'],
+      requiredArtifacts: ['WebAppManifest', 'InstallabilityErrors'],
     };
   }
 
@@ -211,7 +211,6 @@ class InstallableManifest extends Audit {
    *
    */
   static async audit(artifacts, context) {
-    const manifestValues = await ManifestValues.request(artifacts, context);
     const {i18nErrors, warnings} = InstallableManifest.getInstallabilityErrors(artifacts);
 
     const manifestUrl = artifacts.WebAppManifest ? artifacts.WebAppManifest.url : null;
@@ -226,10 +225,16 @@ class InstallableManifest extends Audit {
     const errorReasons = i18nErrors.map(reason => {
       return {reason};
     });
-    /** DevTools InstallabilityErrors does not emit an error unless there is a manifest, so include manifestValues's error */
-    if (manifestValues.isParseFailure) {
-      errorReasons.push({
-        reason: manifestValues.parseFailureReason});
+
+    // If InstallabilityErrors is empty, double check ManifestValues to make sure nothing was missed.
+    // InstallabilityErrors can be empty erroneously in our DevTools web tests.
+    if (!errorReasons.length) {
+      const manifestValues = await ManifestValues.request(artifacts, context);
+      if (manifestValues.isParseFailure) {
+        errorReasons.push({
+          reason: manifestValues.parseFailureReason,
+        });
+      }
     }
 
     // Include the detailed pass/fail checklist as a diagnostic.

diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js
@@ -438,10 +438,8 @@ class GatherRunner {
     }
 
     try {
-      if (baseArtifacts.WebAppManifest) {
-        baseArtifacts.InstallabilityErrors = await InstallabilityErrors.getInstallabilityErrors(
-          passContext.driver.defaultSession);
-      }
+      baseArtifacts.InstallabilityErrors = await InstallabilityErrors.getInstallabilityErrors(
+        passContext.driver.defaultSession);
     } catch (err) {
       log.error('GatherRunner InstallabilityErrors', err);
       baseArtifacts.InstallabilityErrors = {

diff --git a/lighthouse-core/test/audits/installable-manifest-test.js b/lighthouse-core/test/audits/installable-manifest-test.js
@@ -59,11 +59,12 @@ describe('PWA: webapp install banner audit', () => {
 
     it('fails with a non-parsable manifest', () => {
       const artifacts = generateMockArtifacts('{,:}');
+      artifacts.InstallabilityErrors.errors.push({errorId: 'manifest-empty', errorArguments: []});
       const context = generateMockAuditContext();
       return InstallableManifestAudit.audit(artifacts, context).then(result => {
         assert.strictEqual(result.score, 0);
         const items = result.details.items;
-        assert.ok(items[0].reason.includes('failed to parse as valid JSON'));
+        expect(items[0].reason).toBeDisplayString(/could not be parsed/);
       });
     });
 

diff --git a/lighthouse-core/test/fixtures/fraggle-rock/reports/sample-flow-result.json b/lighthouse-core/test/fixtures/fraggle-rock/reports/sample-flow-result.json
@@ -362,9 +362,9 @@
             "description": "Service worker is the technology that enables your app to use many Progressive Web App features, such as offline, add to homescreen, and push notifications. With proper service worker and manifest implementations, browsers can proactively prompt users to add your app to their homescreen, which can lead to higher engagement. [Learn more](https://web.dev/installable-manifest/).",
             "score": 0,
             "scoreDisplayMode": "binary",
-            "numericValue": 2,
+            "numericValue": 1,
             "numericUnit": "element",
-            "displayValue": "2 reasons",
+            "displayValue": "1 reason",
             "warnings": [],
             "details": {
               "type": "table",
@@ -378,9 +378,6 @@
               "items": [
                 {
                   "reason": "Page has no manifest <link> URL"
-                },
-                {
-                  "reason": "No manifest was fetched"
                 }
               ],
               "debugData": {
@@ -5202,19 +5199,19 @@
             },
             {
               "startTime": 72,
-              "name": "lh:computed:ManifestValues",
+              "name": "lh:audit:apple-touch-icon",
               "duration": 1,
               "entryType": "measure"
             },
             {
               "startTime": 73,
-              "name": "lh:audit:apple-touch-icon",
+              "name": "lh:audit:splash-screen",
               "duration": 1,
               "entryType": "measure"
             },
             {
               "startTime": 74,
-              "name": "lh:audit:splash-screen",
+              "name": "lh:computed:ManifestValues",
               "duration": 1,
               "entryType": "measure"
             },
@@ -6418,7 +6415,7 @@
             "lighthouse-core/audits/installable-manifest.js | displayValue": [
               {
                 "values": {
-                  "itemCount": 2
+                  "itemCount": 1
                 },
                 "path": "audits[installable-manifest].displayValue"
               }
@@ -16063,9 +16060,9 @@
             "description": "Service worker is the technology that enables your app to use many Progressive Web App features, such as offline, add to homescreen, and push notifications. With proper service worker and manifest implementations, browsers can proactively prompt users to add your app to their homescreen, which can lead to higher engagement. [Learn more](https://web.dev/installable-manifest/).",
             "score": 0,
             "scoreDisplayMode": "binary",
-            "numericValue": 2,
+            "numericValue": 1,
             "numericUnit": "element",
-            "displayValue": "2 reasons",
+            "displayValue": "1 reason",
             "warnings": [],
             "details": {
               "type": "table",
@@ -16079,9 +16076,6 @@
               "items": [
                 {
                   "reason": "Page has no manifest <link> URL"
-                },
-                {
-                  "reason": "No manifest was fetched"
                 }
               ],
               "debugData": {
@@ -20890,19 +20884,19 @@
             },
             {
               "startTime": 70,
-              "name": "lh:computed:ManifestValues",
+              "name": "lh:audit:apple-touch-icon",
               "duration": 1,
               "entryType": "measure"
             },
             {
               "startTime": 71,
-              "name": "lh:audit:apple-touch-icon",
+              "name": "lh:audit:splash-screen",
               "duration": 1,
               "entryType": "measure"
             },
             {
               "startTime": 72,
-              "name": "lh:audit:splash-screen",
+              "name": "lh:computed:ManifestValues",
               "duration": 1,
               "entryType": "measure"
             },
@@ -22102,7 +22096,7 @@
             "lighthouse-core/audits/installable-manifest.js | displayValue": [
               {
                 "values": {
-                  "itemCount": 2
+                  "itemCount": 1
                 },
                 "path": "audits[installable-manifest].displayValue"
               }

diff --git a/lighthouse-core/test/results/artifacts/artifacts.json b/lighthouse-core/test/results/artifacts/artifacts.json
@@ -7,7 +7,12 @@
   "BenchmarkIndex": 1549,
   "WebAppManifest": null,
   "InstallabilityErrors": {
-    "errors": []
+    "errors": [
+      {
+        "errorId": "no-manifest",
+        "errorArguments": []
+      }
+    ]
   },
   "Stacks": [
     {

diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json
@@ -799,7 +799,7 @@
         ],
         "items": [
           {
-            "reason": "No manifest was fetched"
+            "reason": "Page has no manifest <link> URL"
           }
         ],
         "debugData": {
@@ -7773,19 +7773,19 @@
       },
       {
         "startTime": 0,
-        "name": "lh:computed:ManifestValues",
+        "name": "lh:audit:apple-touch-icon",
         "duration": 100,
         "entryType": "measure"
       },
       {
         "startTime": 0,
-        "name": "lh:audit:apple-touch-icon",
+        "name": "lh:audit:splash-screen",
         "duration": 100,
         "entryType": "measure"
       },
       {
         "startTime": 0,
-        "name": "lh:audit:splash-screen",
+        "name": "lh:computed:ManifestValues",
         "duration": 100,
         "entryType": "measure"
       },
@@ -9020,6 +9020,9 @@
       "lighthouse-core/audits/installable-manifest.js | columnValue": [
         "audits[installable-manifest].details.headings[0].text"
       ],
+      "lighthouse-core/audits/installable-manifest.js | no-manifest": [
+        "audits[installable-manifest].details.items[0].reason"
+      ],
       "lighthouse-core/audits/apple-touch-icon.js | failureTitle": [
         "audits[apple-touch-icon].title"
       ],

diff --git a/...m-webtests/webtests/http/tests/devtools/lighthouse/lighthouse-successful-run-expected.txt b/...m-webtests/webtests/http/tests/devtools/lighthouse/lighthouse-successful-run-expected.txt
@@ -128,6 +128,7 @@ Gathering: SourceMaps
 Gathering: FullPageScreenshot
 Populate base artifacts
 Get webapp manifest
+Get webapp installability errors
 Collect stacks
 Running offlinePass pass
 Resetting state with about:blank