Collapse the 9 manifest PWA audits into 3 by paulirish · Pull Request #1847 · GoogleChrome/lighthouse

paulirish · 2017-03-10T03:47:57Z

updated everything here march 22

Instead of relying on topic grouping, this changes the {add to homescreen, splash screen, omnibox theming} audits to be all or nothing. (Scoring wise, if you're missing manifest short_name, you shouldn't really be getting points for the other manifest items.)

Screenshot of the PWA audits now:

(I stripped out the topics for readability as the double nesting is a little funny)

Functional changes

Checking for display in a2hs/install-banner is back! Based on the resolution in Remove standalone / fullscreen requirement #495 (comment), we accept standalone/fullscreen/minimal-ui.
The 144 and 192 icon checks graduated to 192 and 512 respectively. This fixes the longstanding Audit: icon size coverage #291. Both before and now we're requiring sizes that are larger than you technically need, but that seems OK.
- The 512 icon will catch some sites out. pwa.rocks for example fails now.
Manifest name isn't accepted as a fallback if short_name is missing any longer. Some sites like svgomg fail on this now.

Technical changes

Manifest checks have moved to a computed artifact (manifestValues).
All but one of audits/manifest-*.js have been nuked. (short-name-length remains).
- Their tests have moved to either the manifestValues tests or the PWA audits tests.
the "add to home screen" thing is now corrected referred to as "webapp install banner" in all the places
Flipped back on a skipped test that is good now :)

PTAL!

patrickhulce · 2017-03-10T17:49:57Z

lighthouse-core/audits/service-worker.js

    // artifacts.URL.finalUrl so audit accounts for any redirects.
-    const version = getActivatedServiceWorker(
-        artifacts.ServiceWorker.versions, artifacts.URL.finalUrl);
+    const versions = artifacts.ServiceWorker.versions;


No differences here, right? Just small refactor?

yes. no difference.

patrickhulce · 2017-03-10T17:52:46Z

lighthouse-core/audits/webapp-install-banner.js

+ *   * Site engagement score of 2 or higher
+
+ * This audit covers these requirements with the following exceptions:
+ *   * it doesn't look at standalone/fullscreen


Why? just still working on it?

because #495 and https://twitter.com/triblondon/status/838966967604142080 :)

I'll bring this topic back up on the recent thread.

It seems wrong to not validate something which is in fact a real world requirement.

Maybe now would be a good time to resolve the question of whether Lighthouse is an expression of Chrome's policy, in which case I'm keen to continue to use this forum to advocate for the requirements to change to be in line with web standards, or if it is not, append "... in Google Chrome" to the end of the title of this audit (and I'll toddle off to file a bug in Chrome!).

I'm hopeful of convincing Samsung to drop the fullscreen/standalone requirement for an install prompt.

Also @paulirish thank you for taking note of this point and paying attention to the issue. Appreciated.

ebidel · 2017-03-22T17:57:44Z

@paulirish can you rebase?

paulirish · 2017-03-23T02:35:24Z

Good news, this PR drops 500 lines from the code base. :)
Bad news, we decided to let this PR grow in scope and it's now a fairly large refactor which touches quite a lot of audit & test code.

I have updated this PR's top description with a full description of what's happening here.
#1847 (comment)

Only thing remaining on my list is sorting out the smoke tests, but I should have that done within the hour.

paulirish · 2017-03-23T03:18:10Z

smoke tests are sorted.

ebidel

Looking good.

ebidel · 2017-03-23T16:50:00Z

lighthouse-core/audits/splash-screen.js

+    manifestValues.allChecks
+      .filter(item => splashScreenCheckIds.includes(item.id))
+      .forEach(item => {
+        if (item.passing === false) {


nit: !item.passing. all of your toPasss return bools, ya?

ebidel · 2017-03-23T16:50:42Z

lighthouse-core/audits/splash-screen.js

+    const failures = [];
+
+    return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => {
+      // 1: validate manifest is in order


remove comment? there's only step and the func name is obvi

ebidel · 2017-03-23T17:01:12Z

lighthouse-core/audits/splash-screen.js

+      if (failures.length > 0) {
+        return {
+          rawValue: false,
+          debugString: `Unsatisfied requirements: ${failures.join(', ')}.`,


This look ok when there are a number of failures? Should we line separate?

ebidel · 2017-03-23T17:32:05Z

lighthouse-core/audits/themed-omnibox.js

+    return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => {
+      // 1: validate manifest is in order
+      ThemedOmnibox.assessManifest(manifestValues, failures);
+      // 2: validate we have a SW


validate we have a theme color

ebidel · 2017-03-23T17:37:19Z

lighthouse-core/gather/computed/manifest-values.js

+  /**
+   * Returns results of all manifest checks
+   * @param {Manifest} manifest
+   * @return {<{isParseFailure: !boolean, parseFailureReason: ?string, allChecks: !Array}>}


should this be {{isParseFailure: !boolean, parseFailureReason: ?string, allChecks: !Array}}?

im no closure annotations guru.

ebidel · 2017-03-23T17:39:06Z

lighthouse-core/gather/computed/manifest-values.js

+    let parseFailureReason;
+
+    if (manifest === null) {
+      parseFailureReason = 'Manifest is available';


'Manifest is unavailable'? Maybe better: 'Manifest could not be parsed'

ebidel · 2017-03-23T17:41:01Z

lighthouse-core/gather/computed/manifest-values.js

+      parseFailureReason = 'Manifest is available';
+    }
+    if (manifest && manifest.value === undefined) {
+      parseFailureReason = 'Manifest is parsed as valid JSON';


Maybe I'm confused. The strings don't describe failure conditions.

this is good feedback. I'm going to flip the language in the UI, too.

Failures: Manifest failed to parse as valid JSON. Manifest does not contain a start_url

ebidel · 2017-03-23T17:55:48Z

lighthouse-core/audits/webapp-install-banner.js

+        formatter: Formatter.SUPPORTED_FORMATS.NULL
+      };
+
+      // If we fail, share the failures


the last part of all these audits is the same. I wonder if there's a way to dry that up a bit (possibly other duplicated places). One idea is for these new audits to extend a base audit like Rob did for the a11y audits.

wardpeet · 2017-03-24T21:29:54Z

pinging @paulirish

patrickhulce · 2017-03-27T20:45:00Z

lighthouse-core/audits/splash-screen.js

+      category: 'PWA',
+      name: 'splash-screen',
+      description: 'Configured for a custom splash screen',
+      // When your app launches from a user\'s homescreen, the browser ' 'uses `background_color` to paint the background of the browser ' +'while your app loads for a smooth transition experience. ' + '[Learn more](https://developers.google.com/web/tools/lighthouse/audits/manifest-contains-background_color).',


the browser ' 'uses

?

patrickhulce · 2017-03-27T20:45:50Z

lighthouse-core/audits/splash-screen.js

+  static audit(artifacts) {
+    const failures = [];
+
+    return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => {


thoughts on having these share a base class that does this plumbing and calls assessManifest?

yup. just did this. :)

well. no call directly into assessManifest, as i'm leaving the base audit to be fairly agnostic.

patrickhulce · 2017-03-27T20:50:34Z

lighthouse-core/gather/computed/manifest-values.js

+      {
+        id: 'hasStartUrl',
+        userText: 'Manifest contains `start_url`',
+        toPass: manifest => !!manifest.value.start_url.value


🚲 🏚 nit: feels super audit-y to phrase hasStartUrl as passing in the gatherer since all of these are just facts we're checking for, what about validate/isTrue/something else

i like validate. sgtm.

paulirish · 2017-03-27T21:16:11Z

@ebidel @patrickhulce thanks much for the review!

Updated. Base class done. Inverted the language. Properties renamed for clarity.

patrickhulce · 2017-03-27T22:10:17Z

lighthouse-core/audits/multi-check-audit.js

@@ -0,0 +1,59 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0


ha, I see what you're up to

patrickhulce · 2017-03-27T22:13:06Z

lighthouse-core/audits/multi-check-audit.js

+  }
+
+  /**
+   * @param {!{manifestValues: !Object, failures: !Array<!string>, themeColor: ?string}} result


remove manifestValues and themeColor?

they are sometimes provided. I updated to clarify they are nullable.

ebidel

This is fine by me. We should get this in soon so it can unblock things for me to work on :)

brendankenny

didn't notice you already submitted sometime halfway through this, d'oh. Sorry I took so long to get to this.

Didn't actually look at the manifest checks and if they're the same as before, but I guess I'll stop now :S

brendankenny · 2017-03-28T00:10:23Z

lighthouse-core/audits/manifest-short-name-length.js

  }

+
+  static assessManifest(manifestValues, failures) {


not meant to be here?

brendankenny · 2017-03-28T00:13:19Z

lighthouse-core/audits/multi-check-audit.js

@@ -0,0 +1,59 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


I thought we were going with all or nothing for the license change

brendankenny · 2017-03-28T00:15:21Z

lighthouse-core/audits/multi-check-audit.js

+   * @return {!AuditResult}
+   */
+  static audit(artifacts) {
+    return Promise.resolve(this.audit_(artifacts)).then(result => this.createAuditResult(result));


just make audit_ return a promise? then it's return this.audit_(artifacts).then(...). Or do Promise.resolve().then(_ => this.audit_(artifacts)).then(...) to make sure errors are caught

can also drop the arrow function, .then(this.createAuditResult); since it's static

brendankenny · 2017-03-28T00:19:20Z

lighthouse-core/audits/multi-check-audit.js

+  }
+
+  /**
+   * @param {!{failures: !Array<!string>, themeColor: ?string, manifestValues: ?Object, }} result


you don't use themeColor or manifestValues so can drop from here (which also helps it be a better generic "multi audit" audit base class). @param {{failures: !Array<string>}} result

brendankenny · 2017-03-28T00:21:28Z

lighthouse-core/audits/splash-screen.js

+
+'use strict';
+
+const Audit = require('./multi-check-audit');


should go with MultiCheckAudit so the class hierarchy is clear

brendankenny · 2017-03-28T00:52:21Z

lighthouse-core/gather/computed/manifest-values.js

+    return 'ManifestValues';
+  }
+
+  static get validityIds() {


not used anywhere?

brendankenny · 2017-03-28T00:54:26Z

lighthouse-core/gather/computed/manifest-values.js

+  /**
+   * Returns results of all manifest checks
+   * @param {Manifest} manifest
+   * @return {{isParseFailure: !boolean, parseFailureReason: ?string, allChecks: !Array}}


a little long but handy to document the checks type: @return {{isParseFailure: boolean, parseFailureReason: (string|undefined), allChecks: !Array<{id: string, failureText: string, passing: boolean}>}}

brendankenny · 2017-03-28T00:59:09Z

lighthouse-core/gather/computed/manifest-values.js

+    }
+
+    // manifest is valid, so do the rest of the checks
+    const remainingChecks = ManifestValues.manifestChecks.map(item => {


since all uses of the checks access them strictly by id, does it make sense to switch to an object (with the current ids as the keys) instead of an array?

brendankenny · 2017-03-28T01:01:01Z

lighthouse-core/test/lib/manifest-parser-test.js

-    // https://github.com/GoogleChrome/lighthouse/issues/602
    // 8.10(5)
-    it.skip('falls back to document URL and issues a warning for an invalid URL', () => {
+    it('falls back to document URL and issues a warning for an invalid URL', () => {


yay, one less skip :)

brendankenny · 2017-03-28T01:07:25Z

lighthouse-core/test/audits/splash-screen-test.js

+/* eslint-env mocha */
+describe('PWA: splash screen audit', () => {
+  describe('basics', () => {
+    it('fails if page had no manifest', () => {


it would be nice to move these to be true unit tests, so that the MultiCheckAudit subclasses were just getting back a set of checks with .passing set to true or false, and the unit test could just check if they respond to the correct passing values. Then the actual manifest verification stuff would live in the test for the manifest computed artifact, where the code for that actually lives.

Would also cut down on some of these duplicated tests (e.g. the one off cases) and minimize things like the manifest parser setup even more

paulirish · 2017-03-28T01:58:48Z

@brendankenny thx! just filed #1920 to track these.

Also!! The next ~100 PRs and issues get pretty memorable IDs, so let's make em count! :)
(Not that I'm helping :)

brendankenny mentioned this pull request Mar 10, 2017

Simplify report generation: no pretty print, no formatter classes #1838

Merged

patrickhulce reviewed Mar 10, 2017

View reviewed changes

paulirish force-pushed the manifestvaluescomputed branch from 009853b to 0f76906 Compare March 11, 2017 00:19

brendankenny mentioned this pull request Mar 13, 2017

remove explicit required generateAuditResult() call #1857

Merged

paulirish added this to the Report v2.0alpha milestone Mar 17, 2017

paulirish mentioned this pull request Mar 17, 2017

Collapse Themed omnibox checks into a single audit #1873

Closed

paulirish added the blockingReportRefactor label Mar 17, 2017

paulirish modified the milestone: nevermind Mar 17, 2017

paulirish force-pushed the manifestvaluescomputed branch from 0f76906 to 616d482 Compare March 23, 2017 01:52

paulirish changed the title ~~WIP: Collapse webapp install banner audits into one~~ Collapse the 9 manifest PWA audits into 3 Mar 23, 2017

ebidel suggested changes Mar 23, 2017

View reviewed changes

paulirish mentioned this pull request Mar 27, 2017

Add start_url_works_offline to "User can be prompted to Add to Homescreen" #1903

Closed

paulirish added 13 commits March 27, 2017 13:34

structure for installwebapp audit.

6badee2

start of manifest value as computed artifact

d8b13e1

finish & add tests.

8da5d0d

remove unneccessary audits. keep their tests.

a5714e9

adjust formatter path.

b1517e4

rebase

22ff88c

introduce groups.

a64d011

splash screen.

3c4cb0c

tests for splash are good.

ed444b6

nuke old tests we dont need

d37ee58

add more tests.

f879d4c

update config.

00bf9a8

adapt manifest-short-name

ed4f400

paulirish added 7 commits March 27, 2017 13:34

tests sorted.

ebdabba

remove manifestvalue groups. WIP

e92130f

change manifest parsing failure approach

314c8b1

better handling of missing manifest.

f7c790a

smoke tests rebaselined.

a18c26f

eslint long line.

fea6288

feedback.

da19677

patrickhulce reviewed Mar 27, 2017

View reviewed changes

paulirish added 2 commits March 27, 2017 14:04

invert failure language. adopt audit base class.

05809d4

userText -> failureText. toPass -> validate.

cf3a9fa

paulirish force-pushed the manifestvaluescomputed branch from 159e809 to cf3a9fa Compare March 27, 2017 21:08

cleanup.

9741f53

paulirish force-pushed the manifestvaluescomputed branch from bd6bfd1 to 9741f53 Compare March 27, 2017 21:14

patrickhulce reviewed Mar 27, 2017

View reviewed changes

adjust annotation.

8a5cd3d

ebidel approved these changes Mar 27, 2017

View reviewed changes

ignore multi-check-audit in Runner.getAuditList

bf35e00

paulirish merged commit 5034961 into master Mar 28, 2017

paulirish deleted the manifestvaluescomputed branch March 28, 2017 00:04

brendankenny reviewed Mar 28, 2017

View reviewed changes

paulirish mentioned this pull request Mar 28, 2017

Address PWA9to3 PR feedback from bckenny #1920

Closed

This was referenced Mar 28, 2017

Add PWA check for manifest display: minimal-ui/standalone/fullscreen #1872

Closed

Remove standalone / fullscreen requirement #495

Closed

[meta] I/O Burndown #1937

Closed

paulirish mentioned this pull request Jun 16, 2017

chore(short-name-length-audit): remove dead code #2517

Merged

paulirish added a commit that referenced this pull request Feb 12, 2018

core(multicheck): address feedback from #1847

e7476c7

		@@ -0,0 +1,59 @@
		/**
		* @license Copyright 2017 Google Inc. All Rights Reserved.

Conversation

paulirish commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Functional changes

Technical changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebidel commented Mar 22, 2017

Uh oh!

paulirish commented Mar 23, 2017

Uh oh!

paulirish commented Mar 23, 2017

Uh oh!

ebidel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wardpeet commented Mar 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish commented Mar 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebidel left a comment

Choose a reason for hiding this comment

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

paulirish commented Mar 10, 2017 •

edited

Loading

paulirish Mar 27, 2017 •

edited

Loading

brendankenny Mar 28, 2017 •

edited

Loading