new_audit(pwa): maskable icon audit #10370

Beytoven · 2020-02-21T23:49:24Z

We want to add a new audit that checks whether a PWA's manifest declares at least one icon as maskable.

fixes #10197

patrickhulce

this looks great @Beytoven! great job covering all our bases with the tests, love it! 🎉

lighthouse-core/audits/maskable-icon.js

lighthouse-core/lib/icons.js

patrickhulce · 2020-02-22T19:25:38Z

lighthouse-core/lib/manifest-parser.js

@@ -241,6 +241,7 @@ function parseIcon(raw, manifestUrl) {
  }

  const type = parseString(raw.type, true);
+  const purpose = parseString(raw.purpose, true);


looks like the default is "any" if we want to stick to the spec

patrickhulce · 2020-02-22T19:31:48Z

lighthouse-core/test/results/sample_v2.json

+      "description": "A maskable icon ensures that the icon fills the entire shape when installing the app on a device. [Learn more](https://web.dev/maskable-icon/).",
+      "score": 0,
+      "scoreDisplayMode": "binary",
+      "explanation": "Failures: No manifest was fetched.",


looks like this used to a multicheck audit so you'll probably need to yarn update:sample-json again :)

lighthouse-core/config/default-config.js

patrickhulce · 2020-02-22T19:42:08Z

lighthouse-core/test/audits/maskable-icon-test.js

+    artifacts.WebAppManifest = null;
+
+    const auditResult = await MaskableIconAudit.audit(artifacts, context);
+    assert.equal(auditResult.score, 0);


🚨 totally optional personal appeal below 🚨

since this is a brand new test file I'll attempt to pitch you on why new tests should use expect instead of assert :)

while it has pretty much no impact on this particular file, expect gives us some nice powers over assert, namely

partial matching
expect({foo: 1, ...giantListOfOtherProperties}).toMatchObject({foo: 1}) will pass and on failure still print out the rest of the object for better debugging

better array length debugging
expect(arr).toHaveLength(5) will print the contents of the array on failure for debugging instead of just saying Expected 5 got 4

snapshot testing
expect(someObject).toMatchInlineSnapshot() is super handy for tests where we don't necessarily have an absolute correct answer but we want to be aware when anything affects the behavior (think lantern metrics, complicated byte or time savings estimation logic, etc) jest automatically manages these golden file expectations inline and can update them with the -u flag to jest

mock assertions
expect(jest.fn()).toHaveBeenCalledWith('foo'), much easier to read and grok than the manual mock functions we have crafted elsewhere with assert

on the whole, most of the codebase is still assert and when adding individual tests that don't benefit from expect to thsoe files, totally expected to follow the surrounding style, but in new places where we might want to reap the benefits of expect I strongly support the use expect

and that's my spiel, feel free to decide for yourself moving forward when to use what appropriately :)

You've sold me! Made the switch.

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

patrickhulce

nice work!! pretty much there just a last spec-nit change

do we happen to know of sites that use a maskable icon correctly we can verify in the wild?

lighthouse-core/lib/manifest-parser.js

patrickhulce · 2020-02-25T03:05:27Z

lighthouse-core/lib/manifest-parser.js

+  const purpose = {
+    raw: raw.purpose,
+    /** @type {string[]} */
+    value: [],


Suggested change

value: [],

value: ['any'],

patrickhulce · 2020-02-25T03:07:09Z

lighthouse-core/test/audits/maskable-icon-test.js

+    artifacts.WebAppManifest = null;
+
+    const auditResult = await MaskableIconAudit.audit(artifacts, context);
+    expect(auditResult.score).toEqual(0);


expect 😍

lighthouse-core/test/lib/icons-test.js

paulirish · 2020-02-25T04:14:23Z

do we happen to know of sites that use a maskable icon correctly we can verify in the wild?

https://squoosh.app is the primary one i know about.

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

Beytoven · 2020-02-25T23:22:31Z

do we happen to know of sites that use a maskable icon correctly we can verify in the wild?

https://squoosh.app is the primary one i know about.

I've verified it's working using the PWA mentioned by Paul above. Added a screenshot to the PR for additional verification. @patrickhulce

patrickhulce

LGTM, nicely done @Beytoven! 🎉

patrickhulce · 2020-02-26T00:21:32Z

lighthouse-core/lib/manifest-parser.js

+  const parsedPurpose = parseString(raw.purpose);
+  const purpose = {
+    raw: raw.purpose,
+    /** @type {string[]} */


I don't think you need this now that it's ['any'] instead of [] but I could be wrong :)

lighthouse-core/test/lib/icons-test.js

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

paulirish · 2020-02-26T03:36:16Z

😷 🎨 🎉

mathiasbynens · 2020-02-27T08:18:02Z

Exciting to see this land 🎉 Nice work @Beytoven!

do we happen to know of sites that use a maskable icon correctly we can verify in the wild?

v8.dev has one as well.

Beytoven added 7 commits February 11, 2020 14:58

Initial commit

0263b06

Adding unit tests for maskable icon

24bdafc

Another test

6932590

More tests and updated fixtures

37549e4

Remove installable manifest core check

186095a

Moved maskable icon check into it's own audit

3b214e0

Lint fixes

838df46

Beytoven requested a review from a team as a code owner February 21, 2020 23:49

Beytoven requested review from patrickhulce and removed request for a team February 21, 2020 23:49

googlebot added the cla: yes label Feb 21, 2020

devtools-bot assigned patrickhulce Feb 21, 2020

devtools-bot added the waiting4reviewer label Feb 21, 2020

paulirish changed the title ~~new_audit(PWA): maskable icon audit~~ new_audit(PWA): maskable icon audi Feb 22, 2020

patrickhulce suggested changes Feb 22, 2020

View reviewed changes

patrickhulce added land-when-ci-is-green waiting4committer and removed waiting4reviewer land-when-ci-is-green labels Feb 22, 2020

Apply Patrick's string suggestions

3f291bb

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to Preview February 24, 2020 19:41 View deployment

Icon purpose defaults to any; improve maskable check

dd820b1

vercel bot deployed to Preview February 24, 2020 22:02 View deployment

paulirish changed the title ~~new_audit(PWA): maskable icon audi~~ new_audit(PWA): maskable icon audit Feb 24, 2020

paulirish changed the title ~~new_audit(PWA): maskable icon audit~~ new_audit(PWA): maskable icon audi Feb 25, 2020

paulirish changed the title ~~new_audit(PWA): maskable icon audi~~ new_audit(pwa): maskable icon audit Feb 25, 2020

Store purpose value as an array in parsed manifest

81c3850

vercel bot deployed to Preview February 25, 2020 02:21 View deployment

Switch asserts to expects

69b5c76

vercel bot deployed to Preview February 25, 2020 02:38 View deployment

patrickhulce reviewed Feb 25, 2020

View reviewed changes

Better parsing of purpose into array

f7c1619

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to Preview February 25, 2020 19:22 View deployment

patrickhulce mentioned this pull request Feb 25, 2020

New "Ready to (automatically) publish in app stores" badge for PWA #10300

Closed

Better purpose value setting

4b701b8

vercel bot deployed to Preview February 25, 2020 23:18 View deployment

patrickhulce approved these changes Feb 26, 2020

View reviewed changes

Update lighthouse-core/test/lib/icons-test.js

08d8b16

Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to Preview February 26, 2020 01:40 View deployment

Remove unnecessary type declaration

8c091e7

vercel bot deployed to Preview February 26, 2020 01:48 View deployment

Beytoven merged commit f150573 into master Feb 26, 2020

Beytoven deleted the maskable_icon branch February 26, 2020 02:25

connorjclark added the new_audit label Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new_audit(pwa): maskable icon audit #10370

new_audit(pwa): maskable icon audit #10370

Beytoven commented Feb 21, 2020 •

edited

Loading

patrickhulce left a comment

patrickhulce Feb 22, 2020

patrickhulce Feb 22, 2020

patrickhulce Feb 22, 2020

Beytoven Feb 25, 2020

patrickhulce left a comment

patrickhulce Feb 25, 2020

patrickhulce Feb 25, 2020

paulirish commented Feb 25, 2020

Beytoven commented Feb 25, 2020

patrickhulce left a comment

patrickhulce Feb 26, 2020

paulirish commented Feb 26, 2020

mathiasbynens commented Feb 27, 2020

new_audit(pwa): maskable icon audit #10370

new_audit(pwa): maskable icon audit #10370

Conversation

Beytoven commented Feb 21, 2020 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce Feb 22, 2020

Choose a reason for hiding this comment

patrickhulce Feb 22, 2020

Choose a reason for hiding this comment

patrickhulce Feb 22, 2020

Choose a reason for hiding this comment

Beytoven Feb 25, 2020

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce Feb 25, 2020

Choose a reason for hiding this comment

patrickhulce Feb 25, 2020

Choose a reason for hiding this comment

paulirish commented Feb 25, 2020

Beytoven commented Feb 25, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce Feb 26, 2020

Choose a reason for hiding this comment

paulirish commented Feb 26, 2020

mathiasbynens commented Feb 27, 2020

Beytoven commented Feb 21, 2020 •

edited

Loading