From 398b450ba02047b2d721533bad4af36654f20ddf Mon Sep 17 00:00:00 2001 From: rene Date: Thu, 12 Nov 2026 19:48:28 +0100 Subject: [PATCH 1/4] Build/Test Tools: Measure client-side metrics on admin screens in the performance tests. The admin performance spec only recorded Server-Timing and time to first byte for the Dashboard, so changes that add scripts or styles to every admin screen were not visible in the performance results. Enqueueing the Command Palette on all admin screens in [61022] added about 40 scripts and 5 MB of JavaScript to every classic admin screen without any change in the reported numbers. The spec now also records `domContentLoaded` (from `getLoadingDurations()`) and the number and size of the scripts and stylesheets in the page markup, as `scriptCount`, `scriptBytes`, `stylesheetCount` and `stylesheetBytes`, and it measures the General Settings screen (`options-general.php`) next to the Dashboard. The new suites are added to the results sent to codevitals, and `formatValue()` shows counts as plain numbers and byte sizes in KiB. The sizes use the Resource Timing `encodedBodySize`, not `transferSize`. The `/?clear_cache` request before each measurement loads the front end as a logged-in user, so the toolbar assets are already in the HTTP cache when the admin screen loads, and their `transferSize` is 0, or only the header size when they are revalidated. Which of the two happens depends on the files' Last-Modified date, so the numbers could change between builds without any code change. `encodedBodySize` is the same for cached and fetched responses. Each suite now starts from an empty results object, so the de_DE suites no longer include the Server-Timing values collected for en_US. See #66642, #63845. --- tests/performance/log-results.js | 2 + tests/performance/specs/admin.test.js | 165 +++++++++++++++++--------- tests/performance/utils.js | 6 +- 3 files changed, 116 insertions(+), 57 deletions(-) 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`; } From bb4b78abb14588be15a536d807f09210b639220d Mon Sep 17 00:00:00 2001 From: rene Date: Thu, 12 Nov 2026 19:48:37 +0100 Subject: [PATCH 2/4] Build/Test Tools: Check performance results against budgets. The performance workflow measures pull requests and their target commit, but it only shows the numbers in a collapsed job summary table. Nothing points out a regression and nothing fails, so a change that doubles the load time of every admin screen passes all checks. `check-regressions.js` compares the medians of this change with the target commit on pull requests, and with the last release on trunk pushes, metric by metric. A metric regresses when it grows by more than an absolute and a relative budget; timing metrics must also grow by more than three times the combined robust spread (MAD) of both samples, so CI noise does not trip the check. A metric whose target value is 0 can regress as well. Asset counts and sizes (`scriptBytes`, `stylesheetBytes`), database queries and memory usage are deterministic for a build. Exceeding their budget fails the Compare job on pull requests. Timings only warn. The comparison with the last release is reported but never fails, which gives trunk commits a release-to-release check as well. A crash of the check itself does not fail trunk. The `Accepted Performance Regression` label marks a regression as reviewed, so the job reports it without failing. The label is read from the pull request event, so it takes effect with the next push; re-running a job keeps the labels of the original event. The results are written at the top of the job summary and as workflow annotations. See #66642, #63845. --- .github/workflows/performance.yml | 2 + .../reusable-performance-report-v2.yml | 24 ++ tests/performance/check-regressions.js | 299 ++++++++++++++++++ 3 files changed, 325 insertions(+) create mode 100644 tests/performance/check-regressions.js 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..472a2ddeee6f0 --- /dev/null +++ b/tests/performance/check-regressions.js @@ -0,0 +1,299 @@ +#!/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] + * + * Environment variables: + * - PERFORMANCE_REGRESSION_MODE: `fail` exits with code 1 when a metric with the + * `error` severity regressed. 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 a regression there is real and fails the check. + */ +const deterministic = { minAbsolute: 2, minRelative: 0.05, severity: 'error' }; + +/** + * Timings carry CI noise, so they must also grow by more than `noiseFactor` + * times the combined spread of both samples, and are reported as warnings. + */ +const timing = { + minAbsolute: 10, + minRelative: 0.1, + noiseFactor: 3, + severity: 'warning', +}; + +/** + * 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, severity: string}} 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, + severity: budget.severity, + }; +} + +/** + * Returns the regressions of every scenario measured on both sides. + * + * @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} & 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 ); + + return Object.keys( budgets ) + .filter( + ( metric ) => + afterValues[ metric ]?.length && + beforeValues[ metric ]?.length + ) + .map( ( metric ) => + compareMetricToBudget( + metric, + beforeValues[ metric ], + afterValues[ metric ] + ) + ) + .filter( ( comparison ) => comparison.regressed ) + .map( ( comparison ) => ( { title, ...comparison } ) ); + } ); +} + +/** + * 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 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 ? 'x' : 'warning' }: ${ + regressions.length + } ${ + regressions.length === 1 ? 'metric' : 'metrics' + } got worse by more than the budget allows.\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: regression.severity, + } ) ) + ) }\n`; + } + + if ( failed && regressionAccepted ) { + markdown += + 'This change is marked as an accepted performance regression, so the check does not fail.\n\n'; + } + + return { markdown, failed: failed && ! regressionAccepted }; +} + +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 + + 'Asset counts and sizes, database queries and memory usage are deterministic for a build, so exceeding their budget fails pull requests. ' + + 'Timings are reported as warnings, because shared CI runners add noise.\n'; + +if ( summaryFile ) { + writeFileSync( summaryFile, summaryMarkdown ); +} + +console.log( summaryMarkdown ); + +process.exitCode = targetReport.failed ? 1 : 0; From 392136aaf83e85e93a2adf21d75506b9d7b162eb Mon Sep 17 00:00:00 2001 From: rene Date: Thu, 12 Nov 2026 20:57:32 +0100 Subject: [PATCH 3/4] Build/Test Tools: Mark accepted regressions as accepted in the budget report. With the `Accepted Performance Regression` label, the job summary still showed the red cross and `error` in the severity column. Only one sentence below the table told it apart from a failing check, so reviewers could not see at a glance that the regression had been accepted. An accepted report now uses the warning sign, says in the same line that the label accepts the regression, and lists error-severity metrics as `error (accepted)`. The separate sentence below the table is gone. Reports without the label and the exit codes are unchanged. See #66685. --- tests/performance/check-regressions.js | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/tests/performance/check-regressions.js b/tests/performance/check-regressions.js index 472a2ddeee6f0..11227a25bb2a9 100644 --- a/tests/performance/check-regressions.js +++ b/tests/performance/check-regressions.js @@ -207,6 +207,7 @@ function checkAgainstReference( reference, afterStats ) { ( regression ) => regression.severity === 'error' ); const failed = reference.enforced && hasErrors; + const failureAccepted = failed && regressionAccepted; const errorRegressionsFailCheck = reference.enforced && ! regressionAccepted; @@ -234,11 +235,15 @@ function checkAgainstReference( reference, afterStats ) { markdown += ':white_check_mark: No metric got worse by more than its budget allows.\n\n'; } else { - markdown += `:${ hasErrors ? 'x' : 'warning' }: ${ + markdown += `:${ hasErrors && ! failureAccepted ? 'x' : 'warning' }: ${ regressions.length } ${ regressions.length === 1 ? 'metric' : 'metrics' - } got worse by more than the budget allows.\n\n`; + } 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, @@ -246,17 +251,15 @@ function checkAgainstReference( reference, afterStats ) { Before: formatValue( regression.metric, regression.before ), After: formatValue( regression.metric, regression.after ), Change: formatGrowth( regression ), - Severity: regression.severity, + Severity: + failureAccepted && regression.severity === 'error' + ? 'error (accepted)' + : regression.severity, } ) ) ) }\n`; } - if ( failed && regressionAccepted ) { - markdown += - 'This change is marked as an accepted performance regression, so the check does not fail.\n\n'; - } - - return { markdown, failed: failed && ! regressionAccepted }; + return { markdown, failed: failed && ! failureAccepted }; } const afterStats = parseFile( 'performance-results.json' ); From 7ffc143e4ebede07db9057d7a7cef191428be7f8 Mon Sep 17 00:00:00 2001 From: rene Date: Thu, 12 Nov 2026 23:08:50 +0100 Subject: [PATCH 4/4] Build/Test Tools: Fail the budget check only when wp-admin gets slower. The check failed a pull request as soon as a screen loaded more scripts or bytes than a small budget allowed. Code grows with every feature, so growth on its own is no reason to block a change. What should not happen is a screen getting slower. A timing regression (DOMContentLoaded, LCP, TTFB, server time) now fails the check when the same screen also loads more scripts or stylesheets, runs more database queries or uses more memory. The growth confirms that the slowdown comes from the change and not from runner noise. Growth without a slowdown is still listed in the summary and as a warning annotation, so reviewers see right away when a change adds, for example, 100 KiB of JavaScript and can judge it. A slowdown without any growth is reported as a warning as well, since timings on shared CI runners vary. On the Command Palette change, the check reports 8 errors for the slower screens and 16 warnings for the added scripts and stylesheets. See #66685. --- tests/performance/check-regressions.js | 50 ++++++++++++++++++-------- 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/tests/performance/check-regressions.js b/tests/performance/check-regressions.js index 11227a25bb2a9..76c1899367c8d 100644 --- a/tests/performance/check-regressions.js +++ b/tests/performance/check-regressions.js @@ -6,9 +6,15 @@ * * 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 metric with the - * `error` severity regressed. Any other value only reports. Defaults to `report`. + * - 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. */ @@ -37,26 +43,26 @@ const regressionAccepted = /** * Asset counts and sizes, database queries and memory are deterministic for a - * given build, so a regression there is real and fails the check. + * given build, so any growth beyond the budget is real. */ -const deterministic = { minAbsolute: 2, minRelative: 0.05, severity: 'error' }; +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, and are reported as warnings. + * times the combined spread of both samples. */ const timing = { minAbsolute: 10, minRelative: 0.1, noiseFactor: 3, - severity: 'warning', + 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} + * @type {Record} */ const budgets = { scriptCount: deterministic, @@ -92,7 +98,7 @@ const MAD_TO_STANDARD_DEVIATION = 1.4826; * @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, severity: string}} Comparison. + * @return {{metric: string, before: number, after: number, delta: number, relative: number, regressed: boolean}} Comparison. */ function compareMetricToBudget( metric, before, after ) { const budget = budgets[ metric ]; @@ -121,16 +127,18 @@ function compareMetricToBudget( metric, before, after ) { delta, relative, regressed, - severity: budget.severity, }; } /** * 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} & ReturnType>} Regressions. + * @return {Array<{title: string, severity: 'error'|'warning'} & ReturnType>} Regressions. */ function findRegressions( beforeStats, afterStats ) { return afterStats.flatMap( ( { title, results } ) => { @@ -142,7 +150,7 @@ function findRegressions( beforeStats, afterStats ) { const afterValues = accumulateValues( results ); const beforeValues = accumulateValues( beforeStat.results ); - return Object.keys( budgets ) + const regressions = Object.keys( budgets ) .filter( ( metric ) => afterValues[ metric ]?.length && @@ -155,8 +163,20 @@ function findRegressions( beforeStats, afterStats ) { afterValues[ metric ] ) ) - .filter( ( comparison ) => comparison.regressed ) - .map( ( comparison ) => ( { title, ...comparison } ) ); + .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', + } ) ); } ); } @@ -290,8 +310,8 @@ const summaryMarkdown = '## Performance Budget Check\n\n' + targetReport.markdown + releaseReport.markdown + - 'Asset counts and sizes, database queries and memory usage are deterministic for a build, so exceeding their budget fails pull requests. ' + - 'Timings are reported as warnings, because shared CI runners add noise.\n'; + '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 );