report: better deduping of warnings #12355
Conversation
patrickhulce
left a comment
patrickhulce
left a comment
There was a problem hiding this comment.
nice work! thanks @jckr :)
| const LighthouseRunWarnings = []; | ||
| for (const warning of baseArtifacts.LighthouseRunWarnings) { | ||
| // each entry in baseArtifacts.LighthouseRunWarnings can be a string or a LH.IcuMessage. | ||
| const stringifiedWarning = JSON.stringify(warning); |
There was a problem hiding this comment.
We have the lodash.isequal package in our dependencies which does deep equality like what we're looking for here and isn't sensitive to ordering like a stringify approach is.
Given we tend to have very few warnings here, I'm not as concerned with preserving hashset time complexity :)
WDYT about switching to a LighthouseRunWarnings.some(existing => isDeepEqual(warning, existing) check?
There was a problem hiding this comment.
i see what you mean. updating the branch
paulirish
changed the title
patrickhulce
left a comment
patrickhulce
left a comment
There was a problem hiding this comment.
LGTM 💯
great stuff!
| passContext.LighthouseRunWarnings.push( | ||
| ...runWarnings, | ||
| ...runWarnings, | ||
| // adding one of the warnings with keys in different order |
| // Take only unique LighthouseRunWarnings. | ||
| baseArtifacts.LighthouseRunWarnings = Array.from(new Set(baseArtifacts.LighthouseRunWarnings)); | ||
|
|
||
| /** @type {(string | LH.IcuMessage)[]} */ |
There was a problem hiding this comment.
I'm not sure if you've already been introduced to this minor team war yet ;) but Array<T> vs T[] pops up quite a bit for us.
We typically let it go in reviews and don't have a firm policy on it, so it probably won't come up much again and you'll see mismatched examples throughout the codebase but just in case this was a casual usage and you don't have strong feelings yet, allow me to pitch you on Array<T> for complex, long, or namespaced type, and only use T[] when it's a primitive or simple non-namespaced type 😁 If you've already formed strong opinions here, feel free to ignore :)
Co-authored-by: Patrick Hulce <[email protected]>
|
looks like this is good to go. the type change seems optional. i'm fine with either way. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
Heh, looks like @patrickhulce actually lifted this implementation when he landed c5fddd9#diff-19b3c3e228a1602f508fa6e6df814df9d1e45475f96894455df2f6eb08bf67baR85 However the test wasn't modified. So I've made this PR current and now it's just the test. \o/ |
|
@googlebot I consent. |
4 participants
Summary
This is in response to issue #11514. Currently, gather-runner can dedupe warnings if they are strings, however, they can also be of type LH.IcuMessage, and as a result several such warnings can appear in the final report.
In my previous PR #12351, that problem could appear:
Related Issues/PRs