diff --git a/.github/workflows/performance.yml b/.github/workflows/performance.yml index 057fc827d14de..af53db81138db 100644 --- a/.github/workflows/performance.yml +++ b/.github/workflows/performance.yml @@ -151,6 +151,8 @@ jobs: multisite: ${{ matrix.multisite }} BASE_TAG: ${{ needs.performance.outputs.BASE_TAG }} publish: ${{ contains( fromJson( needs.determine-matrix.outputs.subjects ), 'base' ) && ! matrix.memcached && ! matrix.multisite }} + fail-on-regression: ${{ github.event_name == 'pull_request' }} + regression-accepted: ${{ contains( github.event.pull_request.labels.*.name, 'Accepted Performance Regression' ) }} secrets: CODEVITALS_PROJECT_TOKEN: ${{ secrets.CODEVITALS_PROJECT_TOKEN }} diff --git a/.github/workflows/reusable-performance-report-v2.yml b/.github/workflows/reusable-performance-report-v2.yml index 56cc147491955..befae004f7838 100644 --- a/.github/workflows/reusable-performance-report-v2.yml +++ b/.github/workflows/reusable-performance-report-v2.yml @@ -27,6 +27,16 @@ on: required: false type: boolean default: false + fail-on-regression: + description: 'Whether a metric that exceeds an error budget fails the job.' + required: false + type: boolean + default: false + regression-accepted: + description: 'Whether the regressions of this change have been reviewed and accepted.' + required: false + type: boolean + default: false secrets: CODEVITALS_PROJECT_TOKEN: description: 'The authorization token for https://www.codevitals.run/project/wordpress.' @@ -79,12 +89,26 @@ jobs: - name: List files run: tree artifacts + - name: Check performance budgets + id: check-budgets + continue-on-error: true + env: + PERFORMANCE_REGRESSION_MODE: ${{ inputs.fail-on-regression && 'fail' || 'report' }} + PERFORMANCE_REGRESSION_ACCEPTED: ${{ inputs.regression-accepted }} + run: node ./tests/performance/check-regressions.js "$GITHUB_STEP_SUMMARY" + - name: Compare results run: node ./tests/performance/compare-results.js "${RUNNER_TEMP}/summary.md" - name: Add workflow summary run: cat "${RUNNER_TEMP}/summary.md" >> "$GITHUB_STEP_SUMMARY" + - name: Fail on performance regressions + if: ${{ inputs.fail-on-regression && steps.check-budgets.outcome == 'failure' }} + run: | + echo "::error::The performance budget check failed. See the job summary and the log of the budget check step. If the cost is accepted, add the 'Accepted Performance Regression' label and push a new commit; re-running this job does not pick up new labels." + exit 1 + - name: Set the base sha # Only needed when publishing results. if: ${{ inputs.publish }} diff --git a/tests/performance/check-regressions.js b/tests/performance/check-regressions.js new file mode 100644 index 0000000000000..76c1899367c8d --- /dev/null +++ b/tests/performance/check-regressions.js @@ -0,0 +1,322 @@ +#!/usr/bin/env node + +/* + * Compares the performance results of this change with the ones of its target + * and reports every metric that got worse by more than its budget allows. + * + * Usage: node check-regressions.js [summary-file] + * + * A screen that got slower fails the check only when the same screen also + * loads more scripts or stylesheets, runs more database queries or uses more + * memory, so a slowdown has a cause in the change and is not runner noise. + * Growth without a slowdown, and a slowdown without growth, are reported for + * reviewers but do not fail. + * + * Environment variables: + * - PERFORMANCE_REGRESSION_MODE: `fail` exits with code 1 when a regression with + * the `error` severity was found. Any other value only reports. Defaults to `report`. + * - PERFORMANCE_REGRESSION_ACCEPTED: `true` reports regressions without failing, + * for changes whose cost has been reviewed and accepted. + */ + +/** + * External dependencies. + */ +const { writeFileSync } = require( 'node:fs' ); + +/** + * Internal dependencies + */ +const { + median, + medianAbsoluteDeviation, + accumulateValues, + formatAsMarkdownTable, + formatValue, + parseFile, +} = require( './utils' ); + +const summaryFile = process.argv[ 2 ]; +const failOnError = process.env.PERFORMANCE_REGRESSION_MODE === 'fail'; +const regressionAccepted = + process.env.PERFORMANCE_REGRESSION_ACCEPTED === 'true'; + +/** + * Asset counts and sizes, database queries and memory are deterministic for a + * given build, so any growth beyond the budget is real. + */ +const deterministic = { minAbsolute: 2, minRelative: 0.05, kind: 'cost' }; + +/** + * Timings carry CI noise, so they must also grow by more than `noiseFactor` + * times the combined spread of both samples. + */ +const timing = { + minAbsolute: 10, + minRelative: 0.1, + noiseFactor: 3, + kind: 'timing', +}; + +/** + * A metric regresses when its median grows by more than `minAbsolute` and by + * more than `minRelative` of the target's median. + * + * @type {Record} + */ +const budgets = { + scriptCount: deterministic, + scriptBytes: { + ...deterministic, + minAbsolute: 20 * 1024, + minRelative: 0.02, + }, + stylesheetCount: deterministic, + stylesheetBytes: { + ...deterministic, + minAbsolute: 10 * 1024, + minRelative: 0.02, + }, + wpDbQueries: deterministic, + wpMemoryUsage: { ...deterministic, minAbsolute: 1024 * 1024 }, + domContentLoaded: timing, + largestContentfulPaint: timing, + lcpMinusTtfb: timing, + timeToFirstByte: { ...timing, minAbsolute: 5 }, + wpTotal: { ...timing, minAbsolute: 5 }, +}; + +/** + * Scales the median absolute deviation to estimate the standard deviation of + * normally distributed values without being skewed by outliers. + */ +const MAD_TO_STANDARD_DEVIATION = 1.4826; + +/** + * Returns the comparison of one metric against its budget. + * + * @param {string} metric Metric name. + * @param {number[]} before Values of the target. + * @param {number[]} after Values of this change. + * @return {{metric: string, before: number, after: number, delta: number, relative: number, regressed: boolean}} Comparison. + */ +function compareMetricToBudget( metric, before, after ) { + const budget = budgets[ metric ]; + const beforeMedian = median( before ); + const afterMedian = median( after ); + const delta = afterMedian - beforeMedian; + const relative = + beforeMedian > 0 ? delta / beforeMedian : Number.POSITIVE_INFINITY; + const noise = budget.noiseFactor + ? MAD_TO_STANDARD_DEVIATION * + Math.hypot( + medianAbsoluteDeviation( before ), + medianAbsoluteDeviation( after ) + ) + : 0; + + const regressed = + delta > budget.minAbsolute && + relative > budget.minRelative && + delta > ( budget.noiseFactor ?? 0 ) * noise; + + return { + metric, + before: beforeMedian, + after: afterMedian, + delta, + relative, + regressed, + }; +} + +/** + * Returns the regressions of every scenario measured on both sides. + * + * A timing regression gets the `error` severity when a cost metric of the same + * scenario regressed as well; every other regression is a `warning`. + * + * @param {Array<{title: string, results: Record[]}>} beforeStats Results of the target. + * @param {Array<{title: string, results: Record[]}>} afterStats Results of this change. + * @return {Array<{title: string, severity: 'error'|'warning'} & ReturnType>} Regressions. + */ +function findRegressions( beforeStats, afterStats ) { + return afterStats.flatMap( ( { title, results } ) => { + const beforeStat = beforeStats.find( ( stat ) => stat.title === title ); + if ( ! beforeStat ) { + return []; + } + + const afterValues = accumulateValues( results ); + const beforeValues = accumulateValues( beforeStat.results ); + + const regressions = Object.keys( budgets ) + .filter( + ( metric ) => + afterValues[ metric ]?.length && + beforeValues[ metric ]?.length + ) + .map( ( metric ) => + compareMetricToBudget( + metric, + beforeValues[ metric ], + afterValues[ metric ] + ) + ) + .filter( ( comparison ) => comparison.regressed ); + + const costGrew = regressions.some( + ( regression ) => budgets[ regression.metric ].kind === 'cost' + ); + + return regressions.map( ( regression ) => ( { + title, + ...regression, + severity: + costGrew && budgets[ regression.metric ].kind === 'timing' + ? 'error' + : 'warning', + } ) ); + } ); +} + +/** + * Formats how much a regressed metric grew, in its unit and in percent. + * + * @param {ReturnType} regression Regression. + * @return {string} Formatted growth, for example `+12 ms (+25.0 %)`. + */ +function formatGrowth( regression ) { + const absolute = `+${ formatValue( regression.metric, regression.delta ) }`; + + return Number.isFinite( regression.relative ) + ? `${ absolute } (+${ ( regression.relative * 100 ).toFixed( 1 ) } %)` + : absolute; +} + +/** + * Checks this change against one reference and reports its regressions. + * + * A missing reference is only reported when it is enforced, because the + * workflow measures the target commit on pull requests and the last release + * on trunk, never both. + * + * @param {Object} reference + * @param {string} reference.file Results file of the reference. + * @param {string} reference.heading Heading of the report section. + * @param {string} reference.missingNote Note shown when an enforced reference has no results. + * @param {boolean} reference.enforced Whether error regressions may fail the check. + * @param {Array<{title: string, results: Record[]}>} afterStats Results of this change. + * @return {{markdown: string, failed: boolean}} Report. + */ +function checkAgainstReference( reference, afterStats ) { + const referenceStats = parseFile( reference.file ); + let markdown = `### ${ reference.heading }\n\n`; + + if ( ! referenceStats.length || ! afterStats.length ) { + return { + markdown: reference.enforced + ? `${ markdown }${ reference.missingNote }\n\n` + : '', + failed: false, + }; + } + + const regressions = findRegressions( referenceStats, afterStats ); + const hasErrors = regressions.some( + ( regression ) => regression.severity === 'error' + ); + const failed = reference.enforced && hasErrors; + const failureAccepted = failed && regressionAccepted; + const errorRegressionsFailCheck = + reference.enforced && ! regressionAccepted; + + for ( const regression of regressions ) { + const level = + errorRegressionsFailCheck && regression.severity === 'error' + ? 'error' + : 'warning'; + console.log( + `::${ level } title=Performance regression (${ + reference.heading + })::${ regression.title }: ${ + regression.metric + } went from ${ formatValue( + regression.metric, + regression.before + ) } to ${ formatValue( + regression.metric, + regression.after + ) }, ${ formatGrowth( regression ) }.` + ); + } + + if ( ! regressions.length ) { + markdown += + ':white_check_mark: No metric got worse by more than its budget allows.\n\n'; + } else { + markdown += `:${ hasErrors && ! failureAccepted ? 'x' : 'warning' }: ${ + regressions.length + } ${ + regressions.length === 1 ? 'metric' : 'metrics' + } got worse by more than the budget allows.${ + failureAccepted + ? ' Accepted with the `Accepted Performance Regression` label, so the check does not fail.' + : '' + }\n\n`; + markdown += `${ formatAsMarkdownTable( + regressions.map( ( regression ) => ( { + Scenario: regression.title, + Metric: regression.metric, + Before: formatValue( regression.metric, regression.before ), + After: formatValue( regression.metric, regression.after ), + Change: formatGrowth( regression ), + Severity: + failureAccepted && regression.severity === 'error' + ? 'error (accepted)' + : regression.severity, + } ) ) + ) }\n`; + } + + return { markdown, failed: failed && ! failureAccepted }; +} + +const afterStats = parseFile( 'performance-results.json' ); + +const targetReport = checkAgainstReference( + { + file: 'before-performance-results.json', + heading: 'Compared with the target commit', + missingNote: + 'No build of the target commit was available, so this change could not be compared with it.', + enforced: failOnError, + }, + afterStats +); + +const releaseReport = checkAgainstReference( + { + file: 'base-performance-results.json', + heading: `Compared with the last release${ + process.env.BASE_TAG ? ` (${ process.env.BASE_TAG })` : '' + }`, + enforced: false, + }, + afterStats +); + +const summaryMarkdown = + '## Performance Budget Check\n\n' + + targetReport.markdown + + releaseReport.markdown + + 'A pull request fails when a screen gets slower and the same screen also loads more scripts or stylesheets, runs more database queries or uses more memory. ' + + 'Growth without a slowdown is reported for review, and a slowdown without growth is reported as a warning, because shared CI runners add noise.\n'; + +if ( summaryFile ) { + writeFileSync( summaryFile, summaryMarkdown ); +} + +console.log( summaryMarkdown ); + +process.exitCode = targetReport.failed ? 1 : 0; diff --git a/tests/performance/log-results.js b/tests/performance/log-results.js index a3ef4003fab6c..097310922952f 100644 --- a/tests/performance/log-results.js +++ b/tests/performance/log-results.js @@ -17,6 +17,8 @@ const { median, parseFile, accumulateValues } = require( './utils' ); const testSuiteMap = { 'Admin › Locale: en_US': 'admin', 'Admin › Locale: de_DE': 'admin-l10n', + 'Admin General Settings › Locale: en_US': 'admin-general-settings', + 'Admin General Settings › Locale: de_DE': 'admin-general-settings-l10n', 'Homepage › Theme: twentytwentyone, Locale: en_US': 'home-classic-theme', 'Homepage › Theme: twentytwentyone, Locale: de_DE': 'home-classic-theme-l10n', diff --git a/tests/performance/specs/admin.test.js b/tests/performance/specs/admin.test.js index 3414fdd3dbed9..28348e3d8aa13 100644 --- a/tests/performance/specs/admin.test.js +++ b/tests/performance/specs/admin.test.js @@ -8,66 +8,119 @@ import { test } from '@wordpress/e2e-test-utils-playwright'; */ import { camelCaseDashes, locales } from '../utils'; -const results = { - timeToFirstByte: [], -}; - -test.describe( 'Admin', () => { - for ( const locale of locales ) { - test.describe( `Locale: ${ locale }`, () => { - test.beforeAll( async ( { requestUtils } ) => { - await requestUtils.activateTheme( 'twentytwentyone' ); - await requestUtils.updateSiteSettings( { - language: 'en_US' === locale ? '' : locale, - } ); - } ); +const screens = [ + { + suiteTitle: 'Admin', + path: '/', + }, + { + suiteTitle: 'Admin General Settings', + path: 'options-general.php', + }, +]; - test.afterAll( async ( { requestUtils }, testInfo ) => { - await testInfo.attach( 'results', { - body: JSON.stringify( results, null, 2 ), - contentType: 'application/json', - } ); +/** + * Returns the number and size of the scripts and stylesheets in the page markup. + * + * Sizes are encoded body sizes, which do not depend on whether a response came from the HTTP cache. + * + * @param {import('@playwright/test').Page} page + * @return {Promise<{scriptCount: number, scriptBytes: number, stylesheetCount: number, stylesheetBytes: number}>} Asset metrics. + */ +async function getPageAssetMetrics( page ) { + return await page.evaluate( () => { + const resources = performance.getEntriesByType( 'resource' ); + const measure = ( selector, attribute ) => { + const urls = new Set( + Array.from( + document.querySelectorAll( selector ), + ( element ) => element[ attribute ] + ) + ); + const bytes = resources + .filter( ( resource ) => urls.has( resource.name ) ) + .reduce( ( total, resource ) => total + resource.encodedBodySize, 0 ); + return [ urls.size, bytes ]; + }; - await requestUtils.updateSiteSettings( { - language: '', - } ); + const [ scriptCount, scriptBytes ] = measure( 'script[src]', 'src' ); + const [ stylesheetCount, stylesheetBytes ] = measure( + 'link[rel="stylesheet"][href]', + 'href' + ); - results.timeToFirstByte = []; - } ); + return { scriptCount, scriptBytes, stylesheetCount, stylesheetBytes }; + } ); +} - test.afterAll( async ( {}, testInfo ) => { - await testInfo.attach( 'results', { - body: JSON.stringify( results, null, 2 ), - contentType: 'application/json', +for ( const screen of screens ) { + test.describe( screen.suiteTitle, () => { + let results = {}; + const record = ( key, value ) => { + results[ key ] ??= []; + results[ key ].push( value ); + }; + + for ( const locale of locales ) { + test.describe( `Locale: ${ locale }`, () => { + test.beforeAll( async ( { requestUtils } ) => { + await requestUtils.activateTheme( 'twentytwentyone' ); + await requestUtils.updateSiteSettings( { + language: 'en_US' === locale ? '' : locale, + } ); } ); - } ); - const iterations = Number( process.env.TEST_RUNS ); - for ( let i = 1; i <= iterations; i++ ) { - test( `Measure load time metrics (${ i } of ${ iterations })`, async ( { - page, - admin, - metrics, - } ) => { - // Clear caches using the clear-cache.php mu-plugin. Not actually loading the page. - await page.goto( '/?clear_cache' ); - - // This is the actual page to test. - await admin.visitAdminPage( '/' ); - - const serverTiming = await metrics.getServerTiming(); - - for ( const [ key, value ] of Object.entries( - serverTiming - ) ) { - results[ camelCaseDashes( key ) ] ??= []; - results[ camelCaseDashes( key ) ].push( value ); - } - - const ttfb = await metrics.getTimeToFirstByte(); - results.timeToFirstByte.push( ttfb ); + test.afterAll( async ( { requestUtils }, testInfo ) => { + await testInfo.attach( 'results', { + body: JSON.stringify( results, null, 2 ), + contentType: 'application/json', + } ); + + await requestUtils.updateSiteSettings( { + language: '', + } ); + + results = {}; } ); - } - } ); - } -} ); + + const iterations = Number( process.env.TEST_RUNS ); + for ( let i = 1; i <= iterations; i++ ) { + test( `Measure load time metrics (${ i } of ${ iterations })`, async ( { + page, + admin, + metrics, + } ) => { + // Clear caches using the clear-cache.php mu-plugin. Not actually loading the page. + await page.goto( '/?clear_cache' ); + + // This is the actual page to test. + await admin.visitAdminPage( screen.path ); + + const serverTiming = await metrics.getServerTiming(); + + for ( const [ key, value ] of Object.entries( + serverTiming + ) ) { + record( camelCaseDashes( key ), value ); + } + + record( + 'timeToFirstByte', + await metrics.getTimeToFirstByte() + ); + record( + 'domContentLoaded', + ( await metrics.getLoadingDurations() ).domContentLoaded + ); + + for ( const [ key, value ] of Object.entries( + await getPageAssetMetrics( page ) + ) ) { + record( key, value ); + } + } ); + } + } ); + } + } ); +} diff --git a/tests/performance/utils.js b/tests/performance/utils.js index b7481dfd67605..75facd3d1fe55 100644 --- a/tests/performance/utils.js +++ b/tests/performance/utils.js @@ -120,10 +120,14 @@ function formatValue( metric, value ) { return 1 === value ? 'yes' : 'no'; } - if ( 'wpDbQueries' === metric ) { + if ( 'wpDbQueries' === metric || metric.endsWith( 'Count' ) ) { return value; } + if ( metric.endsWith( 'Bytes' ) ) { + return `${ ( value / 1024 ).toFixed( 2 ) } KiB`; + } + return `${ value.toFixed( 2 ) } ms`; }