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

Adds a command to run performance tests across branches #22418

Merged
merged 4 commits into from
May 19, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions bin/plugin/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const program = require( 'commander' );
const { releaseRC, releaseStable } = require( './commands/release' );
const { prepublishNpmStablePackages } = require( './commands/packages' );
const { getReleaseChangelog } = require( './commands/changelog' );
const { runPerformanceTests } = require( './commands/performance' );

program
.command( 'release-plugin-rc' )
Expand Down Expand Up @@ -42,4 +43,12 @@ program
.description( 'Generates a changelog from merged Pull Requests' )
.action( getReleaseChangelog );

program
.command( 'performance-tests [branches...]' )
.alias( 'perf' )
.description(
'Runs performance tests on two separate branches and outputs the result'
)
.action( runPerformanceTests );

program.parse( process.argv );
229 changes: 229 additions & 0 deletions bin/plugin/commands/performance.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,229 @@
/**
* External dependencies
*/
const path = require( 'path' );

/**
* Internal dependencies
*/
const { formats, log } = require( '../lib/logger' );
const {
runShellScript,
readJSONFile,
askForConfirmation,
getRandomTemporaryPath,
} = require( '../lib/utils' );
const git = require( '../lib/git' );
const config = require( '../config' );

/**
* @typedef WPRawPerformanceResults
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth I hope you'll appreciate all these efforts :)

*
* @property {number[]} load Load Time.
* @property {number[]} domcontentloaded DOM Contentloaded time.
* @property {number[]} type Average type time.
* @property {number[]} focus Average block selection time.
*/

/**
* @typedef WPPerformanceResults
*
* @property {number} load Load Time.
* @property {number} domcontentloaded DOM Contentloaded time.
* @property {number} type Average type time.
* @property {number} minType Minium type time.
* @property {number} maxType Maximum type time.
* @property {number} focus Average block selection time.
* @property {number} minFocus Min block selection time.
* @property {number} maxFocus Max block selection time.
*/
/**
* @typedef WPFormattedPerformanceResults
*
* @property {string} load Load Time.
* @property {string} domcontentloaded DOM Contentloaded time.
* @property {string} type Average type time.
* @property {string} minType Minium type time.
* @property {string} maxType Maximum type time.
* @property {string} focus Average block selection time.
* @property {string} minFocus Min block selection time.
* @property {string} maxFocus Max block selection time.
*/

/**
* Computes the average number from an array numbers.
*
* @param {number[]} array
*
* @return {number} Average.
*/
function average( array ) {
return array.reduce( ( a, b ) => a + b ) / array.length;
}

/**
* Computes the median number from an array numbers.
*
* @param {number[]} array
*
* @return {number} Median.
*/
function median( array ) {
const mid = Math.floor( array.length / 2 ),
numbers = [ ...array ].sort( ( a, b ) => a - b );
return array.length % 2 !== 0
? numbers[ mid ]
: ( numbers[ mid - 1 ] + numbers[ mid ] ) / 2;
}

/**
* Rounds and format a time passed in milliseconds.
*
* @param {number} number
*
* @return {string} Formatted time.
*/
function formatTime( number ) {
const factor = Math.pow( 10, 2 );
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing there's a good reason we're calling Math.pow( 10, 2 ) instead of simply using 100 but I can't figure out what it might be. Any hints @youknowriad?

Copy link
Member

Choose a reason for hiding this comment

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

also I see this is called formatTime but it appears to be applied to a number of different data types and it appears to be used as a way of converting to a fixed number of decimal places.

in trunk the ' ms' suffix is gone, moved to where we store this data in a JSON file. I think it's currently the equivalent of this, but I'm confused because of the disparity between what it's called and what it does.

mapValues( medians, n => Math.round( 100 * n ) / 100 )

Though to be honest if this is all we're doing then would you find it wrong to leave the numbers as they are and then run .toFixed(2) at the place where we print them? I think that could make the intention more clear (if our goal is simply to limit to a fixed number of decimal places when we print these out) and leave the data more raw along the way.

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 intention more clear (if our goal is simply to limit to a fixed number of decimal places when we print these out) and leave the data more raw along the way.

I think that's probably the intent here yeah.

return Math.round( number * factor ) / factor + ' ms';
}

/**
* Curate the raw performance results.
*
* @param {WPRawPerformanceResults} results
*
* @return {WPPerformanceResults} Curated Performance results.
*/
function curateResults( results ) {
return {
load: average( results.load ),
domcontentloaded: average( results.domcontentloaded ),
type: average( results.type ),
minType: Math.min( ...results.type ),
maxType: Math.max( ...results.type ),
focus: average( results.focus ),
minFocus: Math.min( ...results.focus ),
maxFocus: Math.max( ...results.focus ),
};
}

/**
* Runs the performance tests on a given branch.
*
* @param {string} performanceTestDirectory Path to the performance tests' clone.
* @param {string} environmentDirectory Path to the plugin environment's clone.
* @param {string} branch Branch name.
*
* @return {Promise<WPFormattedPerformanceResults>} Performance results for the branch.
*/
async function getPerformanceResultsForBranch(
performanceTestDirectory,
environmentDirectory,
branch
) {
log( '>> Fetching the ' + formats.success( branch ) + ' branch' );
await git.checkoutRemoteBranch( environmentDirectory, branch );

log( '>> Building the ' + formats.success( branch ) + ' branch' );
await runShellScript(
'npm install && npm run build',
environmentDirectory
);

log(
'>> Running the test on the ' + formats.success( branch ) + ' branch'
);
const results = [];
for ( let i = 0; i < 3; i++ ) {
Copy link
Member

Choose a reason for hiding this comment

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

Any significance to three iterations? Or is it just an arbitrary number to try toward some confidence of trusting the output? I guess it depends for what reasons we would expect a "bad" output, but it also occurs to me that there's no randomness or mixing of runs between branches (i.e. A A A B B B C C C instead of A B C A B C A B C or A C B B A C C B A).

Might also be something to extract as a named / documented constant, to clarify all of the above points to the next maintainer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or is it just an arbitrary number to try toward some confidence of trusting the output?

This is the reason, I think cache could create wrong numbers that's why I trust multiple runs better.

For the order, I agree that it's better to switch but the problem is that it will make the command a lot slower unless we have separate wp-env environments entirely. I might consider it separately.

await runShellScript(
'npm run test-performance',
performanceTestDirectory
);
const rawResults = await readJSONFile(
path.join(
performanceTestDirectory,
'packages/e2e-tests/specs/performance/results.json'
)
);
results.push( curateResults( rawResults ) );
}

return {
load: formatTime( median( results.map( ( r ) => r.load ) ) ),
domcontentloaded: formatTime(
median( results.map( ( r ) => r.domcontentloaded ) )
),
type: formatTime( median( results.map( ( r ) => r.type ) ) ),
minType: formatTime( median( results.map( ( r ) => r.minType ) ) ),
maxType: formatTime( median( results.map( ( r ) => r.maxType ) ) ),
focus: formatTime( median( results.map( ( r ) => r.focus ) ) ),
minFocus: formatTime( median( results.map( ( r ) => r.minFocus ) ) ),
maxFocus: formatTime( median( results.map( ( r ) => r.maxFocus ) ) ),
};
}

/**
* Runs the performances tests on an array of branches and output the result.
*
* @param {string[]} branches Branches to compare
*/
async function runPerformanceTests( branches ) {
// The default value doesn't work because commander provides an array.
if ( branches.length === 0 ) {
branches = [ 'master' ];
}

log(
formats.title( '\n💃 Performance Tests 🕺\n\n' ),
'Welcome! This tool runs the performance tests on multiple branches and displays a comparison table.\n' +
'In order to run the tests, the tool is going to load a WordPress environment on 8888 and 8889 ports.\n' +
'Make sure these ports are not used before continuing.\n'
);

await askForConfirmation( 'Ready to go? ' );

log( '>> Cloning the repository' );
const performanceTestDirectory = await git.clone( config.gitRepositoryURL );
const environmentDirectory = getRandomTemporaryPath();
log(
'>> Perf Tests Directory : ' +
formats.success( performanceTestDirectory )
);
log(
'>> Environment Directory : ' + formats.success( environmentDirectory )
);

log( '>> Installing dependencies' );
// The build packages is necessary for the performance folder
await runShellScript(
'npm install && npm run build:packages',
Copy link
Member

@aduth aduth May 18, 2020

Choose a reason for hiding this comment

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

Note: Depending if you're trying to be as efficient as possible, note that build:packages also runs TypeScript. Running ./bin/packages/build.js would be the most direct (example).

Related previous discussion about incorporating TypeScript in build at #18942 (comment).

performanceTestDirectory
);
await runShellScript(
'cp -R ' + performanceTestDirectory + ' ' + environmentDirectory
);

log( '>> Starting the WordPress environment' );
await runShellScript( 'npm run wp-env start' );

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

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

log( '\n>> 🎉 Results.\n' );
console.table( results );
}

module.exports = {
runPerformanceTests,
};
18 changes: 18 additions & 0 deletions bin/plugin/config.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,23 @@
const gitRepoOwner = 'WordPress';

/**
* @typedef WPPluginCLIConfig
*
* @property {string} slug Slug.
* @property {string} name Name.
* @property {string} team Github Team Name.
* @property {string} githubRepositoryOwner Github Repository Owner.
* @property {string} githubRepositoryName Github Repository Name.
* @property {string} pluginEntryPoint Plugin Entry Point File.
* @property {string} buildZipCommand Build Plugin ZIP command.
* @property {string} wpRepositoryReleasesURL WordPress Repository Tags URL.
* @property {string} gitRepositoryURL Git Repository URL.
* @property {string} svnRepositoryURL SVN Repository URL.
*/

/**
* @type {WPPluginCLIConfig}
*/
const config = {
slug: 'gutenberg',
name: 'Gutenberg',
Expand Down
Loading