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

Missing Fields in proto! #6362

Closed
2 tasks
exterkamp opened this issue Oct 22, 2018 · 5 comments · Fixed by #6363
Closed
2 tasks

Missing Fields in proto! #6362

exterkamp opened this issue Oct 22, 2018 · 5 comments · Fixed by #6363
Assignees

Comments

@exterkamp
Copy link
Member

What is the current behavior?

Lighthouse_result.proto has some missing fields! Doh!

  • config_settings.audit_mode
  • Timing
@exterkamp exterkamp self-assigned this Oct 22, 2018
@patrickhulce
Copy link
Collaborator

Maybe step one before/while adding these, is adding a round trip assertion test? :)

@exterkamp
Copy link
Member Author

The reason these were missed was because the sample json cleans these two fields out for testing! So I totally missed them!

I support round-tripping the smokehouse JSONs or pulling some real world ones and round tripping them. The diffing gets complicated though. That has been the problem. @paulirish

@patrickhulce
Copy link
Collaborator

Oh I didn't realize we were just going to reuse the existing diff infra for it :)

Since that diffing was made to go between runs where timing/date/etc might end up changing and this one is about detecting any data loss on a roundtrip would it might make sense to use a stricter diff for the protobuf?

@exterkamp
Copy link
Member Author

Yes, we could do a much stricter diff for protobuf. I'm doing that kind of strict diffing for LR, but it should be done for LH too. The current sample roundtrip was always a first step.

@patrickhulce
Copy link
Collaborator

Oh cool sorry I didn't realize we're already doing it for LR!

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 a pull request may close this issue.

2 participants