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.

Sign up for GitHub

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

Jump to bottom

Refactor performance tests setup #49238

Closed
wants to merge 18 commits into from
Closed

Refactor performance tests setup #49238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8326525
Parallelize perf test branch builds
WunderBart Mar 21, 2023
0a2195b
Try enabling colors in CI
WunderBart Mar 21, 2023
79ea0b0
Try using current branch as env and tests source
WunderBart Mar 29, 2023
f6464ea
Streamling & improve local running + clean up
WunderBart Mar 29, 2023
f66c711
Fix refs grouping
WunderBart Mar 30, 2023
18d7449
Fix log indents
WunderBart Mar 30, 2023
b9da0d3
Undo tmp change
WunderBart Mar 30, 2023
3a1259e
Don't install deps twice
WunderBart Mar 30, 2023
0484374
Fix wp version log
WunderBart Mar 30, 2023
072a7bb
Add writeJSONFile util
WunderBart Apr 12, 2023
2638476
Tweak the logic and clean up + add more useful logs
WunderBart Apr 12, 2023
fd35da9
Format some logs
WunderBart Apr 12, 2023
1e1563b
Update CI workflow to work with the new logic
WunderBart Apr 13, 2023
53cf2a1
Merge remote-tracking branch 'origin' into refactor/perf-tests-tmp-fo…
WunderBart Apr 13, 2023
62f5d99
Use existing modules if package locks match
WunderBart Apr 13, 2023
abb3df1
A few last logging tweaks
WunderBart Apr 13, 2023
166091c
Revert "Use existing modules if package locks match"
WunderBart Apr 13, 2023
0affd72
Make multiline log simpler
WunderBart Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions .github/workflows/performance.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ jobs:

- name: Compare performance with trunk
if: github.event_name == 'pull_request'
run: ./bin/plugin/cli.js perf $GITHUB_SHA trunk --tests-branch $GITHUB_SHA
run: ./bin/plugin/cli.js perf trunk
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we ensure that we run the performance tests that are on the PR. For instance when we make changes to the tests themselves?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh If I'm reading this, there's now an assumption that the perf cli.js is included within a Gutenberg plugin and that we need we compare with the current git hash automatically and use it also as tests branch.

One of the initial goal of the cli is to actually be "independent", while it lives in the Gutenberg plugin, it doesn't have any requirement in the sense that the current branch could be broken, have edits to Gutenberg code... it doesn't matter, the tool would always perform a clone. In fact one goal was to make it independent of Gutenberg entirely and be a tool any plugin can use. It seems the current PR goes into the opposite direction.

Copy link
Member Author

@WunderBart WunderBart Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For the general direction of this PR, see my answer here.

(…) In fact one goal was to make it independent of Gutenberg entirely and be a tool any plugin can use.

I'm struggling to visualize the full picture of how the perf tests CLI runner, as an independent tool, would be utilized by other plugins. Would it not be running from the local gutenberg repo then, but would still be comparing gutenberg branches? It would be really helpful if you could give an example of a plugin development scenario where this tool is used 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the initial goal was to be an independent tool, something like so:

# installing the tool
npm install -g @wordpress/plugin-cli

# writing a config file
touch { repoUrl: "some url", buildCommand: "npm run build", perfCommand: "npm run perf", wpPluginSlug: "someslug" } >> plugin-cli.json

# running the perf command
plugin-cli  ---config plugin-cli.json some-branch some-other-branch

The goal was also for the cli to include more than just perf command, maintaining changeling, releasing to SVN/Github was also a command that was part of the same tool, I see that it was removed now in favor of all the git workflows. I'm kind of sad that it didn't remain as a command to be used by the workflows instead of writing it within the workflow files like we do now.

Anyway, given that the previous direction is also to move away from the goal of being an independent tool, one more step away won't be that harmful here, so I guess feel free to make that change if you think it's a good idea.

Copy link
Member Author

@WunderBart WunderBart Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, the initial goal was to be an independent tool, something like so:

# installing the tool
npm install -g @wordpress/plugin-cli

# writing a config file
touch { repoUrl: "some url", buildCommand: "npm run build", perfCommand: "npm run perf", wpPluginSlug: "someslug" } >> plugin-cli.json

# running the perf command
plugin-cli  ---config plugin-cli.json some-branch some-other-branch

Oh, so by independent, you meant from Gutenberg entirely, which would run perf comparisons for any plugin?

If yes, I think we can still assume and leverage the fact that it will always be running from SOME plugin repo root and that the current branch will be a primary subject of the test. The CLI will still need to run the current plugin's dedicated perf tests, right? So it will be easier to develop/debug those tests with the CLI, and also utilize it by the target plugin's CI, where the current branch is always a primary reference.

Now that I think about it, we can replace Gutenberg with a generic plugin in this refactored script, and it would be all that's needed to make this PR not tie it any further to Gutenberg. There's no check that the current repo is actually Gutenberg.

Also, as I mentioned earlier, we can implement the --from-origin and build everything from GH if we don't want to use the local code.

How does that sound?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me 👍


- name: Compare performance with current WordPress Core and previous Gutenberg versions
if: github.event_name == 'release'
Expand All @@ -48,32 +48,31 @@ jobs:
shell: bash
run: |
IFS=. read -ra PLUGIN_VERSION_ARRAY <<< "$PLUGIN_VERSION"
CURRENT_RELEASE_BRANCH="release/${PLUGIN_VERSION_ARRAY[0]}.${PLUGIN_VERSION_ARRAY[1]}"
PREVIOUS_VERSION_BASE_10=$((PLUGIN_VERSION_ARRAY[0] * 10 + PLUGIN_VERSION_ARRAY[1] - 1))
PREVIOUS_RELEASE_BRANCH="release/$((PREVIOUS_VERSION_BASE_10 / 10)).$((PREVIOUS_VERSION_BASE_10 % 10))"
WP_VERSION=$(awk -F ': ' '/^Tested up to/{print $2}' readme.txt)
IFS=. read -ra WP_VERSION_ARRAY <<< "$WP_VERSION"
WP_MAJOR="${WP_VERSION_ARRAY[0]}.${WP_VERSION_ARRAY[1]}"
./bin/plugin/cli.js perf "wp/$WP_MAJOR" "$PREVIOUS_RELEASE_BRANCH" "$CURRENT_RELEASE_BRANCH" --wp-version "$WP_MAJOR"
./bin/plugin/cli.js perf "$PREVIOUS_RELEASE_BRANCH" "wp/$WP_MAJOR" --wp-version "$WP_MAJOR"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we remove $CURRENT_RELEASE_BRANCH here? I know the release perf tests are broken now but the idea is to have a table that compares the current release, the previous release and the previous WP release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we certain here that $CURRENT_RELEASE_BRANCH is the same thing as $GITHUB_SHA

Copy link
Member Author

@WunderBart WunderBart Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

$CURRENT_RELEASE_BRANCH is actually release/xx.x, which is also $GITHUB_HEAD_REF, not $GUTHUB_SHA. The primary reference for the CI perf in the refactored cli is $GITHUB_HEAD_REF so this comparison will still be, e.g.: release/15.6 vs. release/15.5 vs. wp/6.1. With the refactored CLI we need to only list the refs that we want to compare the current one to.


- name: Compare performance with base branch
if: github.event_name == 'push'
# The base hash used here need to be a commit that is compatible with the current WP version
# The base hash used here need to be a commit that is compatible with the current WP version.
# The current one is debd225d007f4e441ceec80fbd6fa96653f94737 and it needs to be updated every WP major release.
# It is used as a base comparison point to avoid fluctuation in the performance metrics.
run: |
WP_VERSION=$(awk -F ': ' '/^Tested up to/{print $2}' readme.txt)
IFS=. read -ra WP_VERSION_ARRAY <<< "$WP_VERSION"
WP_MAJOR="${WP_VERSION_ARRAY[0]}.${WP_VERSION_ARRAY[1]}"
./bin/plugin/cli.js perf $GITHUB_SHA debd225d007f4e441ceec80fbd6fa96653f94737 --tests-branch $GITHUB_SHA --wp-version "$WP_MAJOR"
./bin/plugin/cli.js perf debd225d007f4e441ceec80fbd6fa96653f94737 --wp-version "$WP_MAJOR"

- name: Compare performance with custom branches
if: github.event_name == 'workflow_dispatch'
env:
BRANCHES: ${{ github.event.inputs.branches }}
WP_VERSION: ${{ github.event.inputs.wpversion }}
run: |
./bin/plugin/cli.js perf $(echo $BRANCHES | tr ',' ' ') --tests-branch $GITHUB_SHA --wp-version "$WP_VERSION"
./bin/plugin/cli.js perf $(echo $BRANCHES | tr ',' ' ') --wp-version "$WP_VERSION"

- name: Archive performance results
if: success()
Expand Down
Loading