core: new inspector issues gatherer for Audit.IssueAdded events #10664

Beytoven · 2020-04-29T00:37:30Z

This PR adds an Issues gatherer that gets details from the Audit.IssueAdded protocol event. It's currently only used for Samesite cookie issues but will eventually include many other issue types such as mixed content and COEP.

Closes #10592

patrickhulce

I'll leave the review review to the folks you requested, but just wanted to throw that in there :)

patrickhulce · 2020-04-29T01:32:45Z

lighthouse-core/config/default-config.js

@@ -151,6 +151,7 @@ const defaultConfig = {
      'seo/robots-txt',
      'seo/tap-targets',
      'accessibility',
+      'issues',


how would you feel about something slightly more specific like audits-issues or something more closely tying it to audits protocol?

how would you feel about something slightly more specific like audits-issues or something more closely tying it to audits protocol?

+1 on something more specific if we can think of something good, and audits-issues makes sense with the protocol names, but also seems like it could be a confusing name in lighthouse :)

Having issues live in the Audits namespace in CDP makes coming up with a nice name tricky. I was thinking audits-issues as well but audits in this case doesn't make sense in a LH context. I was also thinking of maybe inspector-issues as that's the top-level type for all issues.

I could get behind inspector-issues 👍

agreed audits-issues is a bit weird

inspector-issues SGTM too

(though is inspector too ancient history for most folks these days? something like devtools-issues is basically synonymous and may be more obvious what it's referring to)

I was also thinking of maybe inspector-issues as that's the top-level type for all issues.

actually you already pointed out that's exactly what they're called in the protocol so ignore my earlier parenthetical :)

inspector-issues SGTM

brendankenny · 2020-04-29T20:50:41Z

lighthouse-core/config/default-config.js

@@ -151,6 +151,7 @@ const defaultConfig = {
      'seo/robots-txt',
      'seo/tap-targets',
      'accessibility',
+      'issues',


how would you feel about something slightly more specific like audits-issues or something more closely tying it to audits protocol?

+1 on something more specific if we can think of something good, and audits-issues makes sense with the protocol names, but also seems like it could be a confusing name in lighthouse :)

brendankenny · 2020-04-29T21:30:54Z

types/artifacts.d.ts

@@ -110,6 +110,8 @@ declare global {
      Fonts: Artifacts.Font[];
      /** Information on poorly sized font usage and the text affected by it. */
      FontSize: Artifacts.FontSize;
+      /** The issues surfaced in the devtools Issues panel */
+      Issues: Artifacts.Issue[];


ideally we can start with just what we need in a new artifact and then add to it as our needs grow. In this case I think we're only going to start with a mixed content audit so we don't need same site cookie issues?

Starting minimal keeps the artifacts smaller for when they need to be saved (e.g. a future issue we don't care about (yet) could add data URLs that are humongous, taking up disk space but never looked at) and we don't have to worry about code depending on them and changing our mind later (technically these won't be part of our public artifacts yet but it could happen :)

Rearranging a bit could help audits using Issues, as well. To get the subset they want, any audit using an Issues artifact of this form would have to do something like

const mixedContentIssues = artifacts.Issues .filter(issue => issue.code === 'MixedContentIssue'); for (const issue of mixedContentIssues) { // Check because `mixedContentIssueDetails` is optional if (!issue.details.mixedContentIssueDetails) continue; // use issue.details.mixedContentIssueDetails... }

Instead, another form the artifacts could take would be something like

Issues: { mixedContentIssues: Crdp.Audits.MixedContentIssueDetails[]; }

which moves the filtering to a single place in the gatherer (saving the audits from doing type juggling) and allows for easy extension of the type (just add another issue property) when there's another type of issue we want to write an audit for.

@Beytoven this is the shape I was suggesting fyi

@brendankenny Yeah, Connor and I had discussed this earlier and decided to let the team discuss it on this PR. I like the format that you and Connor have suggested for this use case. Just want to call out the trade off here is having to update the gatherer and type-definition (and by extension tests and mock data) any time a new or existing audit needs to consume an issue that isn't already used by another audit.

connorjclark · 2020-04-30T22:07:48Z

types/artifacts.d.ts

@@ -471,7 +471,11 @@ declare global {
        devicePixelRatio: number;
      }

-      export interface Issue {
+      export interface InspectorIssues {
+        mixedContentIssues: Crdp.Audits.MixedContentIssueDetails[];


connorjclark · 2020-04-30T22:10:32Z

lighthouse-core/gather/gatherers/inspector-issues.js

-      };
-    });
+    /** @type {Array<LH.Crdp.Audits.MixedContentIssueDetails>} */
+    const mixedContentIssues = [];


ideas for making this cleaner, easily extensible later on:

define the artifact obj first.

const artifacts = { mixedContentIssues: [], }

use a for/of loop on this._issues

naming–instead of mixedContentIssues, what about mixedContent or even MixedContent? Code using will then read like for (const issue of InspectIssues.MixedContent) { ...

Just to make sure I'm clear, you mean use the for/of loop in place of the filter/forEach chaining that I have now? Then the inside would look something like w/ your suggestions:

if (issue.code === 'MixedContentIssue') { artifact.MixedContent.push(issue.details.mixedContentIssueDetails); }

Am I getting this right?

connorjclark · 2020-05-01T01:58:52Z

lighthouse-core/gather/gatherers/inspector-issues.js

    };
+
+    for (const issue of this._issues) {
+      if (issue.code === 'MixedContentIssue'


Maybe drop the code check?

I wish the generated types were better here. It doesn't allow for type narrowing. https://github.com/ChromeDevTools/devtools-protocol/blob/master/types/protocol.d.ts#L2846

brendankenny · 2020-05-01T02:50:07Z

types/artifacts.d.ts

@@ -472,6 +474,15 @@ declare global {
        devicePixelRatio: number;
      }

+      export interface InspectorIssues {
+        MixedContent: Crdp.Audits.MixedContentIssueDetails[];


Wait, this should be mixedContent lower case; capitalized first letter is (supposed to be) just for constructors.

Capitalized top-level artifact names was a confusing decision from a realllllly long time ago to get artifact names off of the gatherer constructor that we're stuck with now but we shouldn't extend it to their properties :)

(I'm kind of surprised eslint doesn't flag it, maybe we turned it off because of tests or something)

connorjclark · 2020-05-01T21:58:12Z

lighthouse-core/gather/gatherers/inspector-issues.js

+class InspectorIssues extends Gatherer {
+  constructor() {
+    super();
+    /** @type {Array<LH.Crdp.Audits.InspectorIssue>} */


i think you can remove InspectorIssue from artifacts.d.ts btw

connorjclark · 2020-05-05T22:19:58Z

types/artifacts.d.ts

+        mixedContent: Crdp.Audits.MixedContentIssueDetails[];
+      }
+
+      export interface InspectorIssue {


i meant this can be removed

connorjclark · 2020-05-22T02:37:06Z

lighthouse-cli/test/smokehouse/test-definitions/dobetterweb/dbw-expectations.js

+            insecureURL: 'http://www.mixedcontentexamples.com/Content/Test/steveholt.jpg',
+            mainResourceURL: 'https://www.mixedcontentexamples.com/Test/NonSecureImage',
+            request: {
+              url: 'http://www.mixedcontentexamples.com/Content/Test/steveholt.jpg',


steve holt!

connorjclark

lgtm- can you update the devtools protocol package before merging? no need to add the new issue types, that can come on an as-needed basis

…e test

Beytoven added 5 commits April 21, 2020 16:31

Update devtools protocol dependency

ddb5dc4

WIP adding issues gatherer

1bf24fe

Get Isssues gatherer working

9d99d42

Lint fix

4c69c3e

Make use of Artifacts.Issue type

bd655dd

Beytoven requested a review from a team as a code owner April 29, 2020 00:37

Beytoven requested review from paulirish and removed request for a team April 29, 2020 00:37

googlebot added the cla: yes label Apr 29, 2020

devtools-bot assigned paulirish Apr 29, 2020

devtools-bot added the waiting4reviewer label Apr 29, 2020

I keep forgetting to run yarn lint

f013a1b

vercel bot deployed to Preview April 29, 2020 00:41 View deployment

Beytoven requested a review from connorjclark April 29, 2020 00:41

patrickhulce reviewed Apr 29, 2020

View reviewed changes

brendankenny reviewed Apr 29, 2020

View reviewed changes

Rename Issues to InspectorIssues. Update structure as discussed.

ea82fed

vercel bot deployed to Preview April 30, 2020 21:59 View deployment

connorjclark reviewed Apr 30, 2020

View reviewed changes

Beytoven added 2 commits April 30, 2020 17:54

Quick refactor

2f238ad

Merge branch 'master' into issues-gatherer

3bc3383

vercel bot deployed to Preview May 1, 2020 00:58 View deployment

connorjclark reviewed May 1, 2020

View reviewed changes

brendankenny reviewed May 1, 2020

View reviewed changes

samad123qw approved these changes May 1, 2020

View reviewed changes

Lower-case'd mixed content field; remove issues code check

115feea

vercel bot requested a deployment to Preview May 1, 2020 21:48 Pending

connorjclark requested changes May 1, 2020

View reviewed changes

vercel bot deployed to Preview May 1, 2020 22:09 View deployment

connorjclark changed the title ~~core: add Issues gatherer to collect Audit.IssueAdded events~~ core: new inspector issues gatherer for Audit.IssueAdded events May 4, 2020

connorjclark reviewed May 5, 2020

View reviewed changes

Remove unused InspectorIssue type

f37483f

vercel bot deployed to Preview May 6, 2020 23:08 View deployment

Merge master into branch

dd20e85

vercel bot deployed to Preview May 14, 2020 19:56 View deployment

Working on smoketest

c25aba9

vercel bot deployed to Preview May 22, 2020 00:38 View deployment

Finishing smoke test

7f22368

vercel bot deployed to Preview May 22, 2020 02:06 View deployment

connorjclark reviewed May 22, 2020

View reviewed changes

connorjclark approved these changes May 27, 2020

View reviewed changes

Update devtools protocol dependency; comment out issues gatherer smok…

c83a0e4

…e test

vercel bot deployed to Preview May 28, 2020 20:42 View deployment

Beytoven merged commit 5b90acb into master May 28, 2020

Beytoven deleted the issues-gatherer branch May 28, 2020 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: new inspector issues gatherer for Audit.IssueAdded events #10664

core: new inspector issues gatherer for Audit.IssueAdded events #10664

Beytoven commented Apr 29, 2020 •

edited

Loading

patrickhulce left a comment

patrickhulce Apr 29, 2020

brendankenny Apr 29, 2020

Beytoven Apr 29, 2020

patrickhulce Apr 29, 2020

brendankenny Apr 30, 2020

brendankenny Apr 30, 2020

brendankenny Apr 29, 2020

brendankenny Apr 29, 2020

connorjclark Apr 29, 2020

Beytoven Apr 29, 2020

connorjclark Apr 30, 2020

connorjclark Apr 30, 2020 •

edited

Loading

Beytoven Apr 30, 2020

connorjclark Apr 30, 2020

connorjclark May 1, 2020 •

edited

Loading

brendankenny May 1, 2020

connorjclark May 1, 2020 •

edited

Loading

connorjclark May 5, 2020

connorjclark May 22, 2020

connorjclark left a comment •

edited

Loading

core: new inspector issues gatherer for Audit.IssueAdded events #10664

core: new inspector issues gatherer for Audit.IssueAdded events #10664

Conversation

Beytoven commented Apr 29, 2020 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Apr 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark left a comment • edited Loading

Choose a reason for hiding this comment

Beytoven commented Apr 29, 2020 •

edited

Loading

connorjclark Apr 30, 2020 •

edited

Loading

connorjclark May 1, 2020 •

edited

Loading

connorjclark May 1, 2020 •

edited

Loading

connorjclark left a comment •

edited

Loading