clients(lr): allow for custom config #7613
Conversation
connorjclark
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
| // Clean up the configSettings | ||
| // Note: This is not strictly required for conversion if protobuf parsing is set to | ||
| // 'ignore unknown fields' in the language of conversion. | ||
| if (reportJson.configSettings) { |
There was a problem hiding this comment.
why is this being removed?
There was a problem hiding this comment.
It was pruning the JSON sent back to LR, which would then place it in a Config proto that only had {emulatedFormFactor, locale, onlyCategories}. Since the rest weren't saved it made sense to cut the extras here. With the proto changes, most of the settings (i left out things like output) can be saved, so I'm undoing the pruning. The fields that aren't in the proto are safely ignored when serialized.
There was a problem hiding this comment.
Gotcha, yeah basically I'm wondering why we don't need to prune anymore even though we still don't support 100% of the settings :)
Are the remaining extra properties any less of a concern than it was before? Based on the comments here, it seems like it was still technically unnecessary then but we decided to do it anyway 🤷♂️
There was a problem hiding this comment.
it's just a couple string values now. before it was like, 95% of it. pruning is slightly more unnecessary now :)
|
Forgot about the round trip test :) |
| @@ -33,7 +33,7 @@ def clean(): | |||
| proto_lhr = lhr_pb2.LighthouseResult() | |||
|
|
|||
| # fill proto lhr with data from JSON | |||
| Parse(json.dumps(data), proto_lhr) | |||
There was a problem hiding this comment.
I guess to finally figure out my real concern, is this something that anyone else will now have to change and match or are we the only people ever doing this proto-json conversion :)
apologies for being very slow on the uptake here 😆
There was a problem hiding this comment.
pretty sure the proto stuff has one user: us, with LR.
|
I added back the config setting pruning, since we are punting on the proto changes. I think we could keep the |
connorjclark
changed the title
|
Any more review on this? |
|
Ah sorry connor I didn't flush my comments! I wish GH had an outbox of some sort that showed you all the PRs you had pending comments on :D |
| describe('round trip JSON comparison to everything', () => { | ||
| let sampleJson; | ||
|
|
||
| beforeEach(() => { | ||
| sampleJson = JSON.parse(preprocessor.processForProto(sample)); | ||
|
|
||
| // Proto conversion turns null summaries into an empty object. This is OK, |
There was a problem hiding this comment.
this comment gave me pause because it seems like the process of going through proto conversion turns {} into null, the previous comment suggests this is the case as well.
wouldn't it make more sense to adjust our faulty result instead of the source json?
There was a problem hiding this comment.
The result is fine, but the jest matcher doesn't like {} when it expects null. The distinction is meaningless in proto world but important just for jest.
There was a problem hiding this comment.
I get that, but proto doesn't actually seem to turn null summaries into {} based on the previous comments and the fact that you are turning {} into null in the sample json, which means that proto seems to be turning {} into null, not the other way around...right? or am I getting super confused by the names here and roundtripJson is actually the expectation not the one that went through the proto and back
There was a problem hiding this comment.
ahhhh yes, I got it all mixed up here. updated with what I intended to do
| @@ -55,21 +55,28 @@ describe('round trip JSON comparison subsets', () => { | |||
| }); | |||
|
|
|||
| it('has the same config values', () => { | |||
| expect(roundTripJson.configSettings).toMatchObject(sampleJson.configSettings); | |||
| // Config settings from proto round trip should be a subset of the actual settings. | |||
| expect(sampleJson.configSettings).toMatchObject(roundTripJson.configSettings); | |||
There was a problem hiding this comment.
wait how did it work the other way around then if one is a subset of the other 😆
There was a problem hiding this comment.
These objects are actually 100% equal, so either way would be fine. It was only apparent that the order is wrong when I was allowing more properties to be in the proto result.
so the test change isn't needed, but it is more correct. whatever settings are in the round trip json (from proto) should match the values as found in the originating settings json
There was a problem hiding this comment.
LGTM
It's funny that this runLighthouseInLR is gradually growing to be a mutant of the full module interface.
When/if we feel comfortable shipping a changed interface for LR, we could think about putting the parameters into the same order, move over to the same flags (move categoryIDs in there as onlyCategories, lrDevice as special lr-only presets, though that's really on CliFlags), etc. It's not a huge rush since it's all normalized internal to this file, but it could help with the consistency/documentation standpoint
Summary
Main motivator: Smokerider needs to pass a custom config to Lightrider, otherwise the results do not correspond to the expectations.
But also, hey, a way to configure plugins in Lightrider! Just provide a config that extends the LR preset + adds your plugin coolness.
I can open a WIP internal CL for the LR branch that uses these changes, if necessary to review.
Related Issues/PRs
Just the proto changes: #7614
Internal Googler bug