sort trace by timestamp before calculating FMP #756
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brendankenny
commented
Oct 7, 2016
| }); | ||
| }); | ||
|
|
||
| it('scores a -1 when faulty trace data is present', () => { | ||
| return Audit.audit({boo: 'ya'}).then(response => { |
There was a problem hiding this comment.
this test was not actually testing faulty trace data after the trace artifact format change :)
There was a problem hiding this comment.
Newer tests are more robust. Looking good.
There was a problem hiding this comment.
and my earlier comment is caught by the tests you've written. Gotcha.
addyosmani
reviewed
Oct 7, 2016
| // navigationStart is currently essential to FMP calculation. | ||
| // see: https://github.com/GoogleChrome/lighthouse/issues/753 | ||
| if (!navigationStart) { | ||
| throw new Error('No `navigationStart` event found after `TracingStartedInPage` in trace'); |
There was a problem hiding this comment.
I'm trying to reason what this throw will mean for the reporter output. Talking in person, it looks like it should still result in a -1 debug string.
addyosmani
approved these changes
Oct 7, 2016
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
re: #753
This makes sure events in the trace are sorted by timestamp (
event.ts) before we start digging into FMP. This probably gives us more-1s in extension reports but makes us more correct in the answers we report. Will have to remain this way until we figure out why trace events are timestamped the way they are or #618 with m54.Also adds example trace from extension from #753 with weird timing for testing.