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

PR body parsing improvement for test websites #98

Merged
merged 6 commits into from
Oct 24, 2023
Merged

Conversation

max-ostapenko
Copy link
Contributor

@max-ostapenko max-ostapenko commented Oct 24, 2023

Fixed issues with escaping markup code.

Related to workflow run issues in #96.

@max-ostapenko max-ostapenko marked this pull request as draft October 24, 2023 10:08
@max-ostapenko
Copy link
Contributor Author

@tunetheweb seems like workflows in many cases run from base branch, skipping the changes from the head branch.

Again I expected to test the new parsing right here, but had to use a separate repo.
Hope I covered it all and it will succeed after merging.

@max-ostapenko max-ostapenko marked this pull request as ready for review October 24, 2023 14:08
@tunetheweb
Copy link
Member

@tunetheweb seems like workflows in many cases run from base branch, skipping the changes from the head branch.

Don't understand what this means? I thought that's what the checkout of the head branch gets around:

- name: Checkout
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0

@max-ostapenko max-ostapenko marked this pull request as draft October 24, 2023 15:02
@max-ostapenko
Copy link
Contributor Author

The workflow file is fetched even before we do checkout.
So there is no way to modify it when the workflow is already running.

Oh, ok. So we should be able to test it running with pull_request event.

@github-actions
Copy link

Custom metrics for https://almanac.httparchive.org/en/2022/

WPT test run results: http://webpagetest.httparchive.org/results.php?test=231024_DM_9

Custom metrics for https://www.example.com/#dfsdf=sd

WPT test run results: http://webpagetest.httparchive.org/results.php?test=231024_40_B
Changed custom metrics values:

{}
Custom metrics for https://weston.ruter.net/

WPT test run results: http://webpagetest.httparchive.org/results.php?test=231024_ZW_D
Changed custom metrics values:

{}
Custom metrics for https://weston.ruter.net/about/

WPT test run results: http://webpagetest.httparchive.org/results.php?test=231024_NK_F
Changed custom metrics values:

{}
Custom metrics for https://weston.ruter.net/category/wordpress/

WPT test run results: http://webpagetest.httparchive.org/results.php?test=231024_HH_H
Changed custom metrics values:

{}
Custom metrics for https://weston.ruter.net/2023/07/01/running-the-wordpress-plugin-directory-slurper/

WPT test run results: http://webpagetest.httparchive.org/results.php?test=231024_36_J
Changed custom metrics values:

{}

@max-ostapenko
Copy link
Contributor Author

Test description:

In WordPress specific HTTP Archive research, we've so far typically relied on only home pages of each origin. However, home pages tend to have different layout than many other pages have, and our assumptions on e.g. performance are not segmented in any way regarding content type. It would be helpful to e.g. differentiate between specific performance patterns commonly applied on home pages vs on singular posts.

Therefore this PR introduces a WordPress specific content type detection, based on the body classes that the CMS outputs by default. The PR adds 3 fields with information:

  • contentType: This is the most important field and can have one of the following values:
    • home-blog: The home page showing the blog (of latest posts).
    • home-page: The home page showing a "static front page" (as configured in WordPress).
    • blog: The blog (of latest posts) if configured to not also be the home page (only relevant when the home page is showing a "static front page").
    • singular: Any kind of singular content, e.g. a single post, page, etc.
    • archive: Any kind of archive content, e.g. a post type or taxonomy archive.
    • unknown: None of the above could be detected.
  • postType: The post type of the current content, or empty if not applicable based on the contentType. This field should have a value for any content type, except for a taxonomy archive.
  • taxonomy: The taxonomy of the current content, or empty if not applicable based on the contentType. This field should have a value only for a taxonomy archive.

Some WebPageTest tests with this logic (using @westonruter's site as test candidate):

Test websites:

@max-ostapenko max-ostapenko marked this pull request as ready for review October 24, 2023 15:16
@@ -1,7 +1,7 @@
name: Tests

on:
pull_request_target:
pull_request:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so that's how we test it.
Just revert event name and ready to merge.

Suggested change
pull_request:
pull_request_target:

P.S. had to remove test description so that we don't have a failing check.

@tunetheweb tunetheweb merged commit 77853cc into main Oct 24, 2023
3 checks passed
@tunetheweb tunetheweb deleted the safer-body-parsing branch October 24, 2023 15:39
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.

2 participants