-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
* | ||
* @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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing there's a good reason we're calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also I see this is called in 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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++ ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Might also be something to extract as a named / documented constant, to clarify all of the above points to the next maintainer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Related previous discussion about incorporating TypeScript in |
||
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, | ||
}; |
There was a problem hiding this comment.
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 :)