core(script-treemap-data): fix sourceRoot & missing coverage bugs#11825
core(script-treemap-data): fix sourceRoot & missing coverage bugs#11825devtools-bot merged 3 commits intomasterfrom
Conversation
connorjclark
requested review from
patrickhulce
and removed request for
a team
patrickhulce
left a comment
patrickhulce
left a comment
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
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.
does this fixture capture the length of undefined bug?
There was a problem hiding this comment.
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.
(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.
b/c of the JsUsage: {}, before this PR the audit would bail out before processing the source map. now it doesn't.
3 participants
ref #11254
duplicationByPathexpects keys to be modules without a map'ssourceRoot.coursehero-bundle-1coursehero-bundle-1doesn't have a.usagefile, so I removedJsUsagefrom 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
JsUsagegatherer 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.