Skip to content
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

misc(proto): Add proto definition for LHR #6183

Merged
merged 31 commits into from
Oct 16, 2018
Merged

misc(proto): Add proto definition for LHR #6183

merged 31 commits into from
Oct 16, 2018

Conversation

exterkamp
Copy link
Member

@exterkamp exterkamp commented Oct 5, 2018

Summary
Added /proto which contains:

  • proto definition for LHR
  • python test for making a round trip proto test
  • README on how to compile
  • also: made new yarn command yarn compile-proto

Open Items

  • i18n v1 + freeform + expanding for later use
    • Dropping IcuPaths, only keeping RendererFormattedStrings
  • ListValue in i18n
    • ListValues allow a loose definition of icuMessagePaths content
  • changes to LHR -> made preprocessors in tests and in python scripts so that this is not a prerequisite for this change
  • null score are set to 0. :/
    • score and all nullable values set to google.protobuf.Value to be nullable
  • should these fields in the configSettings be nullable? or can they be just, not included in msg
    • additionalTraceCategories
    • blockedUrlPatterns
    • extraHeaders
    • onlyAudits
    • onlyCategories
    • skipAudits
    • ANSWER: NO
  • decide: should we keep all bools as false by wrapping with BoolValue or keep as bool where they will be dropped from output as a default when set to false
    • Keep them false explicitly when in the config!
  • enums causing proto linting errors due to not ALL_CAPS_SNAKE_CASE
  • viewable round trip json files
  • json is 100% consistent with preprocessed sample_v2
  • make ENUM for RuntimeError

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work, excellent turnaround my friend! :D

proto/README.md Outdated Show resolved Hide resolved
proto/lighthouse-response.proto Outdated Show resolved Hide resolved
typings/externs.d.ts Show resolved Hide resolved
proto/lighthouse-response.proto Outdated Show resolved Hide resolved
proto/lighthouse-response.proto Outdated Show resolved Hide resolved
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love our new docstrings, but I wonder if there's some way we could automate keeping them in sync

proto/proto_lhr_validator.py Outdated Show resolved Hide resolved
proto/lighthouse-response.proto Outdated Show resolved Hide resolved

# write round trip json file
with open(path_dir + '/' + 'lhr_round_trip.json', 'w') as f:
json.dump(json_lhr, f, indent=4)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the test going to end up being?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a way for me to rapidly prototype what mods to the sample we needed to make it proto compliant and mess with the JSON easily. This should be converted into a test that makes a round trip diff with the sample json and integrate that into our jest tests IMO I just haven't made that.

.gitignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@googlebot

This comment has been minimized.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff! Lots of stuff. :)

Some comments below, most pretty minor. I didn't review the proto in depth yet. @brendankenny also should dig into that part in particular.


One larger item:

So in this PR we have a proto-preprocessor in two languages. For purely technical debt, it's definitely extra to maintain two copies. :)

I think we could use the JS one for now. And we could still use it in the roundtrip script if we wanted to... an invocation like:

node -e "const jsonStr = require('./lighthouse-core/lib/proto-preprocessor.js').processForProto('{}'); process.stdout.write(jsonStr); "

Or we just string a few npm scripts together since we need this before the make_python_roundtrip script does most of its work. That's probably better. :)

lighthouse-core/lib/proto-preprocessor.js Outdated Show resolved Hide resolved
lighthouse-core/lib/proto-preprocessor.js Outdated Show resolved Hide resolved
lighthouse-core/lib/proto-preprocessor.js Outdated Show resolved Hide resolved
lighthouse-core/lib/proto-preprocessor.js Outdated Show resolved Hide resolved
lighthouse-core/lib/proto-preprocessor.js Outdated Show resolved Hide resolved
proto/scripts/make_python_roundtrip.py Outdated Show resolved Hide resolved
expect(JSON.parse(output)).toMatchObject(expectation);
});

it('removes empty strings', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you'll want another test case here? something involving arrays? details?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a little addition with an array containing an object that has an empty string. It doesn't clear empty strings that are in an array by design though.

proto/scripts/make_python_roundtrip.py Outdated Show resolved Hide resolved
@@ -57,15 +57,21 @@ declare global {
export import CrdpEvents = _CrdpMappings.Events;
export import CrdpCommands = _CrdpMappings.Commands;

/** Simulation settings that control the amount of network & cpu throttling in the run. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx for adding all these! awesome.

We currently dont have a technique to keep these matching the proto ones, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, I think @brendankenny mentions that we don't but should 😢

package.json Outdated Show resolved Hide resolved
@paulirish paulirish merged commit 324b39c into master Oct 16, 2018
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a work in progress PR until it very suddenly wasn't :)

We don't have a workflow set in stone for that, but we've sometimes marked such PRs with a WIP: prefix (which calls it out and prevents merging due to the pr lint), worked on it and called in folks by @name when needed, then, when ready, taking off the "WIP" and announcing ready for review.

PR development can vary quite a bit, but that has served us well in the past to at least give a heads up on the state of things.

Regardless, PR looks good :) I didn't look through the proto definition itself, but I added a few comments on some (fairly trivial) things we should address at some point.

@@ -0,0 +1,378 @@
syntax = "proto3";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need a license header

@@ -0,0 +1,43 @@
import os
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

license header

"minify-latest-run": "./lighthouse-core/scripts/lantern/minify-trace.js ./latest-run/defaultPass.trace.json ./latest-run/defaultPass.trace.min.json && ./lighthouse-core/scripts/lantern/minify-devtoolslog.js ./latest-run/defaultPass.devtoolslog.json ./latest-run/defaultPass.devtoolslog.min.json",
"update:proto": "yarn compile-proto && yarn build-proto",
"compile-proto": "protoc --python_out=./ ./proto/lighthouse-result.proto && mv ./proto/*_pb2.py ./proto/scripts",
"build-proto": "cd proto/scripts && PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION_VERSION=2 python json_roundtrip_via_proto.py"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better name than build-proto? Is it actually building the roundtrip json?

@@ -0,0 +1,72 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't be under test/report/. Should the contents just be in proto-preprocessor-test.js? Or, if the preprocessor is going to eventually go away, seems like it should just be in proto/test/?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants