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

Test comment with WPT link for big metrics #147

Merged
merged 10 commits into from
Oct 14, 2024
Merged

Test comment with WPT link for big metrics #147

merged 10 commits into from
Oct 14, 2024

Conversation

max-ostapenko
Copy link
Contributor

@max-ostapenko max-ostapenko commented Sep 21, 2024

Resolves #146

  • applied formatting (no other changes) to css-variables.js to trigger test results

Test websites:

@max-ostapenko max-ostapenko marked this pull request as ready for review September 21, 2024 21:22
@max-ostapenko
Copy link
Contributor Author

max-ostapenko commented Sep 22, 2024

The point is that there is only one artifact link no matter how many more pages were tested.
So putting it under each toggle separately will be confusing.

And since there is basically no unique content to put under the toggle, I got rid of them in favor of a bullet point.

And maybe we just use original WPT results instead of messing with artifact file.

Maybe this look is nicer?


https://almanac.httparchive.org/en/2022/

WPT test results
Changed custom metrics values:

{
  "_css-variables": {
    "summary": {}
  }
}
https://www.morgenpost.de/politik/article407241629/game-changer-fuer-die-ukraine-bahnt-sich-an-putin-droht.html

WPT test results
Cannot display changed custom metrics due to comment size limits, use test JSON instead.

https://www.example.com

WPT test results
Cannot display changed custom metrics due to comment size limits, use test JSON instead.

@tunetheweb
Copy link
Member

tunetheweb commented Sep 22, 2024

The point is that there is only one artifact link no matter how many more pages were tested. So putting it under each toggle separately will be confusing.

Ah gotcha. Didn’t think of that before.

it kinda looked odd in the changes you’ve made here so far. And inconsistent between the tests that had changes (WPT link in summary) and that didn’t (WPT link not in summary).

And since there is basically no unique content to put under the toggle, I got rid of them in favor of a bullet point.

Well the WPT test link is the unique bit of content.

And maybe we just use original WPT results instead of messing with artifact file.

Yeah to me link would be fine. And maybe instead of changed results it could be “Cannot display changed results due to GitHub comment limits. Use the link instead.” Or something like that.

Maybe this look is nicer?

I think it’s still a bit confusing that the “Change custom metric” doesn’t seem to belong to the first bullet.

Honestly if just have them all under summary/details with the link, and the changed metric (or above message if can’t display those for that test).

@max-ostapenko
Copy link
Contributor Author

Updated example above (toggles, JSON links, no artifacts).
WDYT?

@tunetheweb
Copy link
Member

LGTM!

@github-actions github-actions bot deleted a comment from tunetheweb Oct 14, 2024
Copy link

https://almanac.httparchive.org/en/2022/

WPT test results

Changed custom metrics values:

{
  "_css-variables": {
    "summary": {}
  }
}
https://www.morgenpost.de/politik/article407241629/game-changer-fuer-die-ukraine-bahnt-sich-an-putin-droht.html

WPT test results

Cannot display changed custom metrics due to comment size limits, use test JSON instead.

@max-ostapenko
Copy link
Contributor Author

@tunetheweb please review

@max-ostapenko
Copy link
Contributor Author

max-ostapenko commented Oct 14, 2024

Interestingly add-pr-comment step removed the wrong comment:

@github-actions github-actions bot deleted a comment from tunetheweb 25 minutes ago

Let's see if it will happen again in other PRs.

@tunetheweb
Copy link
Member

WPT test results

Cannot display changed custom metrics due to comment size limits, use test JSON instead.

Been a while since I looked at this, so was confused initially. I wanted to be linked to this screen:
https://webpagetest.httparchive.org/result/241014_QN_5/1/details/#waterfall_view_step1

But neither of the above links does that directly (though fairly easy to get there from first link but I missed that as was distracted by the second link).

WDYT about changing this:

Cannot display changed custom metrics due to comment size limits, use test JSON instead.

To this:

Cannot display changed custom metrics due to comment size limits, use test details or test JSON instead.

@max-ostapenko
Copy link
Contributor Author

A good idea about the details page!
But I would keep the other text as it is to avoid having a third and duplicated link.

@max-ostapenko
Copy link
Contributor Author

document.getElementById("waterfall_view_step1") returns null, so I'll use the next best anchor: https://webpagetest.httparchive.org/result/241014_QN_5/1/details/#result

Copy link

https://almanac.httparchive.org/en/2022/

WPT result details

Changed custom metrics values:

{
  "_css-variables": {
    "summary": {}
  }
}
https://www.morgenpost.de/politik/article407241629/game-changer-fuer-die-ukraine-bahnt-sich-an-putin-droht.html

WPT result details

Cannot display changed custom metrics due to comment size limits, view test JSON instead.

@max-ostapenko
Copy link
Contributor Author

max-ostapenko commented Oct 14, 2024

@tunetheweb with new link we save 2 clicks :)

At the same time I notice that I can get WPT results details page faster than huge JSON rendered

@pmeenan So I thought maybe we could improve custom metrics rendering in WPT, and avoid needing to load full JSON.
Screenshot 2024-10-14 at 18 56 15

@tunetheweb
Copy link
Member

Yeah the fact it's huge (and so I needed to search for the custom metrics) was the main reason I didn't like the JSON.

Can always get that JSON from the link if wanted:

image

@pmeenan
Copy link
Member

pmeenan commented Oct 14, 2024

FWIW, the JSON API takes query params to strip out things and make it a lot smaller and faster.

&requests=0&average=0&standard=0 should make it quite a bit smaller.

@max-ostapenko
Copy link
Contributor Author

https://webpagetest.httparchive.org/jsonResult.php?test=241014_2M_7&pretty=1&requests=0&average=0&standard=0
I didn't notice any difference, still 40.8Mb

Seems these parameters require &pretty=0, which then makes it unreadable.

@max-ostapenko max-ostapenko merged commit b19bc14 into main Oct 14, 2024
5 checks passed
@max-ostapenko max-ostapenko deleted the furious-puma branch October 14, 2024 22:22
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.

Add WebPageTest link when too big
3 participants