From ef03782a1628a4a1e41eb77427bed4eafa6003a0 Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Tue, 14 Jul 2026 10:40:59 +0100 Subject: [PATCH 1/4] Perf tests: fix formsResponses scenario broken by the wp-build boot layout change (#49272) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The formsResponses CodeVitals scenario has failed every iteration since Automattic/jetpack#49272, freezing the forms performance trend. The rebuilt wp-build `boot` shell now renders the dashboard inside a `position: absolute` `.boot-layout` element under `display: contents` wrappers, so the outer `#jetpack-forms-responses-wp-admin-app.boot-layout-container` mount point — the exact selector measure-lcp.js waited on — computes to height 0. Playwright's visible-state wait never resolves for a 0-height element, so the run timed out ("locator resolved to hidden") even though the dashboard renders correctly. - scenarios.js: wait on the rendered `.boot-layout` surface instead of the 0-height mount point. - measure-lcp.js: add a per-scenario `loadState` (forms uses `load`) so the measurement no longer hangs on the dashboard's canUser OPTIONS probe to /wp/v2/settings, whose response is intermittently not delivered in the headless-Chromium fixture; a single perpetually-pending request can no longer blackhole the scenario. Readiness is carried by the visible selector + hydration + a resource-count settle that a stuck request cannot stall. - Tests for the resilient settle and the forms readiness config. Verified locally (3 iterations, --skip-codevitals): formsResponses green (LCP 252 / TTFB 46 / FCP 72 / decodedBytesKB 6574, 91 resources — all within SANITY_RANGES); jetpackConnected and myJetpack unregressed. --- tools/performance/scripts/measure-lcp.js | 73 +++++++++++++++++-- .../scripts/post-to-codevitals.test.js | 64 ++++++++++++++++ tools/performance/scripts/scenarios.js | 23 +++++- 3 files changed, 153 insertions(+), 7 deletions(-) diff --git a/tools/performance/scripts/measure-lcp.js b/tools/performance/scripts/measure-lcp.js index 6d3c00d24dfd..7487f1a1dc62 100644 --- a/tools/performance/scripts/measure-lcp.js +++ b/tools/performance/scripts/measure-lcp.js @@ -62,6 +62,13 @@ 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'; console.log( `Measuring LCP for ${ url }${ targetPath || '' } (${ iterations } iterations)...` ); @@ -118,7 +125,7 @@ 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 } ); @@ -155,7 +162,7 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { // Step 3: 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 +185,21 @@ 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 ( navWaitUntil === 'networkidle' ) { + // 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 instead. A never-delivered request never adds a resource-timing entry, + // so a stuck request can't stall this, while genuine late resources (lazy editor + // modules) still push the count up and extend the wait. This keeps decodedBytesKB + // capture complete without depending on network quiescence. + await waitForResourceCountIdle( page ); + } // Additional short wait for any final rendering after network settles await page.waitForTimeout( 500 ); @@ -518,6 +536,48 @@ 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. + * + * @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] - Hard cap so this can never hang. + * @return {Promise} + */ +async function waitForResourceCountIdle( page, opts = {} ) { + const { intervalMs = 200, stableChecks = 5, maxWaitMs = 20000 } = 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 ); + + if ( count === last ) { + stable += 1; + if ( stable >= stableChecks ) { + return; + } + } else { + stable = 0; + last = count; + } + await page.waitForTimeout( intervalMs ); + } +} + /** * 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 @@ -837,4 +897,5 @@ export { 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..5b50886e00d7 100644 --- a/tools/performance/scripts/post-to-codevitals.test.js +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -23,6 +23,7 @@ import { assertCaptureComplete, assertExpectedUrl, summarizeResources, + waitForResourceCountIdle, } from './measure-lcp.js'; import { checkSanityRange, @@ -2569,3 +2570,66 @@ 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. + */ +function fakeResourcePage( counts ) { + let i = 0; + return { + evaluate: async () => counts[ Math.min( i++, counts.length - 1 ) ], + waitForTimeout: async () => {}, + }; +} + +test( 'waitForResourceCountIdle resolves once the completed-resource count holds steady', async () => { + // Count climbs 10→40→90 then holds; five equal polls (default stableChecks) settle it. + const page = fakeResourcePage( [ 10, 40, 90, 90, 90, 90, 90, 90 ] ); + await waitForResourceCountIdle( page, { intervalMs: 0, stableChecks: 5, maxWaitMs: 5000 } ); + // Resolving (not throwing/hanging) is the assertion. + assert.ok( true ); +} ); + +test( 'waitForResourceCountIdle caps at maxWaitMs instead of hanging when the count never settles', 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. A never-delivered + // request is the real 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 () => {}, + }; + await waitForResourceCountIdle( page, { intervalMs: 0, stableChecks: 5, maxWaitMs: 50 } ); + assert.ok( polls > 0, 'it should have polled at least once' ); +} ); + +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' + ); + assert.ok( + ! /\.boot-layout-container/.test( forms.waitForSelector ), + 'forms selector must not wait on the 0-height boot-layout-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..f8c25b2a5279 100644 --- a/tools/performance/scripts/scenarios.js +++ b/tools/performance/scripts/scenarios.js @@ -90,8 +90,29 @@ 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', + // 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 `minResourceCount` and a resource-count + // settle in measure-lcp.js rather than by network quiescence. Other scenarios keep the + // default 'networkidle'. See FORMS-729. + loadState: 'load', // 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, From 5211e858bbec44b9e641ac957d80434a53fd8bae Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Tue, 14 Jul 2026 13:26:18 +0100 Subject: [PATCH 2/4] Perf tests: harden the formsResponses settle against silent undercounts Review follow-up for FORMS-729. The resource-count settle introduced for the formsResponses scenario could cap out (or settle during a mid-load plateau) and still capture the page, posting an undercounted decodedBytesKB to the append-only CodeVitals store past a loose minResourceCount floor. - waitForResourceCountIdle now reports settled-vs-capped, and measureLCP fails the iteration (fail closed) when the count was still changing at the deadline, at both settle sites. - The initial navigation also settles before the measured reload on the non-networkidle path, so the reload measures a warm cache like the networkidle scenarios always did. - formsResponses minResourceCount raised 40 -> 64 (~70% of the observed 91-resource load, matching myJetpack), with an exact-pin + boundary test. - Settle unit tests now assert the stability algorithm (poll counts, streak reset, cap reporting) instead of resolve-only. - Docs: README forms note corrected (the /wp/v2/settings stall is a local browser-side delivery stall, not a server hang), and loadState is now documented in the measureLCP docblock and the scenarios guide. --- tools/performance/README.md | 4 +- tools/performance/scripts/measure-lcp.js | 54 +++++++++-- .../scripts/post-to-codevitals.test.js | 93 ++++++++++++++----- tools/performance/scripts/scenarios.js | 17 ++-- 4 files changed, 132 insertions(+), 36 deletions(-) diff --git a/tools/performance/README.md b/tools/performance/README.md index 229b2dc3de16..ff8e1e00c459 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 a completed-resource-count settle that fails the iteration (fail closed) if the count is still changing at its deadline; capture completeness is guarded by `minResourceCount` + the SANITY_RANGES gate. 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 diff --git a/tools/performance/scripts/measure-lcp.js b/tools/performance/scripts/measure-lcp.js index 7487f1a1dc62..aca7d27169bf 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 = {} ) { @@ -129,6 +129,18 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { timeout: 60000, } ); await page.waitForSelector( pageReadySelector, { timeout: 30000 } ); + if ( navWaitUntil !== 'networkidle' ) { + // 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. + assertResourceCountSettled( + await waitForResourceCountIdle( page ), + 'warm-up navigation' + ); + } } // Step 2: Set up LCP capture using addInitScript @@ -196,9 +208,10 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { // navWaitUntil): `networkidle` never fires, so settle on the completed-resource count // going quiet instead. A never-delivered request never adds a resource-timing entry, // so a stuck request can't stall this, while genuine late resources (lazy editor - // modules) still push the count up and extend the wait. This keeps decodedBytesKB - // capture complete without depending on network quiescence. - await waitForResourceCountIdle( page ); + // modules) still push the count up and extend the wait. If the count is still + // changing when the cap expires, fail the iteration (fail closed) rather than + // capture a still-loading page's bundle size. + assertResourceCountSettled( await waitForResourceCountIdle( page ), 'measured reload' ); } // Additional short wait for any final rendering after network settles @@ -550,12 +563,16 @@ function summarizeResources( resources ) { * 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. * + * 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 — 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] - Hard cap so this can never hang. - * @return {Promise} + * @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). + * @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 } = opts; @@ -568,7 +585,7 @@ async function waitForResourceCountIdle( page, opts = {} ) { if ( count === last ) { stable += 1; if ( stable >= stableChecks ) { - return; + return { settled: true, count }; } } else { stable = 0; @@ -576,6 +593,28 @@ async function waitForResourceCountIdle( page, opts = {} ) { } await page.waitForTimeout( intervalMs ); } + return { settled: false, count: last }; +} + +/** + * 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 ) { + throw new Error( + `Resource count never settled during ${ phase }: still changing at ${ result.count } resources when the deadline expired — failing this iteration rather than measuring a still-loading page` + ); + } } /** @@ -894,6 +933,7 @@ export { finalizeMeasurement, assertCaptureComplete, assertExpectedUrl, + assertResourceCountSettled, summarizeResources, resolveScenarioSet, computeRunOutcome, diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js index 5b50886e00d7..6a0f2f8c996c 100644 --- a/tools/performance/scripts/post-to-codevitals.test.js +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -22,6 +22,7 @@ import { resolveResultsGit, assertCaptureComplete, assertExpectedUrl, + assertResourceCountSettled, summarizeResources, waitForResourceCountIdle, } from './measure-lcp.js'; @@ -508,13 +509,17 @@ 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. The floor matters + // more here than on myJetpack: this scenario settles on the resource count instead of + // networkidle, so the floor is what catches a settle that fired early in a mid-load plateau. + 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', () => { @@ -2577,36 +2582,84 @@ test( 'a dry run makes no dedup read even when dedup is fully configured', async * 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. + * 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 resolves once the completed-resource count holds steady', async () => { - // Count climbs 10→40→90 then holds; five equal polls (default stableChecks) settle it. - const page = fakeResourcePage( [ 10, 40, 90, 90, 90, 90, 90, 90 ] ); - await waitForResourceCountIdle( page, { intervalMs: 0, stableChecks: 5, maxWaitMs: 5000 } ); - // Resolving (not throwing/hanging) is the assertion. - assert.ok( true ); +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 instead of hanging when the count never settles', async () => { +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. A never-delivered - // request is the real case: it never adds a resource entry, so the count would instead - // go flat — this test pins the harder "never flat" bound. + // 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 () => {}, }; - await waitForResourceCountIdle( page, { intervalMs: 0, stableChecks: 5, maxWaitMs: 50 } ); + 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. + assert.throws( + () => assertResourceCountSettled( { settled: false, count: 55 }, 'measured reload' ), + /never settled during measured reload.*55 resources/ + ); + assert.doesNotThrow( () => + assertResourceCountSettled( { settled: true, count: 91 }, 'measured reload' ) + ); } ); test( 'formsResponses waits on the visible layout, not the 0-height mount point', () => { @@ -2620,10 +2673,6 @@ test( 'formsResponses waits on the visible layout, not the 0-height mount point' '#jetpack-forms-responses-wp-admin-app .boot-layout', 'forms selector must target the rendered layout, not the 0-height container' ); - assert.ok( - ! /\.boot-layout-container/.test( forms.waitForSelector ), - 'forms selector must not wait on the 0-height boot-layout-container' - ); } ); test( 'formsResponses opts out of networkidle so a stuck request cannot blackhole it', () => { diff --git a/tools/performance/scripts/scenarios.js b/tools/performance/scripts/scenarios.js index f8c25b2a5279..0f7b00a99aa3 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. @@ -113,11 +114,15 @@ export const SCENARIOS = [ // settle in measure-lcp.js rather than by network quiescence. Other scenarios keep the // default 'networkidle'. See FORMS-729. loadState: 'load', - // 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, + // 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 — because this scenario settles on the + // resource count rather than networkidle, so the floor is the guard against an early + // settle, not just gross truncation. 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; From bd508c7a8ae14fd318314670f50b1f430924b9a1 Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Tue, 14 Jul 2026 14:24:14 +0100 Subject: [PATCH 3/4] Perf tests: make the forms settle in-flight-aware (networkidle parity) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up for FORMS-729. Both independent review passes converged on the same finding, one with a live repro: a slow legitimate response is invisible to the completed-resource count (a Resource Timing entry appears only at responseEnd), so the settle could report quiet and capture before that response landed, posting an undercounted decodedBytesKB. - New trackPendingRequests ledger: counts requests from issue to finished/failed, excluding only the known-stuck /wp/v2/settings OPTIONS probe (isStuckSettingsProbe). waitForResourceCountIdle now also requires zero relevant in-flight requests on every stable poll — networkidle's quiet + nothing-in-flight guarantee minus the one request that breaks it. Any OTHER stuck request now holds the settle open and fails the iteration at the deadline instead of being silently ignored. - Install the LCP/resource-timing init script before the first navigation so the warm-up settle reads an uncapped Resource Timing buffer (the browser's 250-entry default would silently cap the count once the load grows). - Comments and docs stop overclaiming: the minResourceCount floor only catches captures below its value; the residual quiet-gap window (shared with networkidle itself) is documented and pinned by a dedicated test. - README: the Capture-guards floor now defers to scenarios.js instead of restating numbers that had already drifted (still said 40/~80). - New tests: in-flight request holds the settle open, a never-clearing request fails closed, the isStuckSettingsProbe match matrix, ledger add/remove/exclude/dispose, and the accepted-residual documenting test. --- tools/performance/README.md | 4 +- tools/performance/scripts/measure-lcp.js | 171 +++++++++++++----- .../scripts/post-to-codevitals.test.js | 144 ++++++++++++++- tools/performance/scripts/scenarios.js | 17 +- 4 files changed, 283 insertions(+), 53 deletions(-) diff --git a/tools/performance/README.md b/tools/performance/README.md index ff8e1e00c459..dc4e38c4e357 100644 --- a/tools/performance/README.md +++ b/tools/performance/README.md @@ -43,7 +43,7 @@ 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 a completed-resource-count settle that fails the iteration (fail closed) if the count is still changing at its deadline; capture completeness is guarded by `minResourceCount` + the SANITY_RANGES gate. See the comments on the scenario in `scenarios.js` for the full mechanics. +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. Capture completeness is additionally guarded by `minResourceCount` + the SANITY_RANGES gate; the settle proves quiescence, and shares `networkidle`'s inherent blind spot for a gap before the page issues its next resource wave. See the comments on the scenario in `scenarios.js` for the full mechanics. | CodeVitals key | Field | Type | Description | | ------------------------------------------------------- | ---------------- | ---------------- | --------------------------------------------------------- | @@ -150,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 aca7d27169bf..ee0cbc1f04d5 100644 --- a/tools/performance/scripts/measure-lcp.js +++ b/tools/performance/scripts/measure-lcp.js @@ -98,9 +98,51 @@ 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 = + navWaitUntil !== 'networkidle' ? 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', @@ -137,42 +179,13 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { // partially cold cache. Settle on the resource count here so the reload below // measures the same warmed page networkidle used to guarantee. assertResourceCountSettled( - await waitForResourceCountIdle( page ), + await waitForResourceCountIdle( page, { pendingCount: pendingRequests.count } ), '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: navWaitUntil, timeout: 60000 } ); @@ -206,12 +219,16 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { } 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 instead. A never-delivered request never adds a resource-timing entry, - // so a stuck request can't stall this, while genuine late resources (lazy editor - // modules) still push the count up and extend the wait. If the count is still - // changing when the cap expires, fail the iteration (fail closed) rather than - // capture a still-loading page's bundle size. - assertResourceCountSettled( await waitForResourceCountIdle( page ), 'measured reload' ); + // 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. + assertResourceCountSettled( + await waitForResourceCountIdle( page, { pendingCount: pendingRequests.count } ), + 'measured reload' + ); } // Additional short wait for any final rendering after network settles @@ -334,6 +351,7 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { timestamp: new Date().toISOString(), } ); } finally { + pendingRequests?.dispose(); await browser.close(); } @@ -563,26 +581,38 @@ function summarizeResources( resources ) { * 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 — an incomplete capture the - * caller must treat as a failed iteration (see assertResourceCountSettled), never measure. + * `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 } = 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 ) { + if ( count === last && ! busy ) { stable += 1; if ( stable >= stableChecks ) { return { settled: true, count }; @@ -596,6 +626,63 @@ async function waitForResourceCountIdle( page, opts = {} ) { 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 url = request.url(); + try { + url = decodeURIComponent( url ); + } catch { + // Malformed escape sequence — match against the raw URL instead. + } + return url.includes( '/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 @@ -934,6 +1021,8 @@ export { assertCaptureComplete, assertExpectedUrl, assertResourceCountSettled, + isStuckSettingsProbe, + trackPendingRequests, summarizeResources, resolveScenarioSet, computeRunOutcome, diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js index 6a0f2f8c996c..5070894a90ae 100644 --- a/tools/performance/scripts/post-to-codevitals.test.js +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -23,6 +23,8 @@ import { assertCaptureComplete, assertExpectedUrl, assertResourceCountSettled, + isStuckSettingsProbe, + trackPendingRequests, summarizeResources, waitForResourceCountIdle, } from './measure-lcp.js'; @@ -511,9 +513,10 @@ test( 'the formsResponses scenario posts LCP, TTFB, FCP and decodedBytes to prod // A resource-count floor so a partial capture can't post an undercounted decodedBytesKB. // 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. The floor matters - // more here than on myJetpack: this scenario settles on the resource count instead of - // networkidle, so the floor is what catches a settle that fired early in a mid-load plateau. + // 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 ), @@ -2662,6 +2665,141 @@ test( 'assertResourceCountSettled fails a capped-out settle and passes a clean o ); } ); +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. + // The backstops are minResourceCount (catches < 64) and SANITY_RANGES. 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. + assert.equal( + isStuckSettingsProbe( + req( 'OPTIONS', 'http://localhost:8083/index.php?rest_route=%2Fwp%2Fv2%2Fsettings' ) + ), + true + ); + // Pretty-permalink shape. + 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 + ); + // A malformed escape sequence must not throw; it falls back to matching the raw URL. + 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' ); diff --git a/tools/performance/scripts/scenarios.js b/tools/performance/scripts/scenarios.js index 0f7b00a99aa3..5a5bd46275cb 100644 --- a/tools/performance/scripts/scenarios.js +++ b/tools/performance/scripts/scenarios.js @@ -110,17 +110,20 @@ export const SCENARIOS = [ // 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 `minResourceCount` and a resource-count - // settle in measure-lcp.js rather than by network quiescence. Other scenarios keep the - // default 'networkidle'. See FORMS-729. + // 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 — because this scenario settles on the - // resource count rather than networkidle, so the floor is the guard against an early - // settle, not just gross truncation. Still count-based, not editor-asset-based: lazy-loading - // the editor removes a few large files, not the bulk of the count (see 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), narrowed here by the in-flight + // ledger and backstopped by SANITY_RANGES. 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 From 346f2b561516d5efe2ec2f06476764bdcdbe28b1 Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Tue, 14 Jul 2026 15:37:20 +0100 Subject: [PATCH 4/4] Perf tests: exact-match the stuck-probe exclusion and pin the ledger seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review follow-up for FORMS-729 (both review passes; the over-match was empirically driven through the settle with a held-open near-miss route). - isStuckSettingsProbe now compares the decoded REST route exactly (rest_route query value or /wp-json pathname, modulo a trailing slash) instead of a substring match, so adjacent routes (/wp/v2/settings/child, /wp/v2/settings-extra) and URLs merely carrying the string in a query value stay in the ledger and fail the iteration if they get stuck — matching the documented 'only that one probe' contract. Near-miss cases added to the matcher test. - New settleOrThrow seam used by both readiness sites, with a test pinning that the in-flight ledger is threaded into the settle — the exact one-line mutation the review proved could silently revert the in-flight guarantee with the whole suite green now fails a test. The settle-path predicate is bound once (useResourceSettle) instead of re-derived at three sites. - The fail-closed error message no longer claims the count was 'still changing' when a stuck in-flight request may be the actual holdout (settled:false cannot distinguish the two signals). - Comments/README stop calling SANITY_RANGES a backstop for the quiet-gap residual (the range is far too wide to catch an undercount); the settle's ~1s-quiet requirement is named as the working defense. --- tools/performance/README.md | 2 +- tools/performance/scripts/measure-lcp.js | 70 ++++++++++---- .../scripts/post-to-codevitals.test.js | 96 +++++++++++++++++-- tools/performance/scripts/scenarios.js | 6 +- 4 files changed, 145 insertions(+), 29 deletions(-) diff --git a/tools/performance/README.md b/tools/performance/README.md index dc4e38c4e357..576b4795edd5 100644 --- a/tools/performance/README.md +++ b/tools/performance/README.md @@ -43,7 +43,7 @@ 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. Capture completeness is additionally guarded by `minResourceCount` + the SANITY_RANGES gate; the settle proves quiescence, and shares `networkidle`'s inherent blind spot for a gap before the page issues its next resource wave. See the comments on the scenario in `scenarios.js` for the full mechanics. +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 | | ------------------------------------------------------- | ---------------- | ---------------- | --------------------------------------------------------- | diff --git a/tools/performance/scripts/measure-lcp.js b/tools/performance/scripts/measure-lcp.js index ee0cbc1f04d5..f60b4b246e25 100644 --- a/tools/performance/scripts/measure-lcp.js +++ b/tools/performance/scripts/measure-lcp.js @@ -69,6 +69,9 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { // 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)...` ); @@ -103,8 +106,9 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { // 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 = - navWaitUntil !== 'networkidle' ? trackPendingRequests( page, isStuckSettingsProbe ) : null; + const pendingRequests = useResourceSettle + ? trackPendingRequests( page, isStuckSettingsProbe ) + : null; try { console.log( ` Iteration ${ i + 1 }/${ iterations }...` ); @@ -171,17 +175,14 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { timeout: 60000, } ); await page.waitForSelector( pageReadySelector, { timeout: 30000 } ); - if ( navWaitUntil !== 'networkidle' ) { + 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. - assertResourceCountSettled( - await waitForResourceCountIdle( page, { pendingCount: pendingRequests.count } ), - 'warm-up navigation' - ); + await settleOrThrow( page, pendingRequests, 'warm-up navigation' ); } } @@ -212,7 +213,7 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { // Wait for the resource payload to finish loading and LCP to finalize (LCP stops // updating after user input or visibility change). - if ( navWaitUntil === 'networkidle' ) { + 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 } ); @@ -225,10 +226,7 @@ async function measureLCP( url, username, password, iterations = 5, scenario = { // 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. - assertResourceCountSettled( - await waitForResourceCountIdle( page, { pendingCount: pendingRequests.count } ), - 'measured reload' - ); + await settleOrThrow( page, pendingRequests, 'measured reload' ); } // Additional short wait for any final rendering after network settles @@ -642,13 +640,22 @@ function isStuckSettingsProbe( request ) { if ( request.method() !== 'OPTIONS' ) { return false; } - let url = request.url(); + let parsed; try { - url = decodeURIComponent( url ); + parsed = new URL( request.url() ); } catch { - // Malformed escape sequence — match against the raw URL instead. + // Not an absolute URL — cannot be the probe; leave it in the ledger (fail closed). + return false; } - return url.includes( '/wp/v2/settings' ); + // 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'; } /** @@ -698,12 +705,40 @@ function trackPendingRequests( page, isExcluded ) { */ 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( - `Resource count never settled during ${ phase }: still changing at ${ result.count } resources when the deadline expired — failing this iteration rather than measuring a still-loading page` + `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 @@ -1022,6 +1057,7 @@ export { assertExpectedUrl, assertResourceCountSettled, isStuckSettingsProbe, + settleOrThrow, trackPendingRequests, summarizeResources, resolveScenarioSet, diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js index 5070894a90ae..d395c2f9304d 100644 --- a/tools/performance/scripts/post-to-codevitals.test.js +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -24,6 +24,7 @@ import { assertExpectedUrl, assertResourceCountSettled, isStuckSettingsProbe, + settleOrThrow, trackPendingRequests, summarizeResources, waitForResourceCountIdle, @@ -2655,16 +2656,54 @@ test( 'waitForResourceCountIdle caps at maxWaitMs and reports it did NOT settle' 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. + // 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.*55 resources/ + /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 @@ -2716,9 +2755,11 @@ test( 'documents the accepted residual: a quiet-gap settle at or above the floor // 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. - // The backstops are minResourceCount (catches < 64) and SANITY_RANGES. If this test starts - // failing, the settle got stricter and this documentation should be updated, not the settle - // loosened. + // 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, @@ -2737,18 +2778,25 @@ test( 'documents the accepted residual: a quiet-gap settle at or above the floor 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. + // 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' ) + req( + 'OPTIONS', + 'http://localhost:8083/index.php?rest_route=%2Fwp%2Fv2%2Fsettings&_locale=user' + ) ), true ); - // Pretty-permalink shape. + // 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' ) ), @@ -2761,7 +2809,37 @@ test( 'isStuckSettingsProbe matches only the settings OPTIONS probe', () => { ), false ); - // A malformed escape sequence must not throw; it falls back to matching the raw URL. + // 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 ); } ); diff --git a/tools/performance/scripts/scenarios.js b/tools/performance/scripts/scenarios.js index 5a5bd46275cb..b8da47e5a518 100644 --- a/tools/performance/scripts/scenarios.js +++ b/tools/performance/scripts/scenarios.js @@ -121,8 +121,10 @@ export const SCENARIOS = [ // 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), narrowed here by the in-flight - // ledger and backstopped by SANITY_RANGES. Still count-based, not editor-asset-based: + // 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,