core(script-treemap-data): fix sourceRoot & missing coverage bugs

[connorjclark]

ref #11254

• duplicationByPath expects keys to be modules without a map's sourceRoot.
• While fixing this, I also added a second fixture test using coursehero-bundle-1
• coursehero-bundle-1 doesn't have a .usage file, so I removed JsUsage 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 code

  lighthouse/lighthouse-core/audits/script-treemap-data.js

  Lines 170 to 182 in a589db5

  	if (!bundle || !scriptCoverages) {
  	// No bundle or coverage information, so simply make a single node
  	// detailing how big the script is.
  	rootNodeContainers.push({
  	name,
  	node: {
  	name,
  	resourceBytes: scriptElement.src.length,
  	},
  	});
  	continue;
  	}

  bailed out too early ... even though a map was present, it did not create a module tree. The intention was to add as much data to the root node as possible–during the original PR a refactor simplified things but messed up some boolean logic.
• The prior is kinda a moot point, since the 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.

[patrickhulce]

changes are fine AFAICT, just questions around the test case :)

[patrickhulce]

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 😉 )?

[connorjclark]

#11271 (comment)

[patrickhulce]

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.

[connorjclark]

added an assert for the dupe property

[patrickhulce]

does this fixture capture the length of undefined bug?

[connorjclark]

what?

[patrickhulce]

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?

[patrickhulce]

(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)

[connorjclark]

b/c of the JsUsage: {}, before this PR the audit would bail out before processing the source map. now it doesn't.