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.

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

Perf testing: Cover Site Editor loading time #23842

Merged
merged 15 commits into from
Jul 16, 2020
33 changes: 23 additions & 10 deletions bin/plugin/commands/performance.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,15 @@ function curateResults( results ) {
*
* @param {string} performanceTestDirectory Path to the performance tests' clone.
* @param {string} environmentDirectory Path to the plugin environment's clone.
* @param {string} testSuite Name of the tests set.
* @param {string} branch Branch name.
*
* @return {Promise<WPFormattedPerformanceResults>} Performance results for the branch.
*/
async function getPerformanceResultsForBranch(
performanceTestDirectory,
environmentDirectory,
testSuite,
branch
) {
// Restore clean working directory (e.g. if `package-lock.json` has local
Expand All @@ -147,13 +149,13 @@ async function getPerformanceResultsForBranch(
const results = [];
for ( let i = 0; i < 3; i++ ) {
await runShellScript(
'npm run test-performance',
`npm run test-performance -- packages/e2e-tests/specs/performance/${ testSuite }.performance.test.js`,
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
performanceTestDirectory
);
const rawResults = await readJSONFile(
path.join(
performanceTestDirectory,
'packages/e2e-tests/specs/performance/results.json'
`packages/e2e-tests/specs/performance/${ testSuite }.performance.test.results.json`
)
);
results.push( curateResults( rawResults ) );
Expand Down Expand Up @@ -220,21 +222,32 @@ async function runPerformanceTests( branches, options ) {
log( '>> Starting the WordPress environment' );
await runShellScript( 'npm run wp-env start', environmentDirectory );

/** @type {Record<string, WPFormattedPerformanceResults>} */
const testSuites = [ 'post-editor', 'site-editor' ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an argument instead of running all tests with default to post-editor? or do you think we should be running all tests everytime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in the function body of runPerformanceTests, which is the top-level export of this file (invoked by bin/plugin/cli.js). If we want to make this an argument, we'd basically have to make it one that's passed by the CLI.

I'm leaning towards always running both perf suites. Their main use case is CI (the GH Action), as this is the most reliable guard against perf regressions, so it should IMO be easy to monitor that. Furthermore, the Site Editor test only covers loading time, which is currently pretty short. We can revisit if we add more coverage and testing times grow too much.


/** @type {Record<string,Record<string, WPFormattedPerformanceResults>>} */
const results = {};
for ( const branch of branches ) {
results[ branch ] = await getPerformanceResultsForBranch(
performanceTestDirectory,
environmentDirectory,
branch
);
for ( const testSuite of testSuites ) {
results[ testSuite ] = {};
for ( const branch of branches ) {
results[ testSuite ][
branch
] = await getPerformanceResultsForBranch(
performanceTestDirectory,
environmentDirectory,
testSuite,
branch
);
}
}

log( '>> Stopping the WordPress environment' );
await runShellScript( 'npm run wp-env stop', environmentDirectory );

log( '\n>> 🎉 Results.\n' );
console.table( results );
for ( const testSuite of testSuites ) {
log( `\n>> ${ testSuite }\n` );
console.table( results[ testSuite ] );
}
}

module.exports = {
Expand Down
11 changes: 7 additions & 4 deletions packages/e2e-tests/config/performance-reporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
const { readFileSync, existsSync } = require( 'fs' );
const path = require( 'path' );
const chalk = require( 'chalk' );

function average( array ) {
Expand All @@ -17,14 +18,16 @@ const title = chalk.bold;
const success = chalk.bold.green;

class PerformanceReporter {
onRunComplete() {
const path = __dirname + '/../specs/performance/results.json';
onTestResult( test ) {
const dirname = path.dirname( test.path );
const basename = path.basename( test.path, '.js' );
const filepath = path.join( dirname, basename + '.results.json' );

if ( ! existsSync( path ) ) {
if ( ! existsSync( filepath ) ) {
return;
}

const results = readFileSync( path, 'utf8' );
const results = readFileSync( filepath, 'utf8' );
const { load, domcontentloaded, type, focus } = JSON.parse( results );

if ( load && load.length ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ function getSelectionEventDurations( trace ) {

jest.setTimeout( 1000000 );

describe( 'Performance', () => {
describe( 'Post Editor Performance', () => {
it( 'Loading, typing and selecting blocks', async () => {
const results = {
load: [],
Expand Down Expand Up @@ -183,7 +183,7 @@ describe( 'Performance', () => {
results.focus = focusEvents;

writeFileSync(
__dirname + '/results.json',
__dirname + '/post-editor.performance.test.results.json',
JSON.stringify( results, null, 2 )
);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/**
* External dependencies
*/
import { writeFileSync } from 'fs';

/**
* Internal dependencies
*/
import { useExperimentalFeatures } from '../../experimental-features';

/**
* WordPress dependencies
*/
import { trashAllPosts, visitAdminPage } from '@wordpress/e2e-test-utils';
import { addQueryArgs } from '@wordpress/url';

jest.setTimeout( 1000000 );

describe( 'Site Editor Performance', () => {
useExperimentalFeatures( [
'#gutenberg-full-site-editing',
'#gutenberg-full-site-editing-demo',
] );

beforeAll( async () => {
await trashAllPosts( 'wp_template' );
await trashAllPosts( 'wp_template_part' );
} );
afterAll( async () => {
await trashAllPosts( 'wp_template' );
await trashAllPosts( 'wp_template_part' );
} );

it( 'Loading', async () => {
const results = {
load: [],
domcontentloaded: [],
type: [],
focus: [],
};

visitAdminPage(
'admin.php',
addQueryArgs( '', {
page: 'gutenberg-edit-site',
} ).slice( 1 )
);

let i = 1;

// Measuring loading time
while ( i-- ) {
await page.reload( { waitUntil: [ 'domcontentloaded', 'load' ] } );
Copy link
Member

Choose a reason for hiding this comment

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

Should we wait until the template has loaded on the page? I think this just measures document load, but there is another delay of a few seconds until the template is loaded from the API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. What would be the best way to implement this? Add a waitForSelector for a thing that's only in the template? 🤔

const timings = JSON.parse(
await page.evaluate( () =>
JSON.stringify( window.performance.timing )
)
);
const {
navigationStart,
domContentLoadedEventEnd,
loadEventEnd,
} = timings;
results.load.push( loadEventEnd - navigationStart );
results.domcontentloaded.push(
domContentLoadedEventEnd - navigationStart
);
}

writeFileSync(
__dirname + '/site-editor.performance.test.results.json',
JSON.stringify( results, null, 2 )
);

expect( true ).toBe( true );
ockham marked this conversation as resolved.
Show resolved Hide resolved
} );
} );