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

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 18 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
6 changes: 5 additions & 1 deletion packages/block-library/src/post-content/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,12 @@ function EditableContent( { context = {} } ) {

function Content( props ) {
const { context: { queryId, postType, postId } = {} } = props;
const isDescendentOfQueryLoop = Number.isFinite( queryId );
const userCanEdit = useCanEditEntity( 'postType', postType, postId );
if ( userCanEdit === undefined ) {
return null;
}

const isDescendentOfQueryLoop = Number.isFinite( queryId );
const isEditable = userCanEdit && ! isDescendentOfQueryLoop;

return isEditable ? (
Expand Down
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
177 changes: 92 additions & 85 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 @@ -30,6 +30,9 @@ import {

jest.setTimeout( 1000000 );

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

const results = {
serverResponse: [],
firstPaint: [],
Expand All @@ -46,7 +49,7 @@ const results = {
listViewOpen: [],
};

let id;
let postId;

describe( 'Site Editor Performance', () => {
beforeAll( async () => {
Expand All @@ -59,6 +62,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,112 +79,115 @@ 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.
describe( 'Loading', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have sub describes and iterations within the same "test"? I find this a bit confusing personally. What benefits do we have from this change?

Copy link
Member Author

@WunderBart WunderBart Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't have a strong opinion here, but it sometimes helps separate the concerns better. Like with the Loading block, it's easier to set up the it.each iterator below if it's wrapped with the describe block. Or in case of e.g. multiple typing test cases (like with the post editor perf specs - c1, c2), it would be more readable to group them with the decribe( 'Typing' ) block. Having said that, I'm open to flattening it back with a single, top-level describe.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion either, but feel like whatever we do, it should be the same for all of our tests and there's already other suites using a single parent describe (post editor, classic theme, block theme)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point; consistency is king. I'll switch it to a single parent describe.

Copy link
Member Author

Choose a reason for hiding this comment

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

3da31c0

// Number of 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.
// Having at least one helps ensure that caching quirks don't manifest
// in the results.
const throwaway = 1;
const iterations = sequence( 1, samples + throwaway );

it.each( iterations )(
`trace large post loading durations (%i of ${ iterations.length })`,
async ( i ) => {
// Open the test page in Site Editor.
await visitSiteEditor( {
postId,
postType: 'page',
} );

// Wait for the first content block to render.
await canvas().waitForSelector(
'[data-type="core/post-content"] .wp-block'
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
);

if ( i > throwaway ) {
const {
serverResponse,
firstPaint,
domContentLoaded,
loaded,
firstContentfulPaint,
firstBlock,
} = await getLoadingDurations();

results.serverResponse.push( serverResponse );
results.firstPaint.push( firstPaint );
results.domContentLoaded.push( domContentLoaded );
results.loaded.push( loaded );
results.firstContentfulPaint.push( firstContentfulPaint );
results.firstBlock.push( firstBlock );
}

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,
} );
await canvas().waitForSelector( '.wp-block', { timeout: 120000 } );
WunderBart marked this conversation as resolved.
Show resolved Hide resolved

if ( i < samples ) {
const {
serverResponse,
firstPaint,
domContentLoaded,
loaded,
firstContentfulPaint,
firstBlock,
} = await getLoadingDurations();

results.serverResponse.push( serverResponse );
results.firstPaint.push( firstPaint );
results.domContentLoaded.push( domContentLoaded );
results.loaded.push( loaded );
results.firstContentfulPaint.push( firstContentfulPaint );
results.firstBlock.push( firstBlock );
expect( true ).toBe( true );
}
}
);
} );

it( 'Typing', async () => {
await page.waitForSelector( '.edit-site-visual-editor', {
timeout: 120000,
} );
await canvas().waitForSelector( '.wp-block', { timeout: 120000 } );
describe( 'Typing', () => {
it( 'trace 200 characters typing sequence inside a large post', async () => {
// Open the test page in Site Editor.
await visitSiteEditor( {
postId,
postType: 'page',
} );

// Measuring typing performance inside the post content.
await canvas().waitForSelector(
'[data-type="core/post-content"] [data-type="core/paragraph"]'
);
await enterEditMode();
await canvas().focus(
'[data-type="core/post-content"] [data-type="core/paragraph"]'
);
await insertBlock( 'Paragraph' );
let i = 200;
const traceFile = __dirname + '/trace.json';
await page.tracing.start( {
path: traceFile,
screenshots: false,
categories: [ 'devtools.timeline' ],
} );
while ( i-- ) {
await page.keyboard.type( 'x' );
}
await page.tracing.stop();
const traceResults = JSON.parse( readFile( traceFile ) );
const [ keyDownEvents, keyPressEvents, keyUpEvents ] =
getTypingEventDurations( traceResults );

for ( let j = 0; j < keyDownEvents.length; j++ ) {
results.type.push(
keyDownEvents[ j ] + keyPressEvents[ j ] + keyUpEvents[ j ]
// Wait for the first paragraph to be ready.
const firstParagraph = await canvas().waitForXPath(
'//p[contains(text(), "Lorem ipsum dolor sit amet")]'
);
}

const resultsFilename = basename( __filename, '.js' ) + '.results.json';
// Get inside the post content.
await enterEditMode();

writeFileSync(
join( __dirname, resultsFilename ),
JSON.stringify( results, null, 2 )
);
// Insert a new paragraph right under the first one.
await firstParagraph.focus();
await insertBlock( 'Paragraph' );

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

// 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 i = 0; i < keyDownEvents.length; i++ ) {
results.type.push(
keyDownEvents[ i ] + keyPressEvents[ i ] + keyUpEvents[ i ]
);
}

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

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