diff --git a/tools/performance/README.md b/tools/performance/README.md index 229b2dc3de16..576b4795edd5 100644 --- a/tools/performance/README.md +++ b/tools/performance/README.md @@ -43,6 +43,8 @@ Each scenario posts its metrics in a single CodeVitals call per run (one per `me `admin.php?page=jetpack-forms-responses-wp-admin&p=%2Fresponses%2Finbox`, measured on the same simulated-connection instance as the Dashboard. The `p` route is pinned to the responses inbox: a bare page URL server-redirects to the default tab (`/forms`, the forms list, under Central Form Management), so the scenario asserts the final URL to avoid measuring the wrong page. +Readiness (FORMS-729): this scenario is the one that does **not** use `networkidle` — the page's `canUser` OPTIONS probe to `/wp/v2/settings` can stay pending forever in the local fixture, which would black-hole every navigation. It sets `loadState: 'load'` and readiness is carried by the visible `.boot-layout` selector, the hydration wait, and an **in-flight-aware resource settle**: the completed-resource count must hold steady while an in-flight-request ledger (which excludes only that one known-stuck probe) reads zero — `networkidle`'s own quiet + nothing-in-flight guarantee, minus the request that breaks it. If either signal is still active at the settle's deadline the iteration fails closed. The settle proves quiescence, and shares `networkidle`'s inherent blind spot for a gap before the page issues its next resource wave; in that gap the working defense is the settle's ~1s-quiet requirement (double `networkidle`'s 500ms), with `minResourceCount` catching captures below its floor (the wide `decodedBytesKB` sanity range cannot catch an undercount). See the comments on the scenario in `scenarios.js` for the full mechanics. + | CodeVitals key | Field | Type | Description | | ------------------------------------------------------- | ---------------- | ---------------- | --------------------------------------------------------- | | `forms-responses-connection-sim-largestContentfulPaint` | `lcp` | `lcp` | Forms responses LCP | @@ -85,7 +87,7 @@ Two conditions must hold for My Jetpack to render in the fixture: This tooling flips `jetpack_offline_mode` off install-wide (required for My Jetpack, condition 1 above). Because one WordPress install serves every scenario, this shifts what the **existing** `wp-admin-dashboard-connection-sim-*` and `forms-responses-connection-sim-*` trends measure at the commit it lands: Jetpack runs more code paths when it is not offline. Locally measured before/after on the Dashboard scenario (the one existing scenario that measures cleanly here — see the Forms note below) was small: LCP 140→140 ms, TTFB 57→60 ms, FCP 140→140 ms, decodedBytesKB 4098→4205, resources 89→98. The timing metrics move within noise; the real signal is +9 resources / +107 KB decoded (the extra non-offline code paths). Expect a one-time baseline level shift of that order at the landing commit — every later point measures the non-offline fixture, so the trend settles at the new level rather than returning to the old one. It is a measurement-boundary change, not an ongoing regression. -The `forms-responses-*` trends could not be measured before/after locally: in the local fixture the Forms responses page's `GET /wp/v2/settings` REST request hangs server-side (>60 s, `networkidle` never settles), so every iteration times out (re-verified 2026-07-10 on mirror commit `9ef44a8`). This is a pre-existing local-fixture issue, independent of the offline flip (it hangs with offline on or off) and unrelated to this change — the Forms scenario shipped in a prior PR. Flagged here so a Forms-trend gap around this commit is not mistaken for a regression. +The `forms-responses-*` trends could not be measured before/after locally at the time this landed: the Forms page's `canUser` OPTIONS probe to `/wp/v2/settings` stalls in the local headless-Chromium fixture (later tracing for FORMS-729 showed the server answers in milliseconds — a browser-side delivery stall, local-only, with offline on or off), so under the scenario's original `networkidle` gate every iteration timed out. FORMS-729 has since moved the Forms scenario to `loadState: 'load'` plus a fail-closed resource-count settle (see `scenarios.js`), which measures cleanly despite the stall. Flagged here so a Forms-trend gap around this commit is not mistaken for a regression. ### Known fixture behavior on the My Jetpack page @@ -148,7 +150,7 @@ Post a new metric to a `-staging` CodeVitals key first (e.g. `…-timeToFirstByt A scenario that measures a specific admin page (a `path` + `waitForSelector`, like `formsResponses`) can declare two optional guards so a mis-captured page never reaches its permanent keys. Both fail the iteration closed — a failed capture posts nothing rather than a wrong number. - **`expectUrlIncludes`** — a substring the page's final URL must contain after every redirect settles. It defends the concrete redirect threat: `class-dashboard.php` sends a bare page URL to the forms LIST, which strips the pinned `p=/responses/inbox`, so the guard fires. It does not prove the SPA client-rendered the target route — a client-side divergence that keeps the URL would pass. That is deliberate: a guessed DOM-selector assertion would throw on every iteration if the markup shifts, blackholing the whole series on the append-only store, so the URL check is the safer defense for the redirect it targets. -- **`minResourceCount`** — an iteration whose capture returns fewer resources than a healthy load (40 for `formsResponses`, against a real ~80) is dropped from the sample; if every iteration falls short the run posts nothing. This is a **count** floor, not an "editor asset is present" check: the bundle-size metric is meant to fall when the editor lazy-loads, which removes a few large files rather than the bulk of the count, so a count floor catches a truncated capture without clipping the legitimate improvement. +- **`minResourceCount`** — an iteration whose capture returns fewer resources than a healthy load is dropped from the sample; if every iteration falls short the run posts nothing. The current values live in `scenarios.js` (the single source of truth — as of FORMS-729, 64 for `formsResponses` and `myJetpack`, ~70% of each page's real load) rather than being restated here where they would drift. This is a **count** floor, not an "editor asset is present" check: the bundle-size metric is meant to fall when the editor lazy-loads, which removes a few large files rather than the bulk of the count, so a count floor catches a truncated capture without clipping the legitimate improvement. Scope: the floor catches captures *below* its value; a capture between the floor and the real count relies on the readiness settle (see the `formsResponses` readiness note above) and `SANITY_RANGES`. ### Per-scenario failure isolation diff --git a/tools/performance/scripts/measure-lcp.js b/tools/performance/scripts/measure-lcp.js index 6d3c00d24dfd..f60b4b246e25 100644 --- a/tools/performance/scripts/measure-lcp.js +++ b/tools/performance/scripts/measure-lcp.js @@ -47,7 +47,7 @@ const calibration = loadCalibration(); * @param {string} password - wp-admin password. * @param {number} iterations - Number of measurement iterations. * @param {object} [scenario] - Scenario config; reads optional `path`, `waitForSelector`, - * `expectUrlIncludes`, and `minResourceCount`. + * `expectUrlIncludes`, `loadState`, and `minResourceCount`. * @return {Promise} { summary, results, url }. */ async function measureLCP( url, username, password, iterations = 5, scenario = {} ) { @@ -62,6 +62,16 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { // against measuring the wrong page — e.g. a bare Forms URL redirecting to /forms instead of // the responses inbox — which would populate this scenario's permanent keys from off-target. const expectUrlIncludes = scenario.expectUrlIncludes || null; + // The Playwright load state each navigation waits for. Defaults to 'networkidle' (the settled + // signal every scenario used originally). A scenario whose page keeps a request perpetually + // pending — e.g. the Forms dashboard's canUser OPTIONS to /wp/v2/settings stalls in the + // headless-Chromium fixture — sets 'load' so a single un-settling request cannot blackhole the + // whole scenario; readiness is then carried by the visible-selector + hydration + resource-count + // settle below, not by network quiescence. See scenarios.js (formsResponses) and FORMS-729. + const navWaitUntil = scenario.loadState || 'networkidle'; + // The one predicate the readiness plumbing branches on, bound once so the ledger creation, + // the warm-up settle and the measured settle can never disagree about which path they are on. + const useResourceSettle = navWaitUntil !== 'networkidle'; console.log( `Measuring LCP for ${ url }${ targetPath || '' } (${ iterations } iterations)...` ); @@ -91,9 +101,52 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { } } + // In-flight request ledger for the non-networkidle path: counts every request from issue + // until finished/failed, excluding the known-stuck settings probe, so the resource-count + // settle can refuse to report quiet while legitimate work is still in flight (the + // completed-entries count alone cannot see an in-flight request). null on the networkidle + // path, which needs no ledger — Playwright's networkidle already tracks in-flight requests. + const pendingRequests = useResourceSettle + ? trackPendingRequests( page, isStuckSettingsProbe ) + : null; + try { console.log( ` Iteration ${ i + 1 }/${ iterations }...` ); + // Step 0: Set up LCP capture and the enlarged Resource Timing buffer via addInitScript. + // This injects code that runs BEFORE any page script on EVERY navigation from here on + // (login, warm-up and the measured reload — each document gets a fresh copy, so the + // reload's __lcpEntries never contain earlier pages' entries). Installed before the + // FIRST navigation on purpose: the warm-up resource-count settle reads the timing + // buffer, and the browser's 250-entry default would silently cap (and false-settle) + // the count once the page's real load grows past it. + /* eslint-disable no-undef -- This runs in browser context via Playwright */ + await context.addInitScript( () => { + // This runs in the browser context before page load + + // Raise the Resource Timing buffer well above the default 250 entries. This metric + // exists to watch a GROWING count of @wordpress/* editor module files, so the + // measured quantity and the default cap would collide exactly as the tracked + // regression worsens — past 250 the tail would drop and the decoded-bytes sum would + // silently under-count. (~91 resources today; this is headroom, not a live fix.) + performance.setResourceTimingBufferSize( 10000 ); + + window.__lcpEntries = []; + window.__lcpObserver = new PerformanceObserver( list => { + const entries = list.getEntries(); + for ( const entry of entries ) { + window.__lcpEntries.push( { + startTime: entry.startTime, + element: entry.element?.tagName || 'unknown', + size: entry.size, + url: entry.url, + } ); + } + } ); + window.__lcpObserver.observe( { type: 'largest-contentful-paint', buffered: true } ); + } ); + /* eslint-enable no-undef */ + // Step 1: Log in to WordPress (not measured) await page.goto( `${ url }/wp-login.php`, { waitUntil: 'networkidle', @@ -118,44 +171,24 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { // ready selector before measuring. The Dashboard scenario has no path and skips this. if ( targetPath ) { await page.goto( `${ url }${ targetPath }`, { - waitUntil: 'networkidle', + waitUntil: navWaitUntil, timeout: 60000, } ); await page.waitForSelector( pageReadySelector, { timeout: 30000 } ); + if ( useResourceSettle ) { + // Warm-up parity with the networkidle scenarios: on the default path this goto + // waits for network quiescence, so every cacheable resource is warm before the + // measured reload. With `load` the goto returns while route/post-mount resources + // may still be in flight, and reloading then would abort them — measuring a + // partially cold cache. Settle on the resource count here so the reload below + // measures the same warmed page networkidle used to guarantee. + await settleOrThrow( page, pendingRequests, 'warm-up navigation' ); + } } - // Step 2: Set up LCP capture using addInitScript - // This injects code that runs BEFORE any page script on every navigation - /* eslint-disable no-undef -- This runs in browser context via Playwright */ - await context.addInitScript( () => { - // This runs in the browser context before page load - - // Raise the Resource Timing buffer well above the default 250 entries. This metric - // exists to watch a GROWING count of @wordpress/* editor module files, so the - // measured quantity and the default cap would collide exactly as the tracked - // regression worsens — past 250 the tail would drop and the decoded-bytes sum would - // silently under-count. (~79 resources today; this is headroom, not a live fix.) - performance.setResourceTimingBufferSize( 10000 ); - - window.__lcpEntries = []; - window.__lcpObserver = new PerformanceObserver( list => { - const entries = list.getEntries(); - for ( const entry of entries ) { - window.__lcpEntries.push( { - startTime: entry.startTime, - element: entry.element?.tagName || 'unknown', - size: entry.size, - url: entry.url, - } ); - } - } ); - window.__lcpObserver.observe( { type: 'largest-contentful-paint', buffered: true } ); - } ); - /* eslint-enable no-undef */ - - // Step 3: Reload for a clean measurement of the current page — the Dashboard, or the + // Step 2: Reload for a clean measurement of the current page — the Dashboard, or the // page navigated to above. - await page.reload( { waitUntil: 'networkidle', timeout: 60000 } ); + await page.reload( { waitUntil: navWaitUntil, timeout: 60000 } ); // Wait for the measured page's content to be present after reload. if ( pageReadySelector ) { @@ -178,10 +211,23 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { await page.waitForSelector( '#dashboard-widgets, #wpbody-content', { timeout: 30000 } ); } - // Wait for network to settle and LCP to finalize - // LCP stops updating after user input or visibility change - // Using networkidle is more reliable than a fixed timeout on slow systems - await page.waitForLoadState( 'networkidle', { timeout: 30000 } ); + // Wait for the resource payload to finish loading and LCP to finalize (LCP stops + // updating after user input or visibility change). + if ( ! useResourceSettle ) { + // Default path (Dashboard, My Jetpack): network quiescence is a reliable + // "everything loaded" signal and more robust than a fixed timeout on slow systems. + await page.waitForLoadState( 'networkidle', { timeout: 30000 } ); + } else { + // Resilient path (scenarios with a perpetually-pending request, e.g. Forms — see + // navWaitUntil): `networkidle` never fires, so settle on the completed-resource count + // going quiet AND no legitimate request in flight (the pendingRequests ledger, which + // excludes only the known-stuck probe) — networkidle's own guarantee minus the one + // request that breaks it. A never-delivered excluded request can't stall this, while + // genuine late resources still hold the wait open via the ledger or the count. If + // either signal is still active when the cap expires, fail the iteration (fail + // closed) rather than capture a still-loading page's bundle size. + await settleOrThrow( page, pendingRequests, 'measured reload' ); + } // Additional short wait for any final rendering after network settles await page.waitForTimeout( 500 ); @@ -303,6 +349,7 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { timestamp: new Date().toISOString(), } ); } finally { + pendingRequests?.dispose(); await browser.close(); } @@ -518,6 +565,180 @@ function summarizeResources( resources ) { }; } +/** + * Settle by watching the completed-resource count go quiet — a `networkidle` substitute that a + * perpetually-pending request cannot stall. + * + * `page.waitForLoadState('networkidle')` fires only when there are ~no in-flight requests for + * 500ms; a single request that never delivers a response (the Forms dashboard's canUser OPTIONS to + * /wp/v2/settings does exactly this in the headless-Chromium fixture) keeps it in-flight forever, so + * networkidle never fires and the whole measurement times out. This instead polls + * `performance.getEntriesByType('resource').length`, which only counts *completed* requests: a + * never-delivered request never appears, so it can't hold the count from settling, while genuine + * late resources (lazy editor modules) still bump the count and extend the wait. Resolves once the + * count is unchanged across `stableChecks` consecutive polls, or when `maxWaitMs` elapses (whichever + * first), so a page that keeps streaming resources still returns rather than hanging. + * + * On its own the completed count cannot see an IN-FLIGHT request (a Resource Timing entry appears + * only at responseEnd), so a slow legitimate response could look like quiet. Pass `pendingCount` + * (see trackPendingRequests) and the settle also requires zero relevant in-flight requests on + * every stable poll — together that is `networkidle`'s own guarantee (quiet + nothing in flight) + * minus the excluded stuck request. What remains possible in BOTH designs is a gap before the + * page issues its next wave (nothing in flight, count flat); that pre-existing networkidle window + * is why `minResourceCount` + SANITY_RANGES stay as backstops. This settle detects quiescence — + * it does not by itself prove completeness. + * + * The result says WHICH of the two happened: `settled: true` means genuine stability was observed; + * `settled: false` means the cap expired with the count still changing or a request still in + * flight — an incomplete capture the caller must treat as a failed iteration (see + * assertResourceCountSettled), never measure. + * + * @param {import('playwright').Page} page - The page being measured. + * @param {object} [opts] - Tuning knobs. + * @param {number} [opts.intervalMs=200] - Poll interval. + * @param {number} [opts.stableChecks=5] - Consecutive unchanged polls required (≈1s quiet). + * @param {number} [opts.maxWaitMs=20000] - Deadline on the polling loop. Checked between polls, so it bounds loop scheduling — it does not interrupt a single wedged `page.evaluate` (that failure mode throws or hangs the CDP session itself). + * @param {Function} [opts.pendingCount] - Returns the number of relevant in-flight requests (trackPendingRequests().count). While it returns > 0, polls do not count toward stability. + * @return {Promise<{settled: boolean, count: number}>} Whether stability was reached before the deadline, and the last completed-resource count observed. + */ +async function waitForResourceCountIdle( page, opts = {} ) { + const { intervalMs = 200, stableChecks = 5, maxWaitMs = 20000, pendingCount = null } = opts; + const deadline = Date.now() + maxWaitMs; + let last = -1; + let stable = 0; + while ( Date.now() < deadline ) { + const count = await page.evaluate( () => performance.getEntriesByType( 'resource' ).length ); + const busy = pendingCount ? pendingCount() > 0 : false; + + if ( count === last && ! busy ) { + stable += 1; + if ( stable >= stableChecks ) { + return { settled: true, count }; + } + } else { + stable = 0; + last = count; + } + await page.waitForTimeout( intervalMs ); + } + return { settled: false, count: last }; +} + +/** + * The one request the in-flight ledger must ignore: the wp-build boot framework's `canUser` + * OPTIONS probe to /wp/v2/settings, whose response is intermittently never delivered to the + * browser in the local headless-Chromium fixture (the very request that forced the Forms + * scenario off `networkidle` — see scenarios.js). Matched by method + decoded URL because the + * fixture requests it via `rest_route=%2Fwp%2Fv2%2Fsettings`. Deliberately narrow: ONLY the + * settings OPTIONS probe is excluded, so any other stuck request holds the settle open and + * fails the iteration at the deadline (fail closed) instead of being silently ignored. + * + * @param {import('playwright').Request} request - A Playwright request. + * @return {boolean} True when this is the known-stuck settings probe. + */ +function isStuckSettingsProbe( request ) { + if ( request.method() !== 'OPTIONS' ) { + return false; + } + let parsed; + try { + parsed = new URL( request.url() ); + } catch { + // Not an absolute URL — cannot be the probe; leave it in the ledger (fail closed). + return false; + } + // The REST route this request targets: the `rest_route` query value (the fixture's + // plain-permalink shape — searchParams decodes the %2F encoding), or the /wp-json/ + // pathname under pretty permalinks. Compared EXACTLY (modulo a trailing slash) so adjacent + // routes (/wp/v2/settings/child, /wp/v2/settings-extra) and unrelated URLs that merely + // carry the string in a query value never match — those must stay in the ledger and fail + // the iteration if they get stuck. + const route = + parsed.searchParams.get( 'rest_route' ) ?? parsed.pathname.replace( /^\/wp-json(?=\/)/, '' ); + return route.replace( /\/+$/, '' ) === '/wp/v2/settings'; +} + +/** + * Ledger of in-flight page requests, for the settle's `pendingCount` option. Playwright fires + * `request` when a request is issued and `requestfinished`/`requestfailed` when it completes or + * aborts (navigations abort outstanding requests, which fires `requestfailed`), so the set size + * is the number of requests currently in flight. Requests matching `isExcluded` are never added + * — that is how the known-stuck probe is prevented from holding the settle open forever. + * + * @param {import('playwright').Page} page - The page to track. + * @param {function(import('playwright').Request): boolean} isExcluded - Requests to ignore. + * @return {{count: function(): number, dispose: function(): void}} `count()` for the settle's pendingCount option; `dispose()` detaches the listeners (call in the iteration's finally). + */ +function trackPendingRequests( page, isExcluded ) { + const pending = new Set(); + const onRequest = request => { + if ( ! isExcluded( request ) ) { + pending.add( request ); + } + }; + const onSettled = request => pending.delete( request ); + page.on( 'request', onRequest ); + page.on( 'requestfinished', onSettled ); + page.on( 'requestfailed', onSettled ); + return { + count: () => pending.size, + dispose: () => { + page.off( 'request', onRequest ); + page.off( 'requestfinished', onSettled ); + page.off( 'requestfailed', onSettled ); + }, + }; +} + +/** + * Fail-closed gate on the resource-count settle: a capped-out settle means the page was still + * loading resources when the deadline hit, so measuring it would record a truncated bundle size + * (and possibly a premature LCP) that passes the coarse `minResourceCount`/SANITY_RANGES guards + * and lands on a permanent, no-rollback CodeVitals key looking exactly like a real regression. + * Throwing here turns that silent undercount into a failed iteration: the per-iteration catch in + * measureLCP records the error and the run continues on the remaining iterations, so one slow + * load costs a sample, not the scenario. + * + * @param {{settled: boolean, count: number}} result - Return value of waitForResourceCountIdle. + * @param {string} phase - Which settle this was (for the error message). + * @throws {Error} When the settle capped out instead of reaching stability. + */ +function assertResourceCountSettled( result, phase ) { + if ( ! result.settled ) { + // Neutral wording on purpose: settled:false means the completed count was still moving + // OR a relevant request was still in flight at the deadline — the result cannot say + // which, so the message must not claim "count still changing" when a stuck request may + // be the actual holdout. + throw new Error( + `Page never settled during ${ phase }: resources or in-flight requests were still active (completed count ${ result.count }) when the deadline expired — failing this iteration rather than measuring a still-loading page` + ); + } +} + +/** + * The settle-or-throw step both non-networkidle readiness sites share (warm-up navigation and + * measured reload): run the in-flight-aware settle with the iteration's pending-request ledger + * threaded in, and fail the iteration (fail closed) if it caps out. Extracted so the ledger + * threading is a single, unit-tested seam — a call site that silently dropped `pendingCount` + * would reopen the slow-response undercount this exists to close, and as a plain inline option + * object that regression survived the whole test suite. + * + * @param {import('playwright').Page} page - The page being measured. + * @param {{count: function(): number}} pendingRequests - The iteration's ledger (trackPendingRequests). + * @param {string} phase - Which settle this is (for the error message). + * @param {object} [settleOpts] - waitForResourceCountIdle tuning overrides (tests only; `pendingCount` cannot be overridden). + * @throws {Error} When the settle caps out instead of reaching stability. + */ +async function settleOrThrow( page, pendingRequests, phase, settleOpts = {} ) { + assertResourceCountSettled( + await waitForResourceCountIdle( page, { + ...settleOpts, + pendingCount: pendingRequests.count, + } ), + phase + ); +} + /** * Content-completeness guard for the bundle-size metric. When a scenario declares the minimum * resource count a healthy load produces (`minResourceCount`), throw if the capture returned @@ -834,7 +1055,12 @@ export { finalizeMeasurement, assertCaptureComplete, assertExpectedUrl, + assertResourceCountSettled, + isStuckSettingsProbe, + settleOrThrow, + trackPendingRequests, summarizeResources, resolveScenarioSet, computeRunOutcome, + waitForResourceCountIdle, }; diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js index db79b5d41bf0..d395c2f9304d 100644 --- a/tools/performance/scripts/post-to-codevitals.test.js +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -22,7 +22,12 @@ import { resolveResultsGit, assertCaptureComplete, assertExpectedUrl, + assertResourceCountSettled, + isStuckSettingsProbe, + settleOrThrow, + trackPendingRequests, summarizeResources, + waitForResourceCountIdle, } from './measure-lcp.js'; import { checkSanityRange, @@ -507,13 +512,18 @@ test( 'the formsResponses scenario posts LCP, TTFB, FCP and decodedBytes to prod // wrong tab cannot quietly populate the responses keys from the forms list. assert.equal( scenario.expectUrlIncludes, '/responses/inbox' ); // A resource-count floor so a partial capture can't post an undercounted decodedBytesKB. - // Lower bound is 40 (not 1): a degenerate floor of a handful of resources would pass the old - // `> 0` check while catching nothing. Upper bound stays below the real ~80-resource load so the - // floor keeps its 2x margin and never clips the legitimate editor-lazy-load drop. - assert.ok( - scenario.minResourceCount >= 40 && scenario.minResourceCount < 80, - 'formsResponses must declare a resource-count floor of >=40 and below the real ~80-resource load' + // Pin the exact value (siblings pin every field by equality) so a later edit toward the + // ~91-resource load can't erode the margin silently, and exercise the real guard at the + // boundary: one below the floor must throw, the floor itself must not. Honest scope: the + // floor only catches captures BELOW 64 — a quiet-gap settle at 64–90 passes it (the same + // residual window networkidle itself has; see the residual-risk test in the FORMS-729 + // block) — so it is a backstop under the in-flight-aware settle, not a completeness proof. + assert.equal( scenario.minResourceCount, 64 ); + assert.throws( + () => assertCaptureComplete( { totalRequests: 63 }, scenario ), + /Incomplete capture: 63 resources < expected minimum 64/ ); + assert.doesNotThrow( () => assertCaptureComplete( { totalRequests: 64 }, scenario ) ); } ); test( 'the myJetpack scenario posts LCP, TTFB, FCP and decodedBytes to production keys', () => { @@ -2569,3 +2579,322 @@ test( 'a dry run makes no dedup read even when dedup is fully configured', async global.fetch = origFetch; } } ); + +// --- FORMS-729: forms readiness selector + stuck-request-proof settle --- + +/** + * Fake Playwright page for waitForResourceCountIdle. `counts` is the sequence + * performance.getEntriesByType('resource').length returns on successive polls; the + * last value is repeated once the sequence is exhausted. waitForTimeout is a no-op + * so the test doesn't actually sleep. `polls()` reports how many times the count + * was read, so a test can pin WHERE the loop stopped, not just that it stopped. + */ +function fakeResourcePage( counts ) { + let i = 0; + return { + evaluate: async () => counts[ Math.min( i++, counts.length - 1 ) ], + waitForTimeout: async () => {}, + polls: () => i, + }; +} + +test( 'waitForResourceCountIdle settles via early stability, not the deadline fallback', async () => { + // Count climbs 10→40→90 then holds; five equal polls (stableChecks) settle it. Pinning the + // exact poll count (3 climbing + 5 stable = 8) and settled:true distinguishes a working + // stability detector from a broken one that only returns because the cap expired — a + // disabled stability branch would read far past 8 polls and report settled:false. + const page = fakeResourcePage( [ 10, 40, 90, 90, 90, 90, 90, 90, 90, 90 ] ); + const result = await waitForResourceCountIdle( page, { + intervalMs: 0, + stableChecks: 5, + maxWaitMs: 5000, + } ); + assert.equal( result.settled, true, 'must reach genuine stability, not the cap' ); + assert.equal( result.count, 90, 'must report the stable count it observed' ); + assert.equal( page.polls(), 8, 'must stop at the 5th stable read, not poll to the cap' ); +} ); + +test( 'waitForResourceCountIdle resets its stability streak when the count moves again', async () => { + // Four equal reads of 50 (one short of stableChecks:5) then movement to 80: the streak must + // reset instead of counting the pre-movement reads toward stability, then five equal reads + // of 80 settle it. Total polls: 1×50-first + 4×50-stable + 1×80-first + 5×80-stable = 11. + const page = fakeResourcePage( [ 50, 50, 50, 50, 50, 80, 80, 80, 80, 80, 80, 80 ] ); + const result = await waitForResourceCountIdle( page, { + intervalMs: 0, + stableChecks: 5, + maxWaitMs: 5000, + } ); + assert.equal( result.settled, true ); + assert.equal( result.count, 80, 'must settle on the post-movement count, not the plateau' ); + assert.equal( page.polls(), 11, 'the 4-read plateau must not count toward the new streak' ); +} ); + +test( 'waitForResourceCountIdle caps at maxWaitMs and reports it did NOT settle', async () => { + // A count that increases every poll models a page that keeps streaming (or a buggy + // stub); the maxWait cap must return rather than loop forever, and must say so via + // settled:false — measure-lcp.js turns that into a failed iteration (fail closed) + // instead of capturing a still-loading page. A never-delivered request is the real + // stuck case: it never adds a resource entry, so the count would instead go flat — + // this test pins the harder "never flat" bound. + let polls = 0; + const page = { + evaluate: async () => ++polls, // strictly increasing: never stable + waitForTimeout: async () => {}, + }; + const start = Date.now(); + const result = await waitForResourceCountIdle( page, { + intervalMs: 0, + stableChecks: 5, + maxWaitMs: 50, + } ); + assert.equal( result.settled, false, 'cap expiry must be reported, not disguised as a settle' ); + assert.ok( polls > 0, 'it should have polled at least once' ); + // The deadline must bound the loop: returning takes ~maxWaitMs, not multiples of it. + assert.ok( Date.now() - start < 2000, 'must return near maxWaitMs, not far past it' ); +} ); + +test( 'assertResourceCountSettled fails a capped-out settle and passes a clean one', () => { + // The fail-closed gate measure-lcp.js applies to both settle sites (warm-up navigation and + // measured reload): a capped-out settle must throw (failing the iteration before capture), + // a genuine settle must not. The message must stay neutral about WHICH signal was active — + // settled:false can mean a moving count OR a stuck in-flight request, and the result cannot + // distinguish them, so claiming "count still changing" would misreport the stuck-request case. + assert.throws( + () => assertResourceCountSettled( { settled: false, count: 55 }, 'measured reload' ), + /never settled during measured reload.*resources or in-flight requests.*completed count 55/ + ); + assert.doesNotThrow( () => + assertResourceCountSettled( { settled: true, count: 91 }, 'measured reload' ) + ); +} ); + +test( 'settleOrThrow threads the in-flight ledger into the settle', async () => { + // Pins the exact one-line regression the round-3 review proved survivable: dropping + // `pendingCount` from a settle call site left the whole suite green while silently + // reverting the in-flight guarantee. Through settleOrThrow (the seam both production call + // sites use), a never-clearing pending request MUST fail the settle even though the + // completed count is flat — quiet from the count's point of view. + const page = { evaluate: async () => 89, waitForTimeout: async () => {} }; + await assert.rejects( + settleOrThrow( + page, + { count: () => 1 }, // a request that never finishes + 'measured reload', + { intervalMs: 0, stableChecks: 5, maxWaitMs: 50 } + ), + /never settled during measured reload/ + ); + // And the same flat count with an idle ledger settles cleanly. + await assert.doesNotReject( + settleOrThrow( page, { count: () => 0 }, 'warm-up navigation', { + intervalMs: 0, + stableChecks: 5, + maxWaitMs: 5000, + } ) + ); + // The ledger cannot be overridden through the tuning overrides. + await assert.rejects( + settleOrThrow( page, { count: () => 1 }, 'measured reload', { + intervalMs: 0, + stableChecks: 5, + maxWaitMs: 50, + pendingCount: () => 0, // must lose to the real ledger + } ), + /never settled/ + ); +} ); + +test( 'an in-flight request holds the settle open even while the completed count is flat', async () => { + // The completed-resource count cannot see an in-flight request (an entry appears only at + // responseEnd), so a flat count during a slow legitimate response would otherwise settle + // early and capture before that response lands. With pendingCount wired in, polls made + // while a request is in flight must not count toward stability: here the count is flat at + // 89 throughout, pending is 1 for the first 3 polls, so settling must take 3 + 5 polls — + // settling at poll 6 (ignoring pending) would reproduce the exact live fail-open this + // guards against. + let poll = 0; + const page = { + evaluate: async () => { + poll += 1; + return 89; + }, + waitForTimeout: async () => {}, + }; + const pendingByPoll = [ 1, 1, 1 ]; // then 0 forever + const result = await waitForResourceCountIdle( page, { + intervalMs: 0, + stableChecks: 5, + maxWaitMs: 5000, + pendingCount: () => pendingByPoll[ poll - 1 ] || 0, + } ); + assert.equal( result.settled, true ); + assert.equal( result.count, 89 ); + assert.equal( poll, 8, 'the 3 in-flight polls must not have counted toward stability' ); +} ); + +test( 'a request that never finishes (other than the excluded probe) fails the settle closed', async () => { + // A non-excluded request stuck in flight forever must hold the settle open until the + // deadline and be reported as settled:false — assertResourceCountSettled then fails the + // iteration instead of capturing a page still waiting on a legitimate response. + const page = { + evaluate: async () => 89, // completed count flat: quiet from the count's point of view + waitForTimeout: async () => {}, + }; + const result = await waitForResourceCountIdle( page, { + intervalMs: 0, + stableChecks: 5, + maxWaitMs: 50, + pendingCount: () => 1, // never clears + } ); + assert.equal( result.settled, false ); + assert.throws( () => assertResourceCountSettled( result, 'measured reload' ) ); +} ); + +test( 'documents the accepted residual: a quiet-gap settle at or above the floor still passes', async () => { + // Accepted residual risk, on record: when NOTHING is in flight and the page has not yet + // issued its next resource wave, the settle sees genuine quiet and reports settled at a + // count that can clear the 64 floor (here 70 of an eventual 91). networkidle has this exact + // window too (its 500ms quiet can fall in the same gap) — this is parity, not a new hole. + // In this window the working defense is the settle's ~1s-quiet requirement (double + // networkidle's 500ms); minResourceCount catches captures below 64, and the decodedBytesKB + // SANITY range is far too wide to catch an undercount (NOT a backstop here). If this test + // starts failing, the settle got stricter and this documentation should be updated, not the + // settle loosened. + const page = fakeResourcePage( [ 70, 70, 70, 70, 70, 70, 91 ] ); + const result = await waitForResourceCountIdle( page, { + intervalMs: 0, + stableChecks: 5, + maxWaitMs: 5000, + pendingCount: () => 0, // nothing in flight: the gap is invisible to the ledger too + } ); + assert.equal( result.settled, true ); + assert.equal( result.count, 70, 'settles at the plateau, never seeing the later resources' ); + const forms = SCENARIOS.find( s => s.key === 'formsResponses' ); + assert.doesNotThrow( + () => assertCaptureComplete( { totalRequests: 70 }, forms ), + '70 >= the 64 floor: the floor does not catch this case, by design' + ); +} ); + +test( 'isStuckSettingsProbe matches only the settings OPTIONS probe', () => { + const req = ( method, url ) => ( { method: () => method, url: () => url } ); + // The real fixture shape: OPTIONS via the encoded rest_route form (with extra params). + assert.equal( + isStuckSettingsProbe( + req( + 'OPTIONS', + 'http://localhost:8083/index.php?rest_route=%2Fwp%2Fv2%2Fsettings&_locale=user' + ) + ), + true + ); + // Pretty-permalink shape, with and without a trailing slash. + assert.equal( + isStuckSettingsProbe( req( 'OPTIONS', 'http://localhost:8083/wp-json/wp/v2/settings' ) ), + true + ); + assert.equal( + isStuckSettingsProbe( req( 'OPTIONS', 'http://localhost:8083/wp-json/wp/v2/settings/' ) ), + true + ); + // Same URL but a real data request: never excluded. + assert.equal( + isStuckSettingsProbe( req( 'GET', 'http://localhost:8083/wp-json/wp/v2/settings' ) ), + false + ); + // Other OPTIONS probes: never excluded — any of them getting stuck must fail the iteration. + assert.equal( + isStuckSettingsProbe( + req( 'OPTIONS', 'http://localhost:8083/index.php?rest_route=%2Fwp%2Fv2%2Ffeedback' ) + ), + false + ); + // The exact-route boundary (round-4 review, empirically driven through the settle): adjacent + // routes and URLs merely CARRYING the string must never be excluded — the match is the + // decoded route compared exactly, not a substring. + assert.equal( + isStuckSettingsProbe( + req( 'OPTIONS', 'http://localhost:8083/wp-json/wp/v2/settings/autosaves' ) + ), + false, + 'a child route must not be excluded' + ); + assert.equal( + isStuckSettingsProbe( req( 'OPTIONS', 'http://localhost:8083/wp-json/wp/v2/settings-extra' ) ), + false, + 'a sibling route sharing the prefix must not be excluded' + ); + assert.equal( + isStuckSettingsProbe( + req( 'OPTIONS', 'http://localhost:8083/index.php?rest_route=%2Fwp%2Fv2%2Fsettings%2Fchild' ) + ), + false, + 'a child route in rest_route form must not be excluded' + ); + assert.equal( + isStuckSettingsProbe( + req( 'OPTIONS', 'http://localhost:8083/wp-json/wp/v2/feedback?next=%2Fwp%2Fv2%2Fsettings' ) + ), + false, + 'the settings string in an unrelated query value must not be excluded' + ); + // A non-URL or malformed value must not throw — and must stay in the ledger (fail closed). + assert.equal( isStuckSettingsProbe( req( 'OPTIONS', 'not a url' ) ), false ); + assert.equal( isStuckSettingsProbe( req( 'OPTIONS', 'http://x/%E0%A4%A' ) ), false ); +} ); + +test( 'trackPendingRequests counts in-flight requests, skips excluded ones, and detaches cleanly', () => { + // Fake Playwright page event surface: on/off registries keyed by event name. + const handlers = {}; + const page = { + on: ( event, fn ) => { + ( handlers[ event ] ||= [] ).push( fn ); + }, + off: ( event, fn ) => { + handlers[ event ] = ( handlers[ event ] || [] ).filter( h => h !== fn ); + }, + }; + const emit = ( event, arg ) => ( handlers[ event ] || [] ).forEach( fn => fn( arg ) ); + const excluded = { id: 'stuck' }; + const tracker = trackPendingRequests( page, r => r === excluded ); + + const a = { id: 'a' }; + const b = { id: 'b' }; + emit( 'request', a ); + emit( 'request', b ); + emit( 'request', excluded ); // the stuck probe: never enters the ledger + assert.equal( tracker.count(), 2 ); + emit( 'requestfinished', a ); + assert.equal( tracker.count(), 1 ); + // Navigations abort outstanding requests as requestfailed — must also clear the ledger. + emit( 'requestfailed', b ); + assert.equal( tracker.count(), 0 ); + // Removing something never added (the excluded probe finishing) is a harmless no-op. + emit( 'requestfinished', excluded ); + assert.equal( tracker.count(), 0 ); + + tracker.dispose(); + emit( 'request', { id: 'after-dispose' } ); + assert.equal( tracker.count(), 0, 'a disposed tracker must not keep counting' ); +} ); + +test( 'formsResponses waits on the visible layout, not the 0-height mount point', () => { + const forms = SCENARIOS.find( s => s.key === 'formsResponses' ); + assert.ok( forms, 'formsResponses scenario must exist' ); + // The mount point #jetpack-forms-responses-wp-admin-app.boot-layout-container computes to + // height 0 post-#49272 (content moved into a position:absolute child), so a *visible*-state + // wait on it never resolves. The selector must instead target the rendered .boot-layout. + assert.equal( + forms.waitForSelector, + '#jetpack-forms-responses-wp-admin-app .boot-layout', + 'forms selector must target the rendered layout, not the 0-height container' + ); +} ); + +test( 'formsResponses opts out of networkidle so a stuck request cannot blackhole it', () => { + const forms = SCENARIOS.find( s => s.key === 'formsResponses' ); + assert.equal( forms.loadState, 'load' ); + // The scenarios that measure a settled page keep the default (undefined → 'networkidle'). + assert.equal( SCENARIOS.find( s => s.key === 'jetpackConnected' ).loadState, undefined ); + assert.equal( SCENARIOS.find( s => s.key === 'myJetpack' ).loadState, undefined ); +} ); diff --git a/tools/performance/scripts/scenarios.js b/tools/performance/scripts/scenarios.js index b504fdc632aa..b8da47e5a518 100644 --- a/tools/performance/scripts/scenarios.js +++ b/tools/performance/scripts/scenarios.js @@ -9,7 +9,8 @@ * To add a new scenario: * 1. Add an entry to the SCENARIOS array below. * 2. To measure another PAGE on an existing WordPress instance, reuse that instance's - * dockerService/wpPath/envVar/defaultUrl and set `path` + `waitForSelector` (see formsResponses); + * dockerService/wpPath/envVar/defaultUrl and set `path` + `waitForSelector` (see formsResponses), + * plus the optional `expectUrlIncludes`, `minResourceCount` and `loadState` guards; * no new Docker service or setup is needed. * 3. Only when introducing a NEW WordPress instance, add the Docker service in * docker/docker-compose.yml and its setup in docker/setup-wordpress.sh. @@ -90,13 +91,43 @@ export const SCENARIOS = [ // would populate the `forms-responses-*` keys from the wrong page. `expectUrlIncludes` makes // measure-lcp.js fail the run if a future redirect change moves us off the inbox. path: '/wp-admin/admin.php?page=jetpack-forms-responses-wp-admin&p=%2Fresponses%2Finbox', - waitForSelector: '#jetpack-forms-responses-wp-admin-app.boot-layout-container', + // Wait for the app's rendered layout, NOT the mount point. The wp-build `boot` shell + // (rebuilt in Automattic/jetpack#49272) renders the whole dashboard inside a + // `position: absolute` `.boot-layout` element nested under `display: contents` wrappers, + // so the outer `#jetpack-forms-responses-wp-admin-app.boot-layout-container` mount point + // now computes to height 0. measure-lcp.js waits for the selector to be *visible*, and a + // 0-height element is never visible, so waiting on the container timed out every iteration + // even though the dashboard rendered fine ("locator resolved to hidden"). `.boot-layout` is + // the positioned surface that actually fills the viewport (a stable, non-hashed BEM class + // from the wp-build boot framework), so it reflects the rendered page. Scoped by the app id + // so it can only match this dashboard's layout. See FORMS-729. + waitForSelector: '#jetpack-forms-responses-wp-admin-app .boot-layout', expectUrlIncludes: '/responses/inbox', - // A healthy load of this page fetches ~80 resources; measure-lcp.js fails the run if it - // captures fewer than this, so a truncated/partial capture can't post an in-range but - // undercounted decodedBytesKB. Kept well below the real count (2x margin) and count-based, - // not editor-asset-based, so it never clips the legitimate drop when the editor lazy-loads. - minResourceCount: 40, + // Don't gate the measurement on `networkidle`. The wp-build dashboard framework fires a + // `canUser` OPTIONS probe to `/wp/v2/settings` during boot; in the headless-Chromium Docker + // fixture that request's response is intermittently not delivered to the browser (the server + // answers in ~0.02s and the request completes for curl/isolated fetches — it is a local + // boot-burst delivery stall, confirmed local-only), so `networkidle` never settles and every + // navigation timed out at 60s. `load` + the visible-selector + hydration waits below are a + // deterministic readiness signal that a single perpetually-pending request cannot blackhole; + // completeness for decodedBytesKB is then guarded by an in-flight-aware resource settle in + // measure-lcp.js (networkidle's quiet + nothing-in-flight guarantee minus only that one + // stuck probe, failing the iteration at its deadline) plus the `minResourceCount` floor + // below. Other scenarios keep the default 'networkidle'. See FORMS-729. + loadState: 'load', + // A healthy load of this page fetches ~91 resources (stable across iterations locally); + // measure-lcp.js fails the run if it captures fewer than this floor, so a truncated/partial + // capture can't post an in-range but undercounted decodedBytesKB. Set to ~70% of the + // observed count — the same ratio as myJetpack. Honest scope: the floor only catches a + // capture BELOW 64; a settle during a gap where nothing is in flight and the next wave is + // not yet issued can pass it at 64–90 — the same residual window `networkidle` itself has + // always had (its 500ms quiet can fall in such a gap too). In that window the working + // defense is the settle's ~1s-quiet requirement (double networkidle's 500ms) — the + // decodedBytesKB SANITY range is far too wide to catch an undercount and is NOT a + // backstop for this. Still count-based, not editor-asset-based: + // lazy-loading the editor removes a few large files, not the bulk of the count (see the + // assertCaptureComplete docblock), so this does not clip that legitimate drop. + minResourceCount: 64, // These four post straight to PRODUCTION keys — the `-staging` window in the README // Safeguards is deliberately waived here (owner decision). The substitute for that window is // the SANITY_RANGES + all-or-nothing gate plus manual sign-off before the first live post;