Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(entity-classification): classify unknown urls as "unattributable" #15009

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

alexnj
Copy link
Member

@alexnj alexnj commented Apr 22, 2023

Fixes #14996.

Previously, chrome:// and chrome-extension:// protocol URLs, as well as URLs that aren't part of devtoolsLogs would fail the audit. This change classifies all unknown as third party and groups them under "Unattributable".

With the axe chrome extension that @adamraine faced an error in #14996, the report now would look like:
image

Previously, chrome:// and chrome-extension:// protocol URLs, as well as
URLs that arent part of devtoolsLogs would fail the audit. This change
classifies all uknown as 3P and groups them under "Unattributable".
@alexnj alexnj requested a review from a team as a code owner April 22, 2023 00:51
@alexnj alexnj requested review from connorjclark and removed request for a team April 22, 2023 00:51
@alexnj alexnj changed the title core(entity-classification): classify unknown urls as "Unattributable". core(entity-classification): classify unknown urls as "Unattributable" Apr 22, 2023
@alexnj alexnj changed the title core(entity-classification): classify unknown urls as "Unattributable" core(entity-classification): classify unknown urls as "unattributable" Apr 22, 2023
@brendankenny
Copy link
Member

The chrome:// and chrome-extension:// URLs part of this feels a bit weird, since they aren't unattributable, we know exactly what they are.

Not sure if chrome:// URLs are really an issue unless you're actually testing a chrome:// page, but should there be two changes here?

  • chrome-extension:// URLs attributed to Host Chrome Extension(s) or whatever
  • otherwise unknown URLs no longer throw an error and are put under Unattributable?

@alexnj
Copy link
Member Author

alexnj commented Apr 24, 2023

@brendankenny We still recognize chrome:// pages as a supported URL-type, so we don't want the audits to fail if they are trying to classify those URLs.

I'm not sure if we are adding further value by classifying chrome extensions as a separate entity of their own, and we'll have to group all chrome extensions together anyway, so they're likely best left grouped into unattributable entities.

@brendankenny
Copy link
Member

We still recognize chrome:// pages as a supported URL-type, so we don't want the audits to fail if they are trying to classify those URLs

Right, that's what I meant by they're not an issue unless you're actually testing a chrome:// page since AFAIK you can't load resources from there otherwise. Maybe we should treat all chrome:// URLs as the same entity?

I'm not sure if we are adding further value by classifying chrome extensions as a separate entity of their own, and we'll have to group all chrome extensions together anyway, so they're likely best left grouped into unattributable entities.

It makes sense to not make unattributable URLs a fatal error that breaks audits, but that error was added because we believe LH should be able to know where all URLs came from, and examples to the contrary are corner cases that need to be fixed. Here's a corner case :) We know what they are and we should say so.

Grouping them all together seems fine. It's the dev's extensions, so as long as we make that evident (Chrome extension(s) on test machine?), that's a clear handoff of responsibility at that point.

There also doesn't seem to be any downside? An extra string, but makes the classification clearer and more authoritative.

@alexnj
Copy link
Member Author

alexnj commented Apr 24, 2023

Okay let's merge this in so we can fix the audit failure, and I'll follow up with an issue to classify Chrome extensions separately. @brendankenny wdyt?

@brendankenny
Copy link
Member

SGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chrome plugin URLs in audit results break the audit
4 participants