core(inspector-issues): update inspector-issues audit to match gatherer #13948
Conversation
adamread
changed the title
|
Sorry this took literally forever. Something I'm wondering about is adding smoke tests. Some of the issues are hard to find real-life examples for, and others don't seem to be reported in the issues panel yet. Should I be adding smoke tests for everything too? Thanks. |
| issueTypeLowTextContrast: 'Low text contrast', | ||
| /** Issue related to the reduction of information in user-agent strings. */ | ||
| issueTypeNavigatorUserAgent: 'Using information not present in reduced user-agent strings', | ||
| /** Issue related to the document rendering in quirks mode. */ |
There was a problem hiding this comment.
let's suggest to the translator that terms like "legacy" are also a valid descriptor. Or maybe we should backtick quirks...
There was a problem hiding this comment.
(unresolved): do you perhaps have changes you haven't pushed to the branch?
| for (const issue of navigatorUserAgentIssues) { | ||
| const url = issue.url; | ||
| const lineNumber = issue.location?.lineNumber; | ||
| urls.add(`${url}${lineNumber ? `, line ${lineNumber}` : ''}`); |
There was a problem hiding this comment.
this line looks like a string typo?
| return { | ||
| issueType: str_(UIStrings.issueTypeNavigatorUserAgent), | ||
| subItems: { | ||
| type: 'subitems', |
There was a problem hiding this comment.
since this issue type has source line/col information, we can instead use Audit.makeSourceLocation. It should override the default column type here if you do this:
items: dedupedNavigatorUserAgentIssues.map(url => Audit.makeSourceLocation(url, line, column, bundle))
For an example of how to get the bundle value for this script (the script that url references), see violation-audit
Another option is to leave this for another PR / for us to handle (and just add items with the URL, no line/col info for now)
There was a problem hiding this comment.
oh, and the columns here I think are 1-indexed but we use 0-indexed
lighthouse-core/test/audits/dobetterweb/inspector-issues-test.js
Outdated
Show resolved
Hide resolved
|
I don't think we need smoke tests for every single issue type, you're right that would be a lot of code to write. The one big new thing here is issue types with source location information, so perhaps just one for those would be good? Although I've just noticed we have zero smoke tests for any of these at the moment.. |
|
@adamread resolved the conflict with master for you. do you have time to address feedback? If not, we can take over. thanks! |
Summary
Updates the inspector-issues audit to handle all of the issue types currently being picked up in the gatherer.
Related Issues/PRs
fixes #13147