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

Make the performance tests more stable #48094

Merged
merged 5 commits into from
Feb 21, 2023
Merged

Make the performance tests more stable #48094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b31251d
Make the performance tests more stable
youknowriad Feb 15, 2023
eb64e70
Try a different fix
youknowriad Feb 16, 2023
468711e
Fix typo
WunderBart Feb 16, 2023
c4c5251
Remove path
youknowriad Feb 21, 2023
67998fb
Fix performance tests
youknowriad Feb 21, 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
12 changes: 6 additions & 6 deletions bin/plugin/commands/performance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,14 +323,14 @@ async function runPerformanceTests( branches, options ) {
log( ` >> Fetching the ${ fancyBranch } branch` );
// @ts-ignore
await SimpleGit( buildPath ).reset( 'hard' ).checkout( branch );

log( ` >> Building the ${ fancyBranch } branch` );
await runShellScript(
'npm ci && npm run prebuild:packages && node ./bin/packages/build.js && npx wp-scripts build',
buildPath
);
}

log( ` >> Building the ${ fancyBranch } branch` );
await runShellScript(
'npm ci && npm run prebuild:packages && node ./bin/packages/build.js && npx wp-scripts build',
buildPath
);

Copy link
Contributor Author

@youknowriad youknowriad Feb 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In #45737 we've introduced a potential optimization to the performance job, the problem is that it resulted in the job comparing the wrong thing.

Basically if a branch was equivalent to the "test branch", we reused the same code to gain some time and avoid the git clone, the issue is that we didn't build the code in that case, meaning we were using the WP trunk version of Gutenberg and not the actual branch.

(Currently WP trunk's version does have the '/navigation/single' support in the site editor which explains why this random change fixed the perf job.

The bummer is that you can say that for the last three months, the perf jobs in PRs were useless.

Copy link
Member

Choose a reason for hiding this comment

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

That also explains why I was completely unable to repro locally. 💥
Nice catch, though the bug is indeed a nasty one! 🐛

Copy link
Member

Choose a reason for hiding this comment

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

/cc @kevin940726 @t-hamano

await runShellScript(
'cp ' +
path.resolve(
Expand Down
18 changes: 10 additions & 8 deletions packages/e2e-test-utils/src/site-editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,13 +166,15 @@ export async function openPreviousGlobalStylesPanel() {
* Enters edit mode.
*/
export async function enterEditMode() {
const isViewMode = await page.$(
'.edit-site-visual-editor__editor-canvas[role="button"]'
);
// This check is necessary for the performance tests in old branches
// where the site editor toggle was not implemented yet.
if ( ! isViewMode ) {
return;
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change is not necessary but I think it's a bit safer than the previous code, so I decided to leave it.

await page.waitForSelector(
'.edit-site-visual-editor__editor-canvas[role="button"]',
{ timeout: 3000 }
);

await canvas().click( 'body' );
} catch {
// This catch is necessary for the performance tests in old branches
// where the site editor toggle was not implemented yet.
}
await canvas().click( 'body' );
}
4 changes: 0 additions & 4 deletions packages/e2e-tests/specs/performance/site-editor.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,6 @@ describe( 'Site Editor Performance', () => {
await visitSiteEditor( {
postId: id,
postType: 'page',
// This shouldn't be necessary, but without it the tests fail.
// Could be related to having the necessary cookies in the browser.
// See https://github.com/WordPress/gutenberg/pull/48240/files#r1111760556
path: '/navigation/single',
} );
} );

Expand Down