core: fix sample_v2 generation, proto errors #8605

brendankenny · 2019-04-25T06:07:34Z

recently we (I) have asked a few PRs to update a subset of example artifacts and then sample_v2.json. However if you try to run yarn update:sample-artifacts and then yarn update:sample-json, you'll notice there are a few errors preventing you from actually doing that :)

This PR

fixes the throttling-method mismatch between the -G and the -A by switching -G to devtools to match -A
adds the optional gather boolean to the proto Timing entry interface so that gatherer entries, when reimported with -A, will still result in a proto-able LHR at the end.

I'm not sure how often that will come up in practice for PSI, but it's possible, and with false/undefined as the default, it's zero overhead when there are no gather props (as there aren't in the current sample_v2.json because the source artifacts have Timing: [])
makes lhr.stackPacks optional, matching the proto's treatment of them as a repeated field. This is probably the easiest resolution to lhr.stackPacks doesn't make the proto round trip #8245 (and solves ensure 4.x LHR and 5.0 report renderer compatibility #8598 for stack packs automatically :), and now folks won't have to keep manually adding that back.

cc @exterkamp

fixes #8245

brendankenny · 2019-04-25T06:07:51Z

package.json

@@ -53,7 +53,7 @@
    "type-check": "tsc -p . && tsc -p lighthouse-viewer/",
    "i18n:checks": "./lighthouse-core/scripts/i18n/assert-strings-collected.sh",
    "i18n:collect-strings": "node lighthouse-core/scripts/i18n/collect-strings.js",
-    "update:sample-artifacts": "node lighthouse-core/scripts/update-report-fixtures.js -G",


doesn't do anything :)

brendankenny · 2019-04-25T06:19:19Z

FAIL lighthouse-core/test/report/proto-test.js
● round trip JSON comparison subsets › has the same top level values
- "stackPacks": Array [],

ok, not being able to even have an empty array might be a bridge too far. Have to think about it :)

patrickhulce · 2019-04-25T14:20:04Z

proto/lighthouse-result.proto

@@ -161,6 +161,9 @@ message LighthouseResult {
      string entry_type = 2;
      google.protobuf.DoubleValue start_time = 3;
      google.protobuf.DoubleValue duration = 4;
+
+      // Whether timing entry was collected during artifact gathering.
+      bool gather = 5;


didn't @exterkamp just remove this ? maybe I'm just misremembering what I saw haha

Ha, unless he had amnesia I don't think so... @exterkamp?

I removed it from my PR so it could be here where it made more sense 😄

patrickhulce · 2019-04-25T14:20:29Z

types/lhr.d.ts

@@ -61,7 +61,7 @@ declare global {
      /** The record of all formatted string locations in the LHR and their corresponding source values. */
      i18n: {rendererFormattedStrings: I18NRendererStrings, icuMessagePaths: I18NMessages};
      /** An array containing the result of all stack packs. */
-      stackPacks: Result.StackPack[];
+      stackPacks?: Result.StackPack[];


wait this had no other impact being required the whole time? 😆

wait this had no other impact being required the whole time? 😆

Yeah, the only code using was treating it as optional anyways, so easy change (or so I thought).

exterkamp

repeated vs. ListValue elides [] and makes the samples different, this doesn't address that? Need to make it be elided in sample as well, not [].

exterkamp · 2019-04-25T16:50:48Z

proto/lighthouse-result.proto

@@ -161,6 +161,9 @@ message LighthouseResult {
      string entry_type = 2;
      google.protobuf.DoubleValue start_time = 3;
      google.protobuf.DoubleValue duration = 4;
+
+      // Whether timing entry was collected during artifact gathering.
+      bool gather = 5;


I removed it from my PR so it could be here where it made more sense 😄

exterkamp · 2019-04-25T16:53:40Z

proto/sample_v2_round_trip.json

    "lighthouseVersion": "4.3.0", 
    "requestedUrl": "http://localhost:10200/dobetterweb/dbw_tester.html", 
-    "runWarnings": [],
-    "stackPacks": [],


This makes the proto round trip test fail b/c empty list is elided. [] == nothing in proto as repeated. This comes back to the whole ListValue thing. See appveyor.

Ha, yes I know. See my comment #8605 (comment) :)

brendankenny · 2019-04-25T19:17:09Z

OK, 2a22744 is the solution I'll advocate for.

Basically the JS side is helping out the proto side by allowing lhr.stackPacks to be optional, but the proto is going to have to meet the JS halfway by acknowledging that to it [] and not defined are equivalent. The end result is that both sides agree that stackPacks: [] and no stackPacks at all should be treated as the same thing, so we'll never have a mismatch.

I've enabled that by modifying the proto test expectation so that if stackPacks is an empty array, it can pass if the round-trip proto has an omitted stackPacks. This has the nice benefit of no additions to proto-preprocessor, which we're still hoping to eliminate over time. And to be conservative, it's doing this only for stackPacks, not all default values in our proto (though maybe we eventually should...).

exterkamp

I like this compromise. Technically from the proto side, if we wanted [] and undefined to be equivalent then we'd use repeated which is what we're doing. The test was the problem, it didn't consider them equivalent, but now it does. So LGTM!

core: fix sample_v2 generation, proto errors

1b450ba

brendankenny requested a review from exterkamp as a code owner April 25, 2019 06:07

brendankenny commented Apr 25, 2019

View reviewed changes

patrickhulce reviewed Apr 25, 2019

View reviewed changes

paulirish approved these changes Apr 25, 2019

View reviewed changes

exterkamp requested changes Apr 25, 2019

View reviewed changes

modify test to allow default array for stackPacks

2a22744

vercel bot deployed to staging April 25, 2019 19:09 View deployment

exterkamp approved these changes Apr 25, 2019

View reviewed changes

brendankenny merged commit 0d1faf7 into master Apr 26, 2019

brendankenny deleted the samplev2 branch April 26, 2019 00:28

exterkamp mentioned this pull request May 5, 2019

audit: add apple-touch-icon audit #8857

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: fix sample_v2 generation, proto errors #8605

core: fix sample_v2 generation, proto errors #8605

brendankenny commented Apr 25, 2019

brendankenny Apr 25, 2019

brendankenny commented Apr 25, 2019

patrickhulce Apr 25, 2019

brendankenny Apr 25, 2019

exterkamp Apr 25, 2019

patrickhulce Apr 25, 2019

brendankenny Apr 25, 2019

exterkamp left a comment •

edited

Loading

exterkamp Apr 25, 2019

exterkamp Apr 25, 2019

brendankenny Apr 25, 2019

brendankenny commented Apr 25, 2019

exterkamp left a comment

core: fix sample_v2 generation, proto errors #8605

core: fix sample_v2 generation, proto errors #8605

Conversation

brendankenny commented Apr 25, 2019

Choose a reason for hiding this comment

brendankenny commented Apr 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exterkamp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Apr 25, 2019

exterkamp left a comment

Choose a reason for hiding this comment

exterkamp left a comment •

edited

Loading