-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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: Make theme versions consistent cross-env #50905
Conversation
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
Flaky tests detected in 6980769. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5079288613
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could instead commit the zip binary and use that in wp-env
so that we don't have to generate this giant diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, turns out we can! 😄 I tried passing the zip URL into mapped folder and it worked:
{
"core": "WordPress/WordPress",
"plugins": [ "." ],
"themes": [ "./test/emptytheme" ],
"env": {
"tests": {
"mappings": {
"wp-content/plugins/gutenberg": ".",
"wp-content/mu-plugins": "./packages/e2e-tests/mu-plugins",
"wp-content/plugins/gutenberg-test-plugins": "./packages/e2e-tests/plugins",
"wp-content/themes/gutenberg-test-themes": "./test/gutenberg-test-themes",
"wp-content/themes/gutenberg-test-themes/twentytwentyone": "https://downloads.wordpress.org/theme/twentytwentyone.1.7.zip",
"wp-content/themes/gutenberg-test-themes/twentytwentythree": "https://downloads.wordpress.org/theme/twentytwentythree.1.0.zip"
}
}
}
}
It looks like it has to be a proper URL, though. Local path to a zip file does not work, so we'll need to rely on the availability of those versions. Having said that, I guess it's a good thing - if certain versions become unavailable then we probably should update as well. 😄
I'll make an update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in af5d1c9.
/cc @oandregal
It looks like we need to tell the linter job to skip the |
@@ -16,13 +16,12 @@ describe( 'Front End Performance', () => { | |||
}; | |||
|
|||
beforeAll( async () => { | |||
await activateTheme( 'twentytwentythree' ); | |||
await activateTheme( 'gutenberg-test-themes/twentytwentythree' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting here what I've observed, so it's documented somewhere.
This PR maps the specific versions in use within the gutenberg-test-themes
folder in the theme root (wp-content/themes
) of the testing WordPress environment. As a result, the environment has two instances of the TT1 and TT3 themes: one whose stylesheet is twentytwentyone
(latest version) and another with stylesheet gutenberg-test-themes/twentytwentyone
(the bundled version, here's at 1.7 for TT1).
You can confirm this by going to the "Appearance > Themes" page (inspect the "activate" button of each instance to know its stylesheet
):
Also by asking docker: docker exec -it `docker ps -f name=tests-wordpress_1 -q` ls -lat /var/www/html/wp-content/themes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noting this down! 🙇 Just wanted to add that we can check the available test env themes also via the following wp-cli command:
npx wp-env run tests-cli 'wp theme list'
For this branch, it should return the following:
+-----------------------------------------+----------+--------+---------+
| name | status | update | version |
+-----------------------------------------+----------+--------+---------+
| emptytheme | inactive | none | 1.0 |
| gutenberg-test-themes/emptyhybrid | inactive | none | 1.0 |
| gutenberg-test-themes/style-variations | inactive | none | 1.0 |
| gutenberg-test-themes/twentytwentyone | inactive | none | 1.7 |
| gutenberg-test-themes/twentytwentythree | active | none | 1.0 |
| twentyeleven | inactive | none | 4.3 |
| twentyfifteen | inactive | none | 3.4 |
| twentyfourteen | inactive | none | 3.6 |
| twentynineteen | inactive | none | 2.5 |
| twentyseventeen | inactive | none | 3.2 |
| twentysixteen | inactive | none | 2.9 |
| twentyten | inactive | none | 3.8 |
| twentythirteen | inactive | none | 3.8 |
| twentytwelve | inactive | none | 3.9 |
| twentytwenty | inactive | none | 2.2 |
| twentytwentyone | inactive | none | 1.8 |
| twentytwentythree | inactive | none | 1.1 |
| twentytwentytwo | inactive | none | 1.4 |
+-----------------------------------------+----------+--------+---------+
As a comparison, we can list the themes used in dev env, which should not return the gutenberg-test-themes/*
ones, for example:
> npx wp-env run cli 'wp theme list'
+-------------------+----------+--------+---------+
| name | status | update | version |
+-------------------+----------+--------+---------+
| emptytheme | inactive | none | 1.0 |
| twentyeleven | inactive | none | 4.3 |
| twentyfifteen | inactive | none | 3.4 |
| twentyfourteen | inactive | none | 3.6 |
| twentynineteen | inactive | none | 2.5 |
| twentyseventeen | inactive | none | 3.2 |
| twentysixteen | inactive | none | 2.9 |
| twentyten | inactive | none | 3.8 |
| twentythirteen | inactive | none | 3.8 |
| twentytwelve | inactive | none | 3.9 |
| twentytwenty | inactive | none | 2.2 |
| twentytwentyone | inactive | none | 1.8 |
| twentytwentythree | active | none | 1.1 |
| twentytwentytwo | inactive | none | 1.4 |
+-------------------+----------+--------+---------+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant. Thanks for the tip!
I gave this a test and this is what I've found. Note that I'm learning about the setup as I go. I appreciate your patience and also indications if there's an easier way to test this. I don't know how to verify the test uses the proper theme other than adding code to verify it. So I settle for verifying the
Longer step-by-step. Check the environment for tests (8889) uses the proper themes. It does.
Check the CI perf command uses the proper themes. I found it doesn't.
{
"core": "https://wordpress.org/wordpress-6.2.zip",
"plugins": [
"/tmp/afe13114-c37a-4f1e-bbe9-2a52f51af808/envs/debd225d007f4e441ceec80fbd6fa96653f94737/plugin"
],
"themes": [
"/tmp/afe13114-c37a-4f1e-bbe9-2a52f51af808/tests/test/emptytheme"
],
"env": {
"tests": {
"mappings": {
"wp-content/mu-plugins": "/tmp/afe13114-c37a-4f1e-bbe9-2a52f51af808/tests/packages/e2e-tests/mu-plugins",
"wp-content/plugins/gutenberg-test-plugins": "/tmp/afe13114-c37a-4f1e-bbe9-2a52f51af808/tests/packages/e2e-tests/plugins",
"wp-content/themes/gutenberg-test-themes": "/tmp/afe13114-c37a-4f1e-bbe9-2a52f51af808/tests/test/gutenberg-test-themes",
"wp-content/themes/gutenberg-test-themes/twentytwentyone": "https://downloads.wordpress.org/theme/twentytwentyone.1.7.zip",
"wp-content/themes/gutenberg-test-themes/twentytwentythree": "https://downloads.wordpress.org/theme/twentytwentythree.1.0.zip"
}
}
}
}
Plugins: note no plugins other than hello dolly (expected was gutenberg and e2e tests). Themes: note no emptytheme or two versions of twentytwentyone & twentytwentythree. Check using an external WordPress installation. I'm not sure how to do this.
|
Absolutely! It took a lot of time to familiarize myself with this setup, especially the performance.js runner part. I can see you had issues with verifying if the proper plugins and themes are used. I've verified they're being used correctly OnMyMachine™, and I think I know where the problem lies on your end. Let's go step by step so we're sure we're on the same page:
I hope this is helpful. Thanks for the thorough testing, @oandregal! 🙇 |
I'm not sure I understand this scenario, @oandregal. Could you provide more details here in the context of GB perf testing? |
@oandregal, any chance you could give this one another look? 🙏 |
@WunderBart Ah, thanks for sharing. Right, I missed that the runShellScript command changed the working directory, as you mentioned. This process works as expected then (updated the comment above). |
I can share what I know: some people do not use wp-env and run the e2e tests against a different instance. I haven't done it myself, though I presume they follow these instructions to provide the URL to run the tests against. This is why Riad opposed using wp-cli in the other PR: he wanted the only dependency to be a URL. So. With the setup we have in this PR, people not using
So. My question is: what if instead of having two instances of the same theme ( Alternatively, we'd need to document the use of @WunderBart does this make sense? |
The main issue I see with this is that It also looks like we're currently not supporting an alternative to What do you think? |
ok, let's go with this. I presume we could also switch the performance tests to use the latest version of the theme in a follow-up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach is working as expected. There's one piece of feedback we need to address https://github.com/WordPress/gutenberg/pull/50905/files#r1204138143
I'm pre-approving to expedite the review.
What?
Make performance testing consistent cross-env theme-wise.
This is an alternative approach to #50868 (See #50868 (comment))
Why?
For consistency sake, we've been using fixed versions of
twentytwentyone
andtwentytwentythree
themes in our performance tests. The way they are currently being activated, though, is applied only to theperformance.js
runner (which we use in CI). The same tests, when run directly vianpm run test:performance <suite-name>
, will not use the same themes because they're not available via the root wp-env instance – they will use the latest versions instead.How?
As with other test themes, we can upload the target versions to the
gutenberg-test-themes
folder and utilize them via test env folder mapping in the wp-env config.For more details see @oandregal's comment: #50905 (comment)
Testing Instructions
CI perf tests should pass,
The following should pass locally:
npm run test:performance front-end-block-theme.test.js
npm run test:performance front-end-classic-theme.test.js
npm run test:performance site-editor.test.js
npm run test:performance post-editor.test.js
The following suites should be using the following themes:
front-end-block-theme.test.js
→[email protected]
front-end-classic-theme.test.js
→[email protected]
site-editor.test.js
→emptytheme
post-editor.test.js
→emptytheme