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

tests(lantern): rebase #10129

Merged
merged 3 commits into from
Dec 23, 2019
Merged

tests(lantern): rebase #10129

merged 3 commits into from
Dec 23, 2019

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Dec 19, 2019

What is looks like today:

image


This is with the new traces, comparing with the old data. I wouldn't give this too much concern. See the side-by-side instead.

lantern-rebase

After rebase:

image

Side by side (before - after):

image

@@ -29,7 +29,7 @@ rm -rf lantern-data/
mkdir -p lantern-data/ && cd lantern-data
echo $VERSION > version

curl -o lantern-traces.tar.gz -L https://storage.googleapis.com/lh-lantern-data/lantern-traces-$VERSION.tar.gz
curl -o golden-lantern-traces.zip -L https://storage.googleapis.com/lh-lantern-data/golden-lantern-traces-$VERSION.zip
Copy link
Collaborator Author

@connorjclark connorjclark Dec 19, 2019

Choose a reason for hiding this comment

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

renamed file b/c we are also keeping the big zip files in this bucket, and I wanted to reuse the naming that the collection scripts use.

@patrickhulce
Copy link
Collaborator

wowzah am I reading this right that the median error for LCP is 108% (though it seems like the p50 and p90 are flipped and it should be 78%?, still...)

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.

would be nice to get a before shot in the PR that has the full dataset for posterity instead of just a couple overlap URLs :)

just curious do these traces also have CLS events for us too? I think the plan is to not have a separate lantern impl for it but it would be nice to have a corpus of those in our golden set already

LGTM 💯

we're living in a new world of chrome changes I guess we have some work ahead of us to get back down! :D

@connorjclark
Copy link
Collaborator Author

would be nice to get a before shot in the PR that has the full dataset for posterity instead of just a couple overlap URLs :)

good point. also updated the side-by-side ..

just curious do these traces also have CLS events for us too? I think the plan is to not have a separate lantern impl for it but it would be nice to have a corpus of those in our golden set already

none of these traces have LayoutShift events. Must not be in stable yet.

@connorjclark
Copy link
Collaborator Author

wowzah am I reading this right that the median error for LCP is 108% (though it seems like the p50 and p90 are flipped and it should be 78%?, still...)

yeah its high. "should" be? It's backwards for sure, but I don't think the percentile maths is wrong or anything.

@patrickhulce
Copy link
Collaborator

"should" be? It's backwards for sure, but I don't think the percentile maths is wrong or anything.

"should be" meaning if it's just flipped then the p50 error is 78%, not 108%.

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

Successfully merging this pull request may close these issues.

3 participants