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

Performance Tests: Improve collection and reporting #61450

Merged
merged 7 commits into from
May 13, 2024

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented May 7, 2024

What?

  • Collect Server-Timing information in performance tests
  • Print test results as GitHub Actions summaries for easier overview

See #33645
Fixes #61411

To-do:

  • Ideally we'd prepare & reset the environment like core does (disable cron, disable external HTTP requests, flush object cache). But I don't know where to start 🤔 The setup is quite different from core's.

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

@swissspidy swissspidy added [Type] Build Tooling Issues or PRs related to build tooling [Type] Performance Related to performance efforts labels May 7, 2024
@swissspidy swissspidy requested a review from joemcgill May 7, 2024 15:07
@swissspidy swissspidy removed the [Type] Build Tooling Issues or PRs related to build tooling label May 7, 2024
@swissspidy swissspidy changed the title Collect Server-Timing metrics Performance Tests: Collect Server-Timing metrics May 7, 2024
@Mamaduka
Copy link
Member

Mamaduka commented May 7, 2024

What's the reason for grouping these two in a single PR?

@swissspidy
Copy link
Member Author

@Mamaduka I'm trying to port over some of the recent improvements from the WP core tests, having everything in one PR is more manageable for testing.

@swissspidy swissspidy changed the title Performance Tests: Collect Server-Timing metrics Performance Tests: Improve collection and reporting May 7, 2024

This comment was marked as resolved.

Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

Nice! The summary on this PR already looks pretty good. A couple non-blocking observations:

  • It would be nice to map the spec names for each table to a more human readable format that were more descriptive (example).
  • I would flip columns for trunk and the current commit so they are consistent with the before/after format used in other places where we are reporting comparisons.
  • Using the full commit hash for the header gets pretty long, possibly truncate to 7 characters, which is the default length for most short style SHA-1 hashes or go with the generic "before" and "after" like we do elsewhere.

@swissspidy
Copy link
Member Author

  • It would be nice to map the spec names for each table to a more human readable format that were more descriptive (example).

I think this would require more significant changes . The way the performance reporter is setup, the actual test suite names are not really readable nor unique, nor are the names saved to the result files.

  • I would flip columns for trunk and the current commit so they are consistent with the before/after format used in other places where we are reporting comparisons.

This is actually in line with how the data is saved under the hood and presented on the command line when running tests.

See https://github.com/WordPress/gutenberg/actions/runs/8989145786/job/24691572268?pr=61450#step:5:104 for example.

I was thinking about this but this is not guaranteed to be a commit hash. It can also be a branch name or a tag name.

Again, the output is the same on command line as well.

@youknowriad
Copy link
Contributor

TIL about the job summaries, really nice and avoid digging into the logs. The discoverability is increased a lot without adding huge distracting comments to the PR.

Now I wonder if there's a way to make this a little bit more visible in the main PR page or something (a link or something).

@youknowriad
Copy link
Contributor

@WordPress/gutenberg-core We should look at adding this summary to the Gutenberg job as well.

@tyxla
Copy link
Member

tyxla commented May 8, 2024

also cc @jsnajdr who has been looking at reducing performance test noise and we were also discussing surfacing the results to devs in PRs.

@swissspidy swissspidy marked this pull request as ready for review May 10, 2024 13:03
Copy link

github-actions bot commented May 10, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: swissspidy <[email protected]>
Co-authored-by: joemcgill <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: desrosj <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This is a great improvement. Thanks

@Mamaduka
Copy link
Member

Ideally we'd prepare & reset the environment like core does (disable cron, disable external HTTP requests, flush object cache). But I don't know where to start 🤔 The setup is quite different from core's.

Can you share where the core setup handles this?

@swissspidy
Copy link
Member Author

Ideally we'd prepare & reset the environment like core does (disable cron, disable external HTTP requests, flush object cache). But I don't know where to start 🤔 The setup is quite different from core's.

Can you share where the core setup handles this?

In test-performance.yml

Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

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

Thanks for this, @swissspidy!

For anyone with more ideas about how to use job summaries, Trac-56150 is an overarching ticket for proposing new ways to utilize this feature in our workflows managed in WordPress/wordpress-develop.

@swissspidy swissspidy merged commit 1c7ed9f into trunk May 13, 2024
63 of 64 checks passed
@swissspidy swissspidy deleted the try/expand-performance-tests branch May 13, 2024 13:54
@github-actions github-actions bot added this to the Gutenberg 18.4 milestone May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Summaries to Performance Workflow Runs
6 participants