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

Improve Site Editor performance tests #48138

Merged
merged 23 commits into from
Mar 7, 2023
Merged

Improve Site Editor performance tests #48138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
184bf0d
Fix site editor performance tests
WunderBart Feb 16, 2023
01bc9e2
Restore some comments
WunderBart Feb 16, 2023
3dbc1fe
Try uploading artifacts to see why CI is failing
WunderBart Feb 17, 2023
1a1298c
try setting failure breakpoints to get screenshots
WunderBart Feb 17, 2023
835f3c6
Try checking if the test post is being saved correctly
WunderBart Feb 17, 2023
f4a1a9f
Clear local storage in pageSetup
WunderBart Feb 17, 2023
f2f52a1
Try wait longer for the SE
WunderBart Feb 17, 2023
0ab0c95
Clean up a few bits
WunderBart Feb 18, 2023
dee6e71
Add missing path param
WunderBart Feb 20, 2023
fbe4439
Remove temp code
WunderBart Feb 20, 2023
f84146c
Fix LS clearing
WunderBart Feb 21, 2023
edd93b6
Add more tweaks
WunderBart Feb 21, 2023
4a549fc
Revert "Try uploading artifacts to see why CI is failing"
WunderBart Feb 21, 2023
91bdc33
Improve wording
WunderBart Feb 21, 2023
3781e85
Remove obsolete path param
WunderBart Feb 21, 2023
10d7c04
Add `sequence` util
WunderBart Feb 28, 2023
8486b48
Hold the PostContent block render until userCanEdit status loaded
WunderBart Mar 1, 2023
6dcc933
Don't use hacky selectors
WunderBart Mar 1, 2023
a5899a4
Merge remote-tracking branch 'origin' into fix/site-editor-performanc…
WunderBart Mar 2, 2023
cc09c6f
Move `sequence()` to `utils.js`
WunderBart Mar 2, 2023
5623f43
Wait for any block for the `firstBlock` load time
WunderBart Mar 3, 2023
a0f60cb
Merge remote-tracking branch 'origin' into fix/site-editor-performanc…
WunderBart Mar 3, 2023
3da31c0
Use single parent `describe` for consistency
WunderBart Mar 6, 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 packages/e2e-tests/config/setup-performance-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,24 +26,24 @@ async function setupPage() {
] );
}

// Before every test suite run, delete all content created by the test. This ensures
// other posts/comments/etc. aren't dirtying tests and tests don't depend on
// each other's side-effects.
// Before every test suite run, delete all content created by the test. This
// ensures other posts/comments/etc. aren't dirtying tests and tests don't
// depend on each other's side-effects.
beforeAll( async () => {
enablePageDialogAccept();

await trashAllPosts();
await trashAllPosts( 'wp_block' );
await activatePlugin( 'gutenberg-test-plugin-disables-the-css-animations' );
await clearLocalStorage();
await setupPage();
await activatePlugin( 'gutenberg-test-plugin-disables-the-css-animations' );
} );

afterEach( async () => {
// Clear localStorage between tests so that the next test starts clean.
await clearLocalStorage();
// Close the previous page entirely and create a new page, so that the next test
// isn't affected by page unload work.
// Close the previous page entirely and create a new page, so that the next
// test isn't affected by page unload work.
await page.close();
page = await browser.newPage();
// Set up testing config on new page.
Expand Down
112 changes: 57 additions & 55 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 @@ -26,6 +26,7 @@ import {
deleteFile,
getTypingEventDurations,
getLoadingDurations,
sequence,
} from './utils';

jest.setTimeout( 1000000 );
Expand All @@ -46,7 +47,7 @@ const results = {
listViewOpen: [],
};

let id;
let postId;

describe( 'Site Editor Performance', () => {
beforeAll( async () => {
Expand All @@ -59,6 +60,7 @@ describe( 'Site Editor Performance', () => {
);

await createNewPost( { postType: 'page' } );

await page.evaluate( ( _html ) => {
const { parse } = window.wp.blocks;
const { dispatch } = window.wp.data;
Expand All @@ -75,48 +77,47 @@ describe( 'Site Editor Performance', () => {
}, html );
await saveDraft();

id = await page.evaluate( () =>
postId = await page.evaluate( () =>
new URL( document.location ).searchParams.get( 'post' )
);
} );

afterAll( async () => {
const resultsFilename = basename( __filename, '.js' ) + '.results.json';
writeFileSync(
join( __dirname, resultsFilename ),
JSON.stringify( results, null, 2 )
);
Comment on lines +86 to +90
Copy link
Member Author

Choose a reason for hiding this comment

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

The results object contains data for both Loading and Typing tests, so let's write the file in the afterAll hook instead of the end of the Typing test.

Copy link
Member

Choose a reason for hiding this comment

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

is storing this file alongside the sourcecode the optimal choice here? I know I was confused when first storing results files as artifacts because of the directory structure. would we want to move these instead to our relatively-new __test-results directory?

we could consider some kind of TEST_RESULTS_BASE_DIR ENV to hold the base path if we wanted to avoid doing path-math everywhere: running in CI vs. locally in Docker vs. locally without Docker…

Copy link
Member Author

@WunderBart WunderBart Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed, we should be able to pass the __test-results path as an env variable and save directly there. Will give it a try.

edit:

OTOH, let's leave that for a follow-up PR. It touches all the other perf tests and I wanted to make this PR about the Site Editor ones only.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've drafted the refactor in #48684, but I'm not sure how to handle the "locally without docker" situation. I think it still makes the most sense (as it's most convenient) to store alongside the sourcecode when running e.g. via npm run test:performance -- site-editor.

Let me know if that makes sense to you.

Copy link
Member

Choose a reason for hiding this comment

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

the main thing about colocating with sourcecode is that it leaves its mess distributed around the project. at least when creating a project-root-level __test-results directory it's easy to wipe that out in one command.

see for example challenges with the distributed build, build-styles, build-modules, and build-types directories in each package directory.

I'll add further remarks in the linked PR.

thanks! ✋


await deleteAllTemplates( 'wp_template' );
await deleteAllTemplates( 'wp_template_part' );
await activateTheme( 'twentytwentyone' );
} );

beforeEach( async () => {
await visitSiteEditor( {
postId: id,
postType: 'page',
} );
} );

it( 'Loading', async () => {
const editorURL = await page.url();

// Number of sample measurements to take.
const samples = 3;
// Number of throwaway measurements to perform before recording samples.
// Having at least one helps ensure that caching quirks don't manifest in
// the results.
const throwaway = 1;

let i = throwaway + samples;

// Measuring loading time.
while ( i-- ) {
await page.close();
page = await browser.newPage();

await page.goto( editorURL );
await page.waitForSelector( '.edit-site-visual-editor', {
timeout: 120000,
// Number of loading measurements to take.
const loadingSamples = 3;
// Number of throwaway measurements to perform before recording samples.
// Having at least one helps ensure that caching quirks don't manifest
// in the results.
const loadingSamplesThrowaway = 1;
const loadingIterations = sequence(
1,
loadingSamples + loadingSamplesThrowaway
);
it.each( loadingIterations )(
`Loading (%i of ${ loadingIterations.length })`,
async ( i ) => {
// Open the test page in Site Editor.
await visitSiteEditor( {
postId,
postType: 'page',
} );
await canvas().waitForSelector( '.wp-block', { timeout: 120000 } );
WunderBart marked this conversation as resolved.
Show resolved Hide resolved

if ( i < samples ) {
// Wait for the first block.
await canvas().waitForSelector( '.wp-block' );

// Save results.
if ( i > loadingSamplesThrowaway ) {
const {
serverResponse,
firstPaint,
Expand All @@ -133,52 +134,53 @@ describe( 'Site Editor Performance', () => {
results.firstContentfulPaint.push( firstContentfulPaint );
results.firstBlock.push( firstBlock );
}

expect( true ).toBe( true );
}
} );
);

it( 'Typing', async () => {
await page.waitForSelector( '.edit-site-visual-editor', {
timeout: 120000,
// Open the test page in Site Editor.
await visitSiteEditor( {
postId,
postType: 'page',
} );
await canvas().waitForSelector( '.wp-block', { timeout: 120000 } );

// Measuring typing performance inside the post content.
await canvas().waitForSelector(
'[data-type="core/post-content"] [data-type="core/paragraph"]'
// Wait for the first paragraph to be ready.
const firstParagraph = await canvas().waitForXPath(
'//p[contains(text(), "Lorem ipsum dolor sit amet")]'
);

// Get inside the post content.
await enterEditMode();
await canvas().focus(
'[data-type="core/post-content"] [data-type="core/paragraph"]'
);

// Insert a new paragraph right under the first one.
await firstParagraph.focus();
await insertBlock( 'Paragraph' );
let i = 200;

// Start tracing.
const traceFile = __dirname + '/trace.json';
await page.tracing.start( {
path: traceFile,
screenshots: false,
categories: [ 'devtools.timeline' ],
} );
while ( i-- ) {
await page.keyboard.type( 'x' );
}

// Type "x" 200 times.
await page.keyboard.type( new Array( 200 ).fill( 'x' ).join( '' ) );

// Stop tracing and save results.
await page.tracing.stop();
const traceResults = JSON.parse( readFile( traceFile ) );
const [ keyDownEvents, keyPressEvents, keyUpEvents ] =
getTypingEventDurations( traceResults );

for ( let j = 0; j < keyDownEvents.length; j++ ) {
for ( let i = 0; i < keyDownEvents.length; i++ ) {
results.type.push(
keyDownEvents[ j ] + keyPressEvents[ j ] + keyUpEvents[ j ]
keyDownEvents[ i ] + keyPressEvents[ i ] + keyUpEvents[ i ]
);
}

const resultsFilename = basename( __filename, '.js' ) + '.results.json';

writeFileSync(
join( __dirname, resultsFilename ),
JSON.stringify( results, null, 2 )
);

// Delete the original trace file.
deleteFile( traceFile );

expect( true ).toBe( true );
Expand Down
4 changes: 4 additions & 0 deletions packages/e2e-tests/specs/performance/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,7 @@ export async function getLoadingDurations() {
export function sum( arr ) {
return arr.reduce( ( a, b ) => a + b, 0 );
}

export function sequence( start, length ) {
return Array.from( { length }, ( _, i ) => i + start );
}