-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(script-treemap-data): fix sourceRoot & missing coverage bugs #11825
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes are fine AFAICT, just questions around the test case :)
|
||
it('has root nodes', () => { | ||
expect(JSON.stringify(treemapData).length).toMatchInlineSnapshot(`31703`); | ||
expect(treemapData).toMatchSnapshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything about this we can assert? I'm skeptical of the value of massive snapshots that don't have readable summary statistics.
Put another way, how will the next reviewer not named @connorjclark decide if changes to this snapshot are good or bad? (especially when it's auto collapsed in GH 😉 )?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember :) I accept the canary argument for new functionality we haven't seen in the wild yet and/or don't know how it will change in the future, but this is very specific bug fix PR which I would expect to have a more targeted test. Without any assertion it's very difficult for a reviewer to tell if we regressed on this specific point in a +3000/-3000 PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added an assert for the dupe property
let treemapData; | ||
beforeAll(async () => { | ||
const context = {computedCache: new Map()}; | ||
const {map, content} = loadSourceMapFixture('coursehero-bundle-1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this fixture capture the length of undefined
bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm asking if this fixture gets hit by the bugs you are fixing here. i.e. without your logic changes does this test throw and/or have invalid results?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I think it does based on your PR description, but I couldn't tell what exactly happens in this test if you revert the changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b/c of the JsUsage: {}
, before this PR the audit would bail out before processing the source map. now it doesn't.
ref #11254
duplicationByPath
expects keys to be modules without a map'ssourceRoot
.coursehero-bundle-1
coursehero-bundle-1
doesn't have a.usage
file, so I removedJsUsage
from the new test's artifacts. That of course error'd, since it is a required artifact. When I added an empty object instead (JsUsage: {}
) I noticed a bug where the early bailout codelighthouse/lighthouse-core/audits/script-treemap-data.js
Lines 170 to 182 in a589db5
JsUsage
gatherer should always have an array for every entry. Even though it probably doesn't affect anything, the bailout code was slightly wrong so I fixed that here.