From f90f09df614ad57f84b707b5effcc246742186d2 Mon Sep 17 00:00:00 2001 From: Christian Gastrell Date: Mon, 21 Sep 2026 18:02:43 -0300 Subject: [PATCH 1/7] Script the wp-build port verification check's geometry and network diffs JETPACK-2685 splits the scriptable half of the port verification rule (JETPACK-2573) out of its manual checklist: step 2 (computed styles and geometry) and step 3 (the network panel), compared with a port's feature flag off versus on. Adds tools/port-verification, a standalone CLI (`verify-port run|capture| diff`) built on Playwright. `run` captures a page with the flag off, pauses for the reviewer to flip the flag on the site, captures again, and prints a Markdown report to paste into the PR. The diff and report logic (src/diff.js, src/report.js) is pure and unit-tested against fixtures here; the Playwright capture half (src/capture.js) needs a live site and is documented as unverified in this sandbox (no GPU). Co-Authored-By: Claude Sonnet 5 --- pnpm-lock.yaml | 6 + pnpm-workspace.yaml | 1 + tools/port-verification/.gitignore | 3 + tools/port-verification/README.md | 143 ++++++++++++++++ tools/port-verification/bin/verify-port.js | 188 +++++++++++++++++++++ tools/port-verification/eslint.config.mjs | 15 ++ tools/port-verification/package.json | 22 +++ tools/port-verification/src/capture.js | 168 ++++++++++++++++++ tools/port-verification/src/diff.js | 180 ++++++++++++++++++++ tools/port-verification/src/diff.test.js | 157 +++++++++++++++++ tools/port-verification/src/fixtures.js | 112 ++++++++++++ tools/port-verification/src/report.js | 112 ++++++++++++ tools/port-verification/src/report.test.js | 88 ++++++++++ tools/port-verification/src/selectors.js | 23 +++ 14 files changed, 1218 insertions(+) create mode 100644 tools/port-verification/.gitignore create mode 100644 tools/port-verification/README.md create mode 100644 tools/port-verification/bin/verify-port.js create mode 100644 tools/port-verification/eslint.config.mjs create mode 100644 tools/port-verification/package.json create mode 100644 tools/port-verification/src/capture.js create mode 100644 tools/port-verification/src/diff.js create mode 100644 tools/port-verification/src/diff.test.js create mode 100644 tools/port-verification/src/fixtures.js create mode 100644 tools/port-verification/src/report.js create mode 100644 tools/port-verification/src/report.test.js create mode 100644 tools/port-verification/src/selectors.js diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b8d043aee4ca..8ec895f35948 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -7172,6 +7172,12 @@ importers: specifier: ^1.48.0 version: 1.60.0 + tools/port-verification: + dependencies: + playwright: + specifier: ^1.48.0 + version: 1.60.0 + packages: '@-xun/debug@2.0.2': diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index dc8f3b5f2a9d..3e1bda3886df 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -7,6 +7,7 @@ packages: - tools/e2e-commons/no-lighthouse - tools/js-tools - tools/performance + - tools/port-verification - .github/files/coverage-munger engineStrict: true diff --git a/tools/port-verification/.gitignore b/tools/port-verification/.gitignore new file mode 100644 index 000000000000..cdcc4192deb4 --- /dev/null +++ b/tools/port-verification/.gitignore @@ -0,0 +1,3 @@ +node_modules/ +*.snapshot.json +*-report.md diff --git a/tools/port-verification/README.md b/tools/port-verification/README.md new file mode 100644 index 000000000000..d788e3527af6 --- /dev/null +++ b/tools/port-verification/README.md @@ -0,0 +1,143 @@ +# Port verification check (JETPACK-2685) + +Scripts steps 2 and 3 of the wp-build port verification rule defined in +[JETPACK-2573](https://linear.app/a8c/issue/JETPACK-2573): computed styles/geometry, and the +network panel, compared with the port's feature flag off versus on. Steps 1, 4, 5, 6 and 7 of +that rule stay manual -- see "Not covered" below. + +## What it checks + +- **Step 2, geometry.** `#wpwrap` (page root), `#wpbody-content`, `#wpadminbar` (header), + `#wpfooter`, plus `font-family` and one control's full box model (margin/border/padding, + `box-sizing`, `font-size`). These are core wp-admin markup present whether the flag is off + or on, so a diff catches the port shifting the frame around its content -- the failure mode + JETPACK-2573 calls out: "a 4px shift is invisible in a screenshot and obvious in a number." +- **Step 3, network.** Every request the page fires, matched flag-off to flag-on by method + + path (nonces and cache-busting query params are ignored). Reports requests that only fired + one time, and requests whose status code changed. This is what caught a `design-tokens.css` + 404 and a renamed JITM message path in the My Jetpack pilot -- both zero-pixel changes. + +The one difference JETPACK-2573 accepts is a uniform 8px inset from boot's stage gutter on the +page root. The report says so in its footer; it does not try to auto-approve that one row. + +## Setup + +``` +cd tools/port-verification +pnpm install +pnpm exec playwright install chromium +``` + +## Usage + +One command, run against a live site (a Jurassic Ninja site is the normal target): + +``` +node bin/verify-port.js run \ + --url https://.jurassic.ninja/wp-admin/admin.php?page= \ + --flag \ + --user admin --pass \ + --control-selector '.components-button' \ + > port-verification-report.md +``` + +It captures the page with the flag off, then pauses: + +``` +Flip ON on the site now, then press Enter to continue... +``` + +Flip it with whatever the port uses to force the flag on that site -- `wp jetpack feature-flag + on` over SSH, or the `jetpack-feature-flag` skill's JN helper -- then press Enter. The +script captures again with the flag on, diffs both captures, and prints a Markdown report to +paste straight into the PR (also written to `--out ` if given, or redirect stdout as +above). + +### Options + +| Flag | Meaning | +| -------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- | +| `--url` | Page to capture. Required. | +| `--flag` | Feature flag name, printed in the pause prompt and the report header. Optional. | +| `--user` / `--pass` | wp-admin login. Falls back to `WP_ADMIN_USER` / `WP_ADMIN_PASS`. Omit to capture without logging in (e.g. the site is already authenticated). | +| `--control-selector` | CSS selector for the one control to box-model (step 2). Skipped if omitted. | +| `--wait-selector` | Extra selector to wait for after navigation, e.g. the boot mount, so a slow-hydrating page isn't captured mid-render. | +| `--tolerance` | Geometry tolerance in px. Default `0.5` -- rounding noise, not a real shift. | +| `--out` | Write the report to a file (in addition to stdout). | +| `--headed` | Run the browser headed, for watching the capture happen. | + +### Two-step alternative + +If pausing mid-command inside one process is awkward (e.g. flipping the flag needs a separate +terminal), capture and diff separately: + +``` +node bin/verify-port.js capture --url --user admin --pass --out off.json +# flip the flag +node bin/verify-port.js capture --url --user admin --pass --out on.json +node bin/verify-port.js diff --before off.json --after on.json --out report.md +``` + +## Sample output + +``` +## Port verification -- steps 2 & 3 (JETPACK-2685) + +**Page:** `https://example.jurassic.ninja/wp-admin/admin.php?page=jetpack-forms-responses` +**Flag:** `rsm_jetpack_ui_modernization_forms` +**Before (flag off):** captured 2026-09-21T12:00:00.000Z +**After (flag on):** captured 2026-09-21T12:03:00.000Z + +### Step 2 -- computed styles and geometry + +| Element | Status | Details | +| --- | --- | --- | +| Page root (#wpwrap) | CHANGED | x: 0px -> 8px (Δ8.0px)
y: 0px -> 8px (Δ8.0px)
width: 1280px -> 1264px (Δ16.0px)
height: 900px -> 884px (Δ16.0px) | +| #wpbody-content | OK | — | +| Header (#wpadminbar) | OK | — | +| Footer (#wpfooter) | OK | — | +| Control | OK | — | + +### Step 3 -- network panel + +- Only with flag off (0): + - none +- Only with flag on (1): + - `GET https://example.jurassic.ninja/wp-content/plugins/jetpack/design-tokens.css` -> 404 +- Status code changed (0): + - none + +### Summary + +1 geometry finding(s), 1 network finding(s). +The only difference JETPACK-2573 accepts is a uniform 8px inset from boot's stage gutter on the +page root. Anything else above needs a look before merging. +``` + +## Not covered + +Steps 1 (flag off matches trunk), 4 (deep links in a fresh tab), 5 (RTL), 6 (non-default admin +colour scheme) and 7 (delete `build/` and reload) stay manual -- they need a human looking at +the page, not a diff. + +Steps 5 and 6 are the ones most worth automating next: both known post-ship regressions on +already-ported dashboards ([#51963](https://github.com/Automattic/jetpack/pull/51963), +[#52096](https://github.com/Automattic/jetpack/pull/52096)) were in RTL and colour-scheme, not +in anything steps 2 or 3 here would have caught. The geometry side extends cheaply: capture the +same targets with the page in `dir="rtl"` (content column must not clip under the admin menu) +and with a non-default admin colour scheme active (the backdrop must follow the menu colour, +per `class-wp-build-admin-frame.php`), and diff against the LTR / default-scheme capture instead +of against flag-off. The network side has nothing to add for either -- same requests either way +-- so steps 5 and 6 would each need their own geometry-only assertions in `diff.js`, not a new +network check. + +## Testing + +``` +pnpm test +``` + +Runs `src/*.test.js` against the fixtures in `src/fixtures.js` -- the diff logic (`diff.js`) and +report formatting (`report.js`), which need no browser. `capture.js` (the Playwright half) is +not unit-tested: it needs a real Chromium against a real site, which this repo's sandbox can't +run (no GPU). Verify it by hand against a Jurassic Ninja site. diff --git a/tools/port-verification/bin/verify-port.js b/tools/port-verification/bin/verify-port.js new file mode 100644 index 000000000000..07e2dd9316db --- /dev/null +++ b/tools/port-verification/bin/verify-port.js @@ -0,0 +1,188 @@ +#!/usr/bin/env node +/** + * CLI for JETPACK-2685: steps 2 (geometry) and 3 (network) of the wp-build port + * verification check, run against a live site with the port flag off and on. + * + * Usage: see README.md, or `node bin/verify-port.js --help`. + */ + +import fs from 'fs'; +import readline from 'readline/promises'; +import { parseArgs } from 'util'; +import { diffSnapshots } from '../src/diff.js'; +import { formatReport } from '../src/report.js'; +import { DEFAULT_TOLERANCE_PX } from '../src/selectors.js'; + +// Loaded lazily (only by `capture` and `run`) so `--help` and `diff` -- which need no +// browser -- still work without Playwright installed. +async function loadCapturePage() { + return ( await import( '../src/capture.js' ) ).capturePage; +} + +const USAGE = `Usage: + verify-port run --url [--flag ] [--user --pass ] + [--control-selector ] [--wait-selector ] + [--tolerance ] [--out ] + + verify-port capture --url [--user --pass ] + [--control-selector ] [--wait-selector ] + --out + + verify-port diff --before --after + [--tolerance ] [--out ] + +Credentials also read from WP_ADMIN_USER / WP_ADMIN_PASS. +See README.md for the full walkthrough, including how to flip the flag between captures.`; + +const OPTION_SPEC = { + url: { type: 'string' }, + flag: { type: 'string' }, + user: { type: 'string' }, + pass: { type: 'string' }, + 'control-selector': { type: 'string' }, + 'wait-selector': { type: 'string' }, + tolerance: { type: 'string' }, + out: { type: 'string' }, + before: { type: 'string' }, + after: { type: 'string' }, + headed: { type: 'boolean', default: false }, + help: { type: 'boolean', default: false }, +}; + +/** + * @param {string[]} argv - Everything after the subcommand. + * @return {object} Parsed options, camel-cased from `OPTION_SPEC`'s kebab-case keys. + */ +function parseOptions( argv ) { + const { values } = parseArgs( { args: argv, options: OPTION_SPEC, allowPositionals: false } ); + return { + url: values.url, + flag: values.flag, + username: values.user ?? process.env.WP_ADMIN_USER, + password: values.pass ?? process.env.WP_ADMIN_PASS, + controlSelector: values[ 'control-selector' ], + waitForSelector: values[ 'wait-selector' ], + tolerancePx: values.tolerance ? Number( values.tolerance ) : DEFAULT_TOLERANCE_PX, + out: values.out, + before: values.before, + after: values.after, + headless: ! values.headed, + help: values.help, + }; +} + +/** + * @param {string} path + * @param {object} data + * @return {void} + */ +function writeJson( path, data ) { + fs.writeFileSync( path, JSON.stringify( data, null, 2 ) + '\n' ); +} + +/** + * @param {object} options - From `parseOptions()`. + * @return {Promise} + */ +async function runCapture( options ) { + if ( ! options.url || ! options.out ) { + throw new Error( '--url and --out are required for `capture`.' ); + } + const capturePage = await loadCapturePage(); + const snapshot = await capturePage( options ); + writeJson( options.out, snapshot ); + console.log( `Captured ${ options.url } -> ${ options.out }` ); +} + +/** + * @param {object} options - From `parseOptions()`. + * @return {void} + */ +function runDiff( options ) { + if ( ! options.before || ! options.after ) { + throw new Error( '--before and --after are required for `diff`.' ); + } + const before = JSON.parse( fs.readFileSync( options.before, 'utf8' ) ); + const after = JSON.parse( fs.readFileSync( options.after, 'utf8' ) ); + printReport( before, after, options ); +} + +/** + * @param {object} before - Flag-off snapshot. + * @param {object} after - Flag-on snapshot. + * @param {object} options - From `parseOptions()`. + * @return {void} + */ +function printReport( before, after, options ) { + const diffResult = diffSnapshots( before, after, options ); + const report = formatReport( diffResult, { + url: after.meta?.url ?? before.meta?.url ?? options.url, + flag: options.flag, + beforeCapturedAt: before.meta?.capturedAt, + afterCapturedAt: after.meta?.capturedAt, + } ); + if ( options.out ) { + fs.writeFileSync( options.out, report + '\n' ); + console.log( `Report written to ${ options.out }` ); + } + console.log( '' ); + console.log( report ); +} + +/** + * Capture flag-off, pause for the reviewer to flip the flag on the site, capture + * flag-on, then diff and print. The one command JETPACK-2685 asks for. + * + * @param {object} options - From `parseOptions()`. + * @return {Promise} + */ +async function runFull( options ) { + if ( ! options.url ) { + throw new Error( '--url is required for `run`.' ); + } + const capturePage = await loadCapturePage(); + + console.log( `Capturing with the flag OFF: ${ options.url }` ); + const before = await capturePage( options ); + + const rl = readline.createInterface( { input: process.stdin, output: process.stdout } ); + await rl.question( + `\nFlip ${ options.flag ?? 'the port flag' } ON on the site now, then press Enter to continue... ` + ); + rl.close(); + + console.log( `Capturing with the flag ON: ${ options.url }` ); + const after = await capturePage( options ); + + printReport( before, after, options ); +} + +async function main() { + const argv = process.argv.slice( 2 ); + // A leading option (e.g. bare `--help`) has no subcommand; anything else does. + const hasCommand = argv.length > 0 && ! argv[ 0 ].startsWith( '-' ); + const command = hasCommand ? argv[ 0 ] : undefined; + const options = parseOptions( hasCommand ? argv.slice( 1 ) : argv ); + + if ( options.help || ! command ) { + console.log( USAGE ); + process.exit( options.help ? 0 : 1 ); + } + + if ( command === 'capture' ) { + await runCapture( options ); + } else if ( command === 'diff' ) { + runDiff( options ); + } else if ( command === 'run' ) { + await runFull( options ); + } else { + console.error( `Unknown command: ${ command }\n` ); + console.log( USAGE ); + process.exit( 1 ); + } +} + +main().catch( err => { + console.error( err.stack ?? err.message ); + process.exit( 1 ); +} ); diff --git a/tools/port-verification/eslint.config.mjs b/tools/port-verification/eslint.config.mjs new file mode 100644 index 000000000000..b37679111a28 --- /dev/null +++ b/tools/port-verification/eslint.config.mjs @@ -0,0 +1,15 @@ +import { makeBaseConfig, defineConfig } from 'jetpack-js-tools/eslintrc/base.mjs'; + +export default defineConfig( makeBaseConfig( import.meta.url, { envs: [ 'node', 'browser' ] } ), { + rules: { + 'no-console': 'off', + 'n/no-process-exit': 'off', + 'jsdoc/require-jsdoc': 'off', + 'jsdoc/require-param': 'off', + 'jsdoc/require-returns': 'off', + 'jsdoc/require-returns-description': 'off', + 'jsdoc/require-param-description': 'off', + 'jsdoc/require-param-type': 'off', + 'jsdoc/require-description': 'off', + }, +} ); diff --git a/tools/port-verification/package.json b/tools/port-verification/package.json new file mode 100644 index 000000000000..6537102b1ca9 --- /dev/null +++ b/tools/port-verification/package.json @@ -0,0 +1,22 @@ +{ + "name": "@automattic/jetpack-port-verification", + "version": "1.0.0", + "private": true, + "description": "JETPACK-2685: script the geometry and network-panel checks from the wp-build port verification rule (JETPACK-2573)", + "license": "GPL-2.0-or-later", + "author": "Automattic", + "type": "module", + "bin": { + "verify-port": "./bin/verify-port.js" + }, + "scripts": { + "test": "node --test src/*.test.js", + "verify-port": "node bin/verify-port.js" + }, + "dependencies": { + "playwright": "^1.48.0" + }, + "engines": { + "node": ">=20.11.0" + } +} diff --git a/tools/port-verification/src/capture.js b/tools/port-verification/src/capture.js new file mode 100644 index 000000000000..91a83a446b6b --- /dev/null +++ b/tools/port-verification/src/capture.js @@ -0,0 +1,168 @@ +/** + * Browser side of JETPACK-2685 steps 2 and 3: loads a page with Playwright and pulls a + * snapshot of its geometry and network traffic. Needs a real Chromium -- not covered by + * the unit tests in this directory, which exercise diff.js and report.js against fixtures + * instead. See README.md for how a reviewer runs this against a live site. + */ + +import { chromium } from 'playwright'; +import { DEFAULT_GEOMETRY_TARGETS } from './selectors.js'; + +/** + * Log in through wp-login.php. + * + * @param {import('playwright').Page} page + * @param {string} siteUrl + * @param {string} username + * @param {string} password + * @return {Promise} + */ +async function login( page, siteUrl, username, password ) { + const origin = new URL( siteUrl ).origin; + await page.goto( `${ origin }/wp-login.php`, { waitUntil: 'networkidle' } ); + await page.fill( '#user_login', username ); + await page.fill( '#user_pass', password ); + await Promise.all( [ + page.waitForNavigation( { waitUntil: 'networkidle' } ), + page.click( '#wp-submit' ), + ] ); +} + +/** + * Read one target's rect + computed style out of the page. Runs inside the browser via + * `page.evaluate`, so it can only use DOM APIs -- no imports from this module reach it. + * + * @param {object} targetsArg - `{ [key]: { label, selector } }`, control's selector resolved by the caller. + * @return {object} `{ [key]: { label, rect, style } }` for every target whose selector matched. + */ +/* c8 ignore start -- runs inside the browser context; exercised only by a live capture. */ +function extractGeometryInPage( targetsArg ) { + /** + * @param {Element} el + * @return {{x: number, y: number, width: number, height: number}} + */ + function readRect( el ) { + const r = el.getBoundingClientRect(); + return { + x: Math.round( r.x ), + y: Math.round( r.y ), + width: Math.round( r.width ), + height: Math.round( r.height ), + }; + } + + /** + * @param {Element} el + * @param {boolean} isControl - Whether to also read the box-model props (margin/border/padding). + * @return {object} + */ + function readStyle( el, isControl ) { + const cs = window.getComputedStyle( el ); + const style = { fontFamily: cs.fontFamily }; + if ( isControl ) { + Object.assign( style, { + marginTop: parseFloat( cs.marginTop ) || 0, + marginRight: parseFloat( cs.marginRight ) || 0, + marginBottom: parseFloat( cs.marginBottom ) || 0, + marginLeft: parseFloat( cs.marginLeft ) || 0, + paddingTop: parseFloat( cs.paddingTop ) || 0, + paddingRight: parseFloat( cs.paddingRight ) || 0, + paddingBottom: parseFloat( cs.paddingBottom ) || 0, + paddingLeft: parseFloat( cs.paddingLeft ) || 0, + borderTopWidth: parseFloat( cs.borderTopWidth ) || 0, + borderRightWidth: parseFloat( cs.borderRightWidth ) || 0, + borderBottomWidth: parseFloat( cs.borderBottomWidth ) || 0, + borderLeftWidth: parseFloat( cs.borderLeftWidth ) || 0, + boxSizing: cs.boxSizing, + fontSize: cs.fontSize, + } ); + } + return style; + } + + const out = {}; + for ( const [ key, target ] of Object.entries( targetsArg ) ) { + if ( ! target.selector ) { + continue; + } + const el = document.querySelector( target.selector ); + if ( ! el ) { + continue; + } + out[ key ] = { + label: target.label, + rect: readRect( el ), + style: readStyle( el, key === 'control' ), + }; + } + return out; +} +/* c8 ignore stop */ + +/** + * Capture one snapshot: navigate (logging in first if credentials are given), wait for + * the page to settle, then read geometry for `targets` and every response since navigation + * started. + * + * @param {object} options + * @param {string} options.url - Page to capture. + * @param {string} [options.username] - wp-admin username; skips login if omitted. + * @param {string} [options.password] - wp-admin password. + * @param {string} [options.controlSelector] - CSS selector for the one control to box-model. Skipped if omitted. + * @param {string} [options.waitForSelector] - Extra selector to wait for (visible) after navigation, e.g. the boot mount when the flag is on. + * @param {object} [options.targets] - Selector config; defaults to `DEFAULT_GEOMETRY_TARGETS`. + * @param {boolean} [options.headless] - Defaults to true. + * @return {Promise<{meta: object, geometry: object, network: object[]}>} The captured snapshot. + */ +export async function capturePage( options ) { + const { + url, + username, + password, + controlSelector, + waitForSelector, + targets = DEFAULT_GEOMETRY_TARGETS, + headless = true, + } = options; + + const resolvedTargets = controlSelector + ? { ...targets, control: { ...targets.control, selector: controlSelector } } + : targets; + + const browser = await chromium.launch( { headless } ); + try { + const context = await browser.newContext(); + const page = await context.newPage(); + + const network = []; + page.on( 'response', response => { + const request = response.request(); + network.push( { + url: response.url(), + method: request.method(), + status: response.status(), + resourceType: request.resourceType(), + } ); + } ); + + if ( username && password ) { + await login( page, url, username, password ); + } + + await page.goto( url, { waitUntil: 'networkidle' } ); + if ( waitForSelector ) { + await page.waitForSelector( waitForSelector, { state: 'visible' } ); + await page.waitForLoadState( 'networkidle' ); + } + + const geometry = await page.evaluate( extractGeometryInPage, resolvedTargets ); + + return { + meta: { url, capturedAt: new Date().toISOString() }, + geometry, + network, + }; + } finally { + await browser.close(); + } +} diff --git a/tools/port-verification/src/diff.js b/tools/port-verification/src/diff.js new file mode 100644 index 000000000000..f20d383c5767 --- /dev/null +++ b/tools/port-verification/src/diff.js @@ -0,0 +1,180 @@ +/** + * Pure comparison logic for JETPACK-2685 steps 2 and 3. No browser here: everything + * takes and returns plain data, so it can run against fixtures without Playwright. + */ + +import { DEFAULT_IGNORED_QUERY_PARAMS, DEFAULT_TOLERANCE_PX } from './selectors.js'; + +/** + * Compare one geometry target's `rect` (numeric px fields) between before and after. + * + * @param {object} before - `{ [field]: number }`, or undefined if the target was missing. + * @param {object} after - Same shape, from the other capture. + * @param {number} tolerancePx - Deltas at or below this are ignored. + * @return {string[]} One human-readable line per field that moved beyond tolerance. + */ +function diffRect( before, after, tolerancePx ) { + if ( ! before || ! after ) { + return []; + } + const lines = []; + for ( const field of Object.keys( { ...before, ...after } ) ) { + const b = before[ field ]; + const a = after[ field ]; + if ( typeof b !== 'number' || typeof a !== 'number' ) { + continue; + } + const delta = Math.abs( a - b ); + if ( delta > tolerancePx ) { + lines.push( `${ field }: ${ b }px -> ${ a }px (Δ${ delta.toFixed( 1 ) }px)` ); + } + } + return lines; +} + +/** + * Compare one geometry target's `style` map (computed style / box-model values) between + * before and after. Numeric values (e.g. paddingTop in px) use `tolerancePx`; anything + * else (e.g. fontFamily, boxSizing) is compared as an exact string. + * + * @param {object} before - `{ [prop]: number|string }`, or undefined. + * @param {object} after - Same shape, from the other capture. + * @param {number} tolerancePx - Numeric-field tolerance. + * @return {string[]} One line per prop that differs. + */ +function diffStyle( before, after, tolerancePx ) { + if ( ! before || ! after ) { + return []; + } + const lines = []; + for ( const prop of Object.keys( { ...before, ...after } ) ) { + const b = before[ prop ]; + const a = after[ prop ]; + if ( typeof b === 'number' && typeof a === 'number' ) { + const delta = Math.abs( a - b ); + if ( delta > tolerancePx ) { + lines.push( `${ prop }: ${ b }px -> ${ a }px (Δ${ delta.toFixed( 1 ) }px)` ); + } + continue; + } + if ( b !== a ) { + lines.push( `${ prop }: ${ JSON.stringify( b ) } -> ${ JSON.stringify( a ) }` ); + } + } + return lines; +} + +/** + * Diff step 2's geometry snapshots: one entry per configured target (root, header, + * footer, #wpbody-content, control). + * + * @param {object} before - `{ [targetKey]: { label, rect?, style? } }`. + * @param {object} after - Same shape, from the flag-on capture. + * @param {object} [options] + * @param {number} [options.tolerancePx] - See `DEFAULT_TOLERANCE_PX`. + * @return {Array} `{ key, label, status: 'ok'|'changed'|'missing', details }`, one per target present on either side. + */ +export function diffGeometry( before = {}, after = {}, options = {} ) { + const tolerancePx = options.tolerancePx ?? DEFAULT_TOLERANCE_PX; + const keys = new Set( [ ...Object.keys( before ), ...Object.keys( after ) ] ); + const results = []; + + for ( const key of keys ) { + const b = before[ key ]; + const a = after[ key ]; + const label = ( b || a )?.label ?? key; + + if ( ! b || ! a ) { + // Present on one side only. Expected for boot-only markup when the flag is off; + // the caller decides severity per target (see report.js: `required` targets fail). + results.push( { + key, + label, + status: 'missing', + before: b ?? null, + after: a ?? null, + details: [], + } ); + continue; + } + + const details = [ + ...diffRect( b.rect, a.rect, tolerancePx ), + ...diffStyle( b.style, a.style, tolerancePx ), + ]; + results.push( { key, label, status: details.length ? 'changed' : 'ok', details } ); + } + + return results; +} + +/** + * Build the network-diff key for one captured request: method + path, with volatile + * query params (nonces, cache busters) stripped so two genuinely-identical requests + * from separate page loads still match. + * + * @param {{url: string, method: string}} request + * @param {object} [options] + * @param {string[]} [options.ignoreQueryParams] - See `DEFAULT_IGNORED_QUERY_PARAMS`. + * @return {string} The normalized key. + */ +export function normalizeRequestKey( request, options = {} ) { + const ignoreQueryParams = options.ignoreQueryParams ?? DEFAULT_IGNORED_QUERY_PARAMS; + try { + const parsed = new URL( request.url ); + for ( const param of ignoreQueryParams ) { + parsed.searchParams.delete( param ); + } + parsed.searchParams.sort(); + return `${ request.method } ${ parsed.pathname }${ parsed.search }`; + } catch { + // Not an absolute URL (e.g. already a bare path in a fixture). Compare as-is. + return `${ request.method } ${ request.url }`; + } +} + +/** + * Diff step 3's network captures: which requests only fired with the flag off, which + * only fired with it on, and which fired both times but with a different status code. + * + * @param {Array} before - `[{ url, method, status, resourceType }]`. + * @param {Array} after - Same shape, from the flag-on capture. + * @param {object} [options] + * @return {{onlyBefore: object[], onlyAfter: object[], statusChanged: object[]}} + */ +export function diffNetwork( before = [], after = [], options = {} ) { + const beforeMap = new Map( before.map( r => [ normalizeRequestKey( r, options ), r ] ) ); + const afterMap = new Map( after.map( r => [ normalizeRequestKey( r, options ), r ] ) ); + + const onlyBefore = []; + const statusChanged = []; + for ( const [ key, b ] of beforeMap ) { + const a = afterMap.get( key ); + if ( ! a ) { + onlyBefore.push( b ); + } else if ( a.status !== b.status ) { + statusChanged.push( { key, before: b, after: a } ); + } + } + + const onlyAfter = [ ...afterMap.entries() ] + .filter( ( [ key ] ) => ! beforeMap.has( key ) ) + .map( ( [ , r ] ) => r ); + + return { onlyBefore, onlyAfter, statusChanged }; +} + +/** + * Run both step-2 and step-3 diffs over a pair of captured snapshots. + * + * @param {object} before - `{ geometry, network }`, as written by `capture.js` (flag off). + * @param {object} after - Same shape (flag on). + * @param {object} [options] + * @return {{geometry: object[], network: object}} + */ +export function diffSnapshots( before, after, options = {} ) { + return { + geometry: diffGeometry( before.geometry, after.geometry, options ), + network: diffNetwork( before.network, after.network, options ), + }; +} diff --git a/tools/port-verification/src/diff.test.js b/tools/port-verification/src/diff.test.js new file mode 100644 index 000000000000..e8862a30ab17 --- /dev/null +++ b/tools/port-verification/src/diff.test.js @@ -0,0 +1,157 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { diffGeometry, diffNetwork, diffSnapshots, normalizeRequestKey } from './diff.js'; +import { + flagOffSnapshot, + flagOnSnapshot, + flagOnSnapshotWith404, + flagOnSnapshotWithGeometryShift, +} from './fixtures.js'; + +describe( 'diffGeometry', () => { + it( 'reports ok for identical targets', () => { + const before = { + wpbodyContent: { label: 'x', rect: { width: 100 }, style: { fontFamily: 'Arial' } }, + }; + const after = { + wpbodyContent: { label: 'x', rect: { width: 100 }, style: { fontFamily: 'Arial' } }, + }; + const [ result ] = diffGeometry( before, after ); + assert.equal( result.status, 'ok' ); + assert.deepEqual( result.details, [] ); + } ); + + it( 'ignores a rect delta within tolerance', () => { + const before = { wpbodyContent: { label: 'x', rect: { width: 100 } } }; + const after = { wpbodyContent: { label: 'x', rect: { width: 100.3 } } }; + const [ result ] = diffGeometry( before, after, { tolerancePx: 0.5 } ); + assert.equal( result.status, 'ok' ); + } ); + + it( 'flags a rect delta beyond tolerance', () => { + const before = { wpbodyContent: { label: 'x', rect: { width: 100 } } }; + const after = { wpbodyContent: { label: 'x', rect: { width: 104 } } }; + const [ result ] = diffGeometry( before, after, { tolerancePx: 0.5 } ); + assert.equal( result.status, 'changed' ); + assert.match( result.details[ 0 ], /width: 100px -> 104px \(Δ4\.0px\)/ ); + } ); + + it( 'flags a font-family change as a string diff', () => { + const before = { header: { label: 'Header', rect: {}, style: { fontFamily: 'Arial' } } }; + const after = { header: { label: 'Header', rect: {}, style: { fontFamily: 'Helvetica' } } }; + const [ result ] = diffGeometry( before, after ); + assert.equal( result.status, 'changed' ); + assert.match( result.details[ 0 ], /fontFamily/ ); + } ); + + it( 'flags a control box-model prop that moved beyond tolerance', () => { + const before = { + control: { label: 'Control', rect: { width: 100 }, style: { paddingTop: 6 } }, + }; + const after = { + control: { label: 'Control', rect: { width: 100 }, style: { paddingTop: 10 } }, + }; + const [ result ] = diffGeometry( before, after, { tolerancePx: 0.5 } ); + assert.equal( result.status, 'changed' ); + assert.match( result.details[ 0 ], /paddingTop: 6px -> 10px/ ); + } ); + + it( 'marks a target present only on one side as missing', () => { + const before = { control: { label: 'Control', rect: {}, style: {} } }; + const after = {}; + const [ result ] = diffGeometry( before, after ); + assert.equal( result.status, 'missing' ); + assert.equal( result.after, null ); + } ); + + it( 'skips a target absent from both snapshots', () => { + const results = diffGeometry( {}, {} ); + assert.deepEqual( results, [] ); + } ); +} ); + +describe( 'normalizeRequestKey', () => { + it( 'strips ignored query params so two loads with different nonces still match', () => { + const a = { url: 'https://site.test/wp-json/x?_wpnonce=aaa&foo=1', method: 'GET' }; + const b = { url: 'https://site.test/wp-json/x?_wpnonce=bbb&foo=1', method: 'GET' }; + assert.equal( normalizeRequestKey( a ), normalizeRequestKey( b ) ); + } ); + + it( 'keeps params that were not marked as ignorable', () => { + const a = { url: 'https://site.test/wp-json/x?foo=1', method: 'GET' }; + const b = { url: 'https://site.test/wp-json/x?foo=2', method: 'GET' }; + assert.notEqual( normalizeRequestKey( a ), normalizeRequestKey( b ) ); + } ); + + it( 'falls back to a raw compare for a non-absolute URL', () => { + assert.equal( + normalizeRequestKey( { url: '/relative/path', method: 'GET' } ), + 'GET /relative/path' + ); + } ); +} ); + +describe( 'diffNetwork', () => { + it( 'finds no differences for identical request lists', () => { + const requests = [ { url: 'https://site.test/a.js', method: 'GET', status: 200 } ]; + const result = diffNetwork( requests, requests ); + assert.deepEqual( result, { onlyBefore: [], onlyAfter: [], statusChanged: [] } ); + } ); + + it( 'catches a request that only fires after the flag flips (the design-tokens.css 404 case)', () => { + const before = [ { url: 'https://site.test/a.js', method: 'GET', status: 200 } ]; + const after = [ + { url: 'https://site.test/a.js', method: 'GET', status: 200 }, + { url: 'https://site.test/design-tokens.css', method: 'GET', status: 404 }, + ]; + const result = diffNetwork( before, after ); + assert.equal( result.onlyAfter.length, 1 ); + assert.equal( result.onlyAfter[ 0 ].status, 404 ); + assert.equal( result.onlyBefore.length, 0 ); + } ); + + it( 'catches a status code change on the same request', () => { + const before = [ { url: 'https://site.test/a.js', method: 'GET', status: 200 } ]; + const after = [ { url: 'https://site.test/a.js', method: 'GET', status: 500 } ]; + const result = diffNetwork( before, after ); + assert.equal( result.statusChanged.length, 1 ); + assert.equal( result.statusChanged[ 0 ].before.status, 200 ); + assert.equal( result.statusChanged[ 0 ].after.status, 500 ); + } ); + + it( 'catches a request that disappears after the flag flips', () => { + const before = [ { url: 'https://site.test/legacy.js', method: 'GET', status: 200 } ]; + const after = []; + const result = diffNetwork( before, after ); + assert.equal( result.onlyBefore.length, 1 ); + } ); +} ); + +describe( 'diffSnapshots (fixtures)', () => { + it( 'a clean port reports no geometry or network findings beyond the accepted root inset', () => { + const { geometry, network } = diffSnapshots( flagOffSnapshot(), flagOnSnapshot() ); + const nonRootFindings = geometry.filter( g => g.key !== 'root' && g.status !== 'ok' ); + assert.deepEqual( nonRootFindings, [] ); + assert.deepEqual( network.onlyBefore, [] ); + assert.deepEqual( network.onlyAfter, [] ); + assert.deepEqual( network.statusChanged, [] ); + + const root = geometry.find( g => g.key === 'root' ); + assert.equal( root.status, 'changed' ); + assert.match( root.details.join( ' ' ), /x: 0px -> 8px/ ); + } ); + + it( 'flags the pilot-style design-tokens.css 404 as a network-only finding', () => { + const { network } = diffSnapshots( flagOffSnapshot(), flagOnSnapshotWith404() ); + assert.equal( network.onlyAfter.length, 1 ); + assert.match( network.onlyAfter[ 0 ].url, /design-tokens\.css/ ); + assert.equal( network.onlyAfter[ 0 ].status, 404 ); + } ); + + it( 'flags a #wpbody-content geometry shift beyond tolerance', () => { + const { geometry } = diffSnapshots( flagOffSnapshot(), flagOnSnapshotWithGeometryShift() ); + const finding = geometry.find( g => g.key === 'wpbodyContent' ); + assert.equal( finding.status, 'changed' ); + assert.match( finding.details.join( ' ' ), /width: 1120px -> 1132px \(Δ12\.0px\)/ ); + } ); +} ); diff --git a/tools/port-verification/src/fixtures.js b/tools/port-verification/src/fixtures.js new file mode 100644 index 000000000000..fd3bd8207e6e --- /dev/null +++ b/tools/port-verification/src/fixtures.js @@ -0,0 +1,112 @@ +/** + * Fixture snapshots for src/*.test.js, shaped like `capture.js`'s output. `flagOffSnapshot` + * and `flagOnSnapshot` model a clean port: only the accepted 8px stage-gutter inset on the + * page root differs. The other fixtures start from a clone and introduce one specific defect, + * matching the two real bugs JETPACK-2573 cites from the My Jetpack pilot. + */ + +/** + * + */ +export function flagOffSnapshot() { + return { + meta: { + url: 'https://example.jurassic.ninja/wp-admin/admin.php?page=jetpack', + capturedAt: '2026-09-21T12:00:00.000Z', + }, + geometry: { + root: { + label: 'Page root (#wpwrap)', + rect: { x: 0, y: 0, width: 1280, height: 900 }, + style: { fontFamily: 'Arial, sans-serif' }, + }, + wpbodyContent: { + label: '#wpbody-content', + rect: { x: 160, y: 32, width: 1120, height: 860 }, + style: { fontFamily: 'Arial, sans-serif' }, + }, + header: { + label: 'Header (#wpadminbar)', + rect: { x: 0, y: 0, width: 1280, height: 32 }, + style: { fontFamily: 'Arial, sans-serif' }, + }, + footer: { + label: 'Footer (#wpfooter)', + rect: { x: 160, y: 880, width: 1120, height: 20 }, + style: { fontFamily: 'Arial, sans-serif' }, + }, + control: { + label: 'Control', + rect: { width: 100, height: 36 }, + style: { + fontFamily: 'Arial, sans-serif', + marginTop: 8, + marginRight: 8, + marginBottom: 8, + marginLeft: 8, + paddingTop: 6, + paddingRight: 12, + paddingBottom: 6, + paddingLeft: 12, + borderTopWidth: 1, + borderRightWidth: 1, + borderBottomWidth: 1, + borderLeftWidth: 1, + boxSizing: 'border-box', + fontSize: '13px', + }, + }, + }, + network: [ + { + url: 'https://example.jurassic.ninja/wp-content/plugins/jetpack/dashboard.js', + method: 'GET', + status: 200, + resourceType: 'script', + }, + { + url: 'https://example.jurassic.ninja/wp-json/jetpack/v4/settings?_wpnonce=aaa', + method: 'GET', + status: 200, + resourceType: 'xhr', + }, + ], + }; +} + +/** A clean port: the page root shifts by the one accepted 8px stage-gutter inset, nothing else moves. */ +export function flagOnSnapshot() { + const snapshot = flagOffSnapshot(); + snapshot.meta.capturedAt = '2026-09-21T12:03:00.000Z'; + snapshot.geometry.root = { + ...snapshot.geometry.root, + rect: { x: 8, y: 8, width: 1264, height: 884 }, + }; + snapshot.network[ 1 ] = { + ...snapshot.network[ 1 ], + url: snapshot.network[ 1 ].url.replace( 'aaa', 'bbb' ), + }; + return snapshot; +} + +/** Reproduces the pilot's design-tokens.css 404: a new request, same everything else. */ +export function flagOnSnapshotWith404() { + const snapshot = flagOnSnapshot(); + snapshot.network.push( { + url: 'https://example.jurassic.ninja/wp-content/plugins/jetpack/design-tokens.css', + method: 'GET', + status: 404, + resourceType: 'stylesheet', + } ); + return snapshot; +} + +/** Reproduces a real geometry regression: #wpbody-content grows 12px wider than trunk. */ +export function flagOnSnapshotWithGeometryShift() { + const snapshot = flagOnSnapshot(); + snapshot.geometry.wpbodyContent = { + ...snapshot.geometry.wpbodyContent, + rect: { ...snapshot.geometry.wpbodyContent.rect, width: 1132 }, + }; + return snapshot; +} diff --git a/tools/port-verification/src/report.js b/tools/port-verification/src/report.js new file mode 100644 index 000000000000..b5100e6e1962 --- /dev/null +++ b/tools/port-verification/src/report.js @@ -0,0 +1,112 @@ +/** + * Turns a diff result (from diff.js) into the Markdown block a reviewer pastes into a + * port PR. No browser here either -- plain data in, a string out. + */ + +import { DEFAULT_GEOMETRY_TARGETS } from './selectors.js'; + +const STATUS_LABEL = { + ok: 'OK', + changed: 'CHANGED', + missing: 'MISSING', +}; + +/** + * @param {object} geometryDiff - One entry from `diffGeometry()`. + * @param {string} geometryDiff.key - Target key (e.g. `wpbodyContent`). + * @param {string} geometryDiff.label - Human-readable target name. + * @param {string} geometryDiff.status - `'ok'` | `'changed'` | `'missing'`. + * @param {?object} geometryDiff.before - The flag-off capture's entry for this target, or null. + * @param {string[]} geometryDiff.details - Lines describing what changed. + * @param {object} targets - Selector config, for `required`. + * @return {string} A single Markdown table row. + */ +function geometryRow( { key, label, status, before, details }, targets ) { + const required = targets[ key ]?.required ?? false; + const effectiveStatus = + status === 'missing' && ! required ? 'ok (not present, optional)' : STATUS_LABEL[ status ]; + let detailText = '—'; + if ( status === 'missing' ) { + const side = ! before ? 'before (flag off)' : 'after (flag on)'; + detailText = `not found in ${ side }${ required ? ' -- required target' : '' }`; + } else if ( details.length ) { + detailText = details.join( '
' ); + } + return `| ${ label } | ${ effectiveStatus } | ${ detailText } |`; +} + +/** + * @param {object[]} requests + * @return {string} One Markdown bullet list line per request, or a single "none" line. + */ +function requestList( requests ) { + if ( requests.length === 0 ) { + return ' - none'; + } + return requests.map( r => ` - \`${ r.method } ${ r.url }\` -> ${ r.status }` ).join( '\n' ); +} + +/** + * @param {{key: string, before: object, after: object}[]} changes - From `diffNetwork().statusChanged`. + * @return {string} One Markdown bullet list line per changed request, or a single "none" line. + */ +function statusChangeList( changes ) { + if ( changes.length === 0 ) { + return ' - none'; + } + return changes + .map( c => ` - \`${ c.key }\`: ${ c.before.status } -> ${ c.after.status }` ) + .join( '\n' ); +} + +/** + * @param {{geometry: object[], network: object}} diffResult - From `diffSnapshots()`. + * @param {object} meta - Run context to print in the header. + * @param {string} meta.url - Page URL that was captured. + * @param {string} [meta.flag] - Feature flag name. + * @param {string} [meta.beforeCapturedAt] - ISO timestamp of the flag-off capture. + * @param {string} [meta.afterCapturedAt] - ISO timestamp of the flag-on capture. + * @param {object} [targets] - Selector config; defaults to `DEFAULT_GEOMETRY_TARGETS`. + * @return {string} Markdown, ready to paste into a PR description. + */ +export function formatReport( diffResult, meta = {}, targets = DEFAULT_GEOMETRY_TARGETS ) { + const { geometry, network } = diffResult; + const geometryFindings = geometry.filter( + g => + g.status === 'changed' || + ( g.status === 'missing' && ( targets[ g.key ]?.required ?? false ) ) + ).length; + const networkFindings = + network.onlyBefore.length + network.onlyAfter.length + network.statusChanged.length; + + const lines = [ + '## Port verification -- steps 2 & 3 (JETPACK-2685)', + '', + `**Page:** \`${ meta.url ?? 'unknown' }\``, + meta.flag ? `**Flag:** \`${ meta.flag }\`` : null, + `**Before (flag off):** captured ${ meta.beforeCapturedAt ?? 'unknown' }`, + `**After (flag on):** captured ${ meta.afterCapturedAt ?? 'unknown' }`, + '', + '### Step 2 -- computed styles and geometry', + '', + '| Element | Status | Details |', + '| --- | --- | --- |', + ...geometry.map( g => geometryRow( g, targets ) ), + '', + '### Step 3 -- network panel', + '', + `- Only with flag off (${ network.onlyBefore.length }):`, + requestList( network.onlyBefore ), + `- Only with flag on (${ network.onlyAfter.length }):`, + requestList( network.onlyAfter ), + `- Status code changed (${ network.statusChanged.length }):`, + statusChangeList( network.statusChanged ), + '', + '### Summary', + '', + `${ geometryFindings } geometry finding(s), ${ networkFindings } network finding(s).`, + "The only difference JETPACK-2573 accepts is a uniform 8px inset from boot's stage gutter on the page root. Anything else above needs a look before merging.", + ].filter( line => line !== null ); + + return lines.join( '\n' ); +} diff --git a/tools/port-verification/src/report.test.js b/tools/port-verification/src/report.test.js new file mode 100644 index 000000000000..319b3c24b13b --- /dev/null +++ b/tools/port-verification/src/report.test.js @@ -0,0 +1,88 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { diffSnapshots } from './diff.js'; +import { + flagOffSnapshot, + flagOnSnapshot, + flagOnSnapshotWith404, + flagOnSnapshotWithGeometryShift, +} from './fixtures.js'; +import { formatReport } from './report.js'; + +describe( 'formatReport', () => { + it( 'includes the page, flag and capture timestamps in the header', () => { + const report = formatReport( diffSnapshots( flagOffSnapshot(), flagOnSnapshot() ), { + url: 'https://example.jurassic.ninja/wp-admin/admin.php?page=jetpack', + flag: 'rsm_jetpack_ui_modernization_x', + beforeCapturedAt: '2026-09-21T12:00:00.000Z', + afterCapturedAt: '2026-09-21T12:03:00.000Z', + } ); + assert.match( report, /admin\.php\?page=jetpack/ ); + assert.match( report, /rsm_jetpack_ui_modernization_x/ ); + assert.match( report, /2026-09-21T12:00:00\.000Z/ ); + assert.match( report, /2026-09-21T12:03:00\.000Z/ ); + } ); + + it( 'lists every geometry target as a table row, including the accepted root inset', () => { + const report = formatReport( diffSnapshots( flagOffSnapshot(), flagOnSnapshot() ), { + url: 'x', + } ); + assert.match( report, /\| Page root \(#wpwrap\) \| CHANGED \|/ ); + assert.match( report, /\| #wpbody-content \| OK \|/ ); + assert.match( report, /\| Header \(#wpadminbar\) \| OK \|/ ); + assert.match( report, /\| Footer \(#wpfooter\) \| OK \|/ ); + assert.match( report, /\| Control \| OK \|/ ); + } ); + + it( 'surfaces a geometry regression as a CHANGED row with the pixel delta', () => { + const report = formatReport( + diffSnapshots( flagOffSnapshot(), flagOnSnapshotWithGeometryShift() ), + { url: 'x' } + ); + assert.match( report, /\| #wpbody-content \| CHANGED \| width: 1120px -> 1132px/ ); + } ); + + it( 'surfaces a network-only request in step 3', () => { + const report = formatReport( diffSnapshots( flagOffSnapshot(), flagOnSnapshotWith404() ), { + url: 'x', + } ); + assert.match( report, /Only with flag on \(1\):/ ); + assert.match( report, /design-tokens\.css` -> 404/ ); + } ); + + it( 'prints "none" for an empty network bucket', () => { + const report = formatReport( diffSnapshots( flagOffSnapshot(), flagOnSnapshot() ), { + url: 'x', + } ); + assert.match( report, /Only with flag off \(0\):\n {2}- none/ ); + } ); + + it( 'summary counts findings, excluding the missing-but-optional control', () => { + const before = { geometry: {}, network: [] }; + const after = { geometry: {}, network: [] }; + const report = formatReport( diffSnapshots( before, after ), { url: 'x' } ); + assert.match( report, /0 geometry finding\(s\), 0 network finding\(s\)\./ ); + } ); + + it( 'escalates a required target missing from one side to a geometry finding', () => { + const before = { + geometry: { header: { label: 'Header (#wpadminbar)', rect: { width: 10 }, style: {} } }, + network: [], + }; + const after = { geometry: {}, network: [] }; + const report = formatReport( diffSnapshots( before, after ), { url: 'x' } ); + assert.match( report, /\| Header \(#wpadminbar\) \| MISSING \|/ ); + assert.match( report, /1 geometry finding\(s\)/ ); + } ); + + it( 'treats a control missing on one side as optional, not a finding', () => { + const before = { + geometry: { control: { label: 'Control', rect: { width: 10 }, style: {} } }, + network: [], + }; + const after = { geometry: {}, network: [] }; + const report = formatReport( diffSnapshots( before, after ), { url: 'x' } ); + assert.match( report, /\| Control \| ok \(not present, optional\) \|/ ); + assert.match( report, /0 geometry finding\(s\)/ ); + } ); +} ); diff --git a/tools/port-verification/src/selectors.js b/tools/port-verification/src/selectors.js new file mode 100644 index 000000000000..22c0db4ec2f1 --- /dev/null +++ b/tools/port-verification/src/selectors.js @@ -0,0 +1,23 @@ +/** + * Default targets for step 2 (JETPACK-2685): the WP admin frame elements that must + * stay put when a wp-build port is only a chassis swap. `#wpwrap`, `#wpadminbar` and + * `#wpfooter` are core wp-admin markup present on every page, flag on or off, so they + * catch the port shifting the frame around its own content -- not the content itself. + * + * `control` has no default: a representative interactive control (a button, a toggle) + * is page-specific. Pass one with --control-selector. + */ +export const DEFAULT_GEOMETRY_TARGETS = { + root: { label: 'Page root (#wpwrap)', selector: '#wpwrap', required: true }, + wpbodyContent: { label: '#wpbody-content', selector: '#wpbody-content', required: true }, + header: { label: 'Header (#wpadminbar)', selector: '#wpadminbar', required: true }, + footer: { label: 'Footer (#wpfooter)', selector: '#wpfooter', required: true }, + control: { label: 'Control', selector: null, required: false }, +}; + +// Query params that legitimately differ between two page loads (nonces, cache busters) +// without the port having changed anything. --ignore-query-param adds more. +export const DEFAULT_IGNORED_QUERY_PARAMS = [ '_wpnonce', 'ver', 'v', 't', '_', '_locale' ]; + +// Below this, a geometry delta is rounding noise, not a real shift. +export const DEFAULT_TOLERANCE_PX = 0.5; From c14953cff98fe315b20a9470b1223b90a0d77fac Mon Sep 17 00:00:00 2001 From: Christian Gastrell Date: Mon, 21 Sep 2026 18:21:00 -0300 Subject: [PATCH 2/7] Fix wpfooter false positive, redact nonces, and narrow the ignore list PR review on #52579 found three issues in the port verification script: - Blocker: boot hides #wpfooter by design (see projects/js-packages/base-styles/admin-page-layout.scss, so every live run reported it CHANGED with bogus zero-rect deltas -- getBoundingClientRect() on a display:none element is all zeros. capture.js now records a per-target hidden flag; diff.js reports a visibility change as its own hidden-changed status instead of diffing a meaningless rect; report.js renders it as OK (hidden by design) only for targets whose selectors.js entry sets allowHidden (just the footer). An unexpected visibility change on any other target still surfaces as a CHANGED finding. - report.js printed the raw request URL in the network section, so a nonce value survived into Markdown meant to be pasted into a public PR. Added diff.js redactUrl(), which strips the same ignored query params used for matching, and used it for display too. - Dropped v and t from DEFAULT_IGNORED_QUERY_PARAMS -- generic enough to carry real state (an API version, a tab filter) that stripping them could hide a genuine step-3 difference. Added --ignore-query-param for a site that needs them ignored anyway. Also hardened capture.js login(): it now waits for #dashboard-widgets/#wpbody after submit and fails with a clear message instead of silently proceeding to capture wp-login.php own failure page on a wrong password (mirrors tools/performance/scripts/measure-lcp.js). 12 new tests (25 to 37), covering the hidden/missing distinction, the nonce redaction, and the v/t opt-in. node --test and eslint --max-warnings=0 both clean. Co-Authored-By: Claude Sonnet 5 EOF ) --- tools/port-verification/README.md | 34 ++++--- tools/port-verification/bin/verify-port.js | 26 +++-- tools/port-verification/src/capture.js | 35 ++++++- tools/port-verification/src/diff.js | 61 ++++++++++-- tools/port-verification/src/diff.test.js | 109 ++++++++++++++++++++- tools/port-verification/src/fixtures.js | 36 ++++++- tools/port-verification/src/report.js | 48 ++++++--- tools/port-verification/src/report.test.js | 36 ++++++- tools/port-verification/src/selectors.js | 11 ++- 9 files changed, 340 insertions(+), 56 deletions(-) diff --git a/tools/port-verification/README.md b/tools/port-verification/README.md index d788e3527af6..11fe4a3c9233 100644 --- a/tools/port-verification/README.md +++ b/tools/port-verification/README.md @@ -12,10 +12,15 @@ that rule stay manual -- see "Not covered" below. `box-sizing`, `font-size`). These are core wp-admin markup present whether the flag is off or on, so a diff catches the port shifting the frame around its content -- the failure mode JETPACK-2573 calls out: "a 4px shift is invisible in a screenshot and obvious in a number." + A target that goes `display: none` (e.g. `#wpfooter`, which boot hides by design -- see + `projects/js-packages/base-styles/admin-page-layout.scss`) is reported as a visibility + change, not a bogus zero-rect geometry shift. - **Step 3, network.** Every request the page fires, matched flag-off to flag-on by method + - path (nonces and cache-busting query params are ignored). Reports requests that only fired - one time, and requests whose status code changed. This is what caught a `design-tokens.css` - 404 and a renamed JITM message path in the My Jetpack pilot -- both zero-pixel changes. + path (nonces and cache-busting query params -- see `DEFAULT_IGNORED_QUERY_PARAMS` in + `src/selectors.js` -- are stripped before matching and before display, so the report never + quotes a live nonce). Reports requests that only fired one time, and requests whose status + code changed. This is what caught a `design-tokens.css` 404 and a renamed JITM message path + in the My Jetpack pilot -- both zero-pixel changes. The one difference JETPACK-2573 accepts is a uniform 8px inset from boot's stage gutter on the page root. The report says so in its footer; it does not try to auto-approve that one row. @@ -55,16 +60,17 @@ above). ### Options -| Flag | Meaning | -| -------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- | -| `--url` | Page to capture. Required. | -| `--flag` | Feature flag name, printed in the pause prompt and the report header. Optional. | -| `--user` / `--pass` | wp-admin login. Falls back to `WP_ADMIN_USER` / `WP_ADMIN_PASS`. Omit to capture without logging in (e.g. the site is already authenticated). | -| `--control-selector` | CSS selector for the one control to box-model (step 2). Skipped if omitted. | -| `--wait-selector` | Extra selector to wait for after navigation, e.g. the boot mount, so a slow-hydrating page isn't captured mid-render. | -| `--tolerance` | Geometry tolerance in px. Default `0.5` -- rounding noise, not a real shift. | -| `--out` | Write the report to a file (in addition to stdout). | -| `--headed` | Run the browser headed, for watching the capture happen. | +| Flag | Meaning | +| ---------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `--url` | Page to capture. Required. | +| `--flag` | Feature flag name, printed in the pause prompt and the report header. Optional. | +| `--user` / `--pass` | wp-admin login. Falls back to `WP_ADMIN_USER` / `WP_ADMIN_PASS`. Omit to capture without logging in (e.g. the site is already authenticated). | +| `--control-selector` | CSS selector for the one control to box-model (step 2). Skipped if omitted. | +| `--wait-selector` | Extra selector to wait for after navigation, e.g. the boot mount, so a slow-hydrating page isn't captured mid-render. | +| `--tolerance` | Geometry tolerance in px. Default `0.5` -- rounding noise, not a real shift. | +| `--ignore-query-param` | Repeatable. Adds a query param to the default ignore list for step 3 matching and display (`_wpnonce`, `ver`, `_`, `_locale`). Use it for a site-specific volatile param -- generic names like `v` or `t` are deliberately not ignored by default, since they can carry real state (an API version, a tab filter). | +| `--out` | Write the report to a file (in addition to stdout). | +| `--headed` | Run the browser headed, for watching the capture happen. | ### Two-step alternative @@ -95,7 +101,7 @@ node bin/verify-port.js diff --before off.json --after on.json --out report.md | Page root (#wpwrap) | CHANGED | x: 0px -> 8px (Δ8.0px)
y: 0px -> 8px (Δ8.0px)
width: 1280px -> 1264px (Δ16.0px)
height: 900px -> 884px (Δ16.0px) | | #wpbody-content | OK | — | | Header (#wpadminbar) | OK | — | -| Footer (#wpfooter) | OK | — | +| Footer (#wpfooter) | OK (hidden by design) | visibility: visible -> hidden | | Control | OK | — | ### Step 3 -- network panel diff --git a/tools/port-verification/bin/verify-port.js b/tools/port-verification/bin/verify-port.js index 07e2dd9316db..0ad5d236200a 100644 --- a/tools/port-verification/bin/verify-port.js +++ b/tools/port-verification/bin/verify-port.js @@ -11,7 +11,7 @@ import readline from 'readline/promises'; import { parseArgs } from 'util'; import { diffSnapshots } from '../src/diff.js'; import { formatReport } from '../src/report.js'; -import { DEFAULT_TOLERANCE_PX } from '../src/selectors.js'; +import { DEFAULT_IGNORED_QUERY_PARAMS, DEFAULT_TOLERANCE_PX } from '../src/selectors.js'; // Loaded lazily (only by `capture` and `run`) so `--help` and `diff` -- which need no // browser -- still work without Playwright installed. @@ -32,6 +32,8 @@ const USAGE = `Usage: [--tolerance ] [--out ] Credentials also read from WP_ADMIN_USER / WP_ADMIN_PASS. +--ignore-query-param (repeatable) adds to the default ignored list +(${ DEFAULT_IGNORED_QUERY_PARAMS.join( ', ' ) }) for step 3's request matching and display. See README.md for the full walkthrough, including how to flip the flag between captures.`; const OPTION_SPEC = { @@ -42,6 +44,7 @@ const OPTION_SPEC = { 'control-selector': { type: 'string' }, 'wait-selector': { type: 'string' }, tolerance: { type: 'string' }, + 'ignore-query-param': { type: 'string', multiple: true, default: [] }, out: { type: 'string' }, before: { type: 'string' }, after: { type: 'string' }, @@ -55,6 +58,7 @@ const OPTION_SPEC = { */ function parseOptions( argv ) { const { values } = parseArgs( { args: argv, options: OPTION_SPEC, allowPositionals: false } ); + const extraIgnoreParams = values[ 'ignore-query-param' ]; return { url: values.url, flag: values.flag, @@ -63,6 +67,9 @@ function parseOptions( argv ) { controlSelector: values[ 'control-selector' ], waitForSelector: values[ 'wait-selector' ], tolerancePx: values.tolerance ? Number( values.tolerance ) : DEFAULT_TOLERANCE_PX, + ignoreQueryParams: extraIgnoreParams.length + ? [ ...DEFAULT_IGNORED_QUERY_PARAMS, ...extraIgnoreParams ] + : undefined, out: values.out, before: values.before, after: values.after, @@ -115,12 +122,17 @@ function runDiff( options ) { */ function printReport( before, after, options ) { const diffResult = diffSnapshots( before, after, options ); - const report = formatReport( diffResult, { - url: after.meta?.url ?? before.meta?.url ?? options.url, - flag: options.flag, - beforeCapturedAt: before.meta?.capturedAt, - afterCapturedAt: after.meta?.capturedAt, - } ); + const report = formatReport( + diffResult, + { + url: after.meta?.url ?? before.meta?.url ?? options.url, + flag: options.flag, + beforeCapturedAt: before.meta?.capturedAt, + afterCapturedAt: after.meta?.capturedAt, + }, + undefined, + options + ); if ( options.out ) { fs.writeFileSync( options.out, report + '\n' ); console.log( `Report written to ${ options.out }` ); diff --git a/tools/port-verification/src/capture.js b/tools/port-verification/src/capture.js index 91a83a446b6b..b33603300ec5 100644 --- a/tools/port-verification/src/capture.js +++ b/tools/port-verification/src/capture.js @@ -22,10 +22,21 @@ async function login( page, siteUrl, username, password ) { await page.goto( `${ origin }/wp-login.php`, { waitUntil: 'networkidle' } ); await page.fill( '#user_login', username ); await page.fill( '#user_pass', password ); - await Promise.all( [ - page.waitForNavigation( { waitUntil: 'networkidle' } ), - page.click( '#wp-submit' ), - ] ); + + try { + await Promise.all( [ + page.waitForURL( '**/wp-admin/**', { waitUntil: 'networkidle' } ), + page.click( '#wp-submit' ), + ] ); + // Confirm login landed in wp-admin (the Dashboard is the post-login screen) rather than + // proceeding to capture wp-login.php's own failure page, which would otherwise report + // every required target as MISSING. Mirrors tools/performance/scripts/measure-lcp.js. + await page.waitForSelector( '#dashboard-widgets, #wpbody', { timeout: 30000 } ); + } catch { + throw new Error( + `Login did not land in wp-admin (still on ${ page.url() }) -- check --user/--pass.` + ); + } } /** @@ -33,10 +44,23 @@ async function login( page, siteUrl, username, password ) { * `page.evaluate`, so it can only use DOM APIs -- no imports from this module reach it. * * @param {object} targetsArg - `{ [key]: { label, selector } }`, control's selector resolved by the caller. - * @return {object} `{ [key]: { label, rect, style } }` for every target whose selector matched. + * @return {object} `{ [key]: { label, hidden, rect, style } }` for every target whose selector matched. */ /* c8 ignore start -- runs inside the browser context; exercised only by a live capture. */ function extractGeometryInPage( targetsArg ) { + /** + * `display: none` (or `visibility: hidden`) collapses `getBoundingClientRect()` to all + * zeros, which would otherwise read as a bogus geometry shift rather than a visibility + * change (see the #wpfooter case selectors.js documents). + * + * @param {Element} el + * @return {boolean} + */ + function isHidden( el ) { + const cs = window.getComputedStyle( el ); + return cs.display === 'none' || cs.visibility === 'hidden'; + } + /** * @param {Element} el * @return {{x: number, y: number, width: number, height: number}} @@ -91,6 +115,7 @@ function extractGeometryInPage( targetsArg ) { } out[ key ] = { label: target.label, + hidden: isHidden( el ), rect: readRect( el ), style: readStyle( el, key === 'control' ), }; diff --git a/tools/port-verification/src/diff.js b/tools/port-verification/src/diff.js index f20d383c5767..9c9bd02b4e9b 100644 --- a/tools/port-verification/src/diff.js +++ b/tools/port-verification/src/diff.js @@ -68,11 +68,17 @@ function diffStyle( before, after, tolerancePx ) { * Diff step 2's geometry snapshots: one entry per configured target (root, header, * footer, #wpbody-content, control). * - * @param {object} before - `{ [targetKey]: { label, rect?, style? } }`. + * A target hidden on either side (`display: none` / `visibility: hidden`) skips the rect + * and style diff entirely -- `getBoundingClientRect()` on a hidden element is all zeros, + * which would otherwise read as a bogus geometry shift. A visibility change is reported as + * its own `'hidden-changed'` status instead; the caller decides whether that particular + * target is allowed to do that (see report.js: `allowHidden`, for #wpfooter). + * + * @param {object} before - `{ [targetKey]: { label, hidden?, rect?, style? } }`. * @param {object} after - Same shape, from the flag-on capture. * @param {object} [options] * @param {number} [options.tolerancePx] - See `DEFAULT_TOLERANCE_PX`. - * @return {Array} `{ key, label, status: 'ok'|'changed'|'missing', details }`, one per target present on either side. + * @return {Array} `{ key, label, status: 'ok'|'changed'|'missing'|'hidden-changed', details }`, one per target present on either side. */ export function diffGeometry( before = {}, after = {}, options = {} ) { const tolerancePx = options.tolerancePx ?? DEFAULT_TOLERANCE_PX; @@ -98,6 +104,21 @@ export function diffGeometry( before = {}, after = {}, options = {} ) { continue; } + if ( b.hidden || a.hidden ) { + if ( Boolean( b.hidden ) === Boolean( a.hidden ) ) { + results.push( { key, label, status: 'ok', details: [] } ); + } else { + const visibility = b.hidden ? 'hidden -> visible' : 'visible -> hidden'; + results.push( { + key, + label, + status: 'hidden-changed', + details: [ `visibility: ${ visibility }` ], + } ); + } + continue; + } + const details = [ ...diffRect( b.rect, a.rect, tolerancePx ), ...diffStyle( b.style, a.style, tolerancePx ), @@ -108,6 +129,18 @@ export function diffGeometry( before = {}, after = {}, options = {} ) { return results; } +/** + * @param {URL} parsedUrl + * @param {string[]} ignoreQueryParams + * @return {URL} The same `URL`, mutated in place with those params removed. + */ +function stripQueryParams( parsedUrl, ignoreQueryParams ) { + for ( const param of ignoreQueryParams ) { + parsedUrl.searchParams.delete( param ); + } + return parsedUrl; +} + /** * Build the network-diff key for one captured request: method + path, with volatile * query params (nonces, cache busters) stripped so two genuinely-identical requests @@ -121,10 +154,7 @@ export function diffGeometry( before = {}, after = {}, options = {} ) { export function normalizeRequestKey( request, options = {} ) { const ignoreQueryParams = options.ignoreQueryParams ?? DEFAULT_IGNORED_QUERY_PARAMS; try { - const parsed = new URL( request.url ); - for ( const param of ignoreQueryParams ) { - parsed.searchParams.delete( param ); - } + const parsed = stripQueryParams( new URL( request.url ), ignoreQueryParams ); parsed.searchParams.sort(); return `${ request.method } ${ parsed.pathname }${ parsed.search }`; } catch { @@ -133,6 +163,25 @@ export function normalizeRequestKey( request, options = {} ) { } } +/** + * Strip the same ignored query params from a URL for display. Report output is pasted into + * a public PR (this is a public repo), and a `_wpnonce` value is still a nonce even after + * it stopped mattering for matching -- see `normalizeRequestKey`. + * + * @param {string} url + * @param {object} [options] + * @param {string[]} [options.ignoreQueryParams] - See `DEFAULT_IGNORED_QUERY_PARAMS`. + * @return {string} The URL with those params removed, or the input unchanged if it does not parse. + */ +export function redactUrl( url, options = {} ) { + const ignoreQueryParams = options.ignoreQueryParams ?? DEFAULT_IGNORED_QUERY_PARAMS; + try { + return stripQueryParams( new URL( url ), ignoreQueryParams ).toString(); + } catch { + return url; + } +} + /** * Diff step 3's network captures: which requests only fired with the flag off, which * only fired with it on, and which fired both times but with a different status code. diff --git a/tools/port-verification/src/diff.test.js b/tools/port-verification/src/diff.test.js index e8862a30ab17..5065b0219d73 100644 --- a/tools/port-verification/src/diff.test.js +++ b/tools/port-verification/src/diff.test.js @@ -1,11 +1,18 @@ import assert from 'node:assert/strict'; import { describe, it } from 'node:test'; -import { diffGeometry, diffNetwork, diffSnapshots, normalizeRequestKey } from './diff.js'; +import { + diffGeometry, + diffNetwork, + diffSnapshots, + normalizeRequestKey, + redactUrl, +} from './diff.js'; import { flagOffSnapshot, flagOnSnapshot, flagOnSnapshotWith404, flagOnSnapshotWithGeometryShift, + flagOnSnapshotWithUnexpectedlyHiddenHeader, } from './fixtures.js'; describe( 'diffGeometry', () => { @@ -68,6 +75,45 @@ describe( 'diffGeometry', () => { const results = diffGeometry( {}, {} ); assert.deepEqual( results, [] ); } ); + + it( 'reports a visible-to-hidden transition as hidden-changed, not a bogus zero-rect delta', () => { + const before = { + footer: { label: 'Footer', hidden: false, rect: { x: 160, y: 880, width: 1120, height: 20 } }, + }; + const after = { + footer: { label: 'Footer', hidden: true, rect: { x: 0, y: 0, width: 0, height: 0 } }, + }; + const [ result ] = diffGeometry( before, after ); + assert.equal( result.status, 'hidden-changed' ); + assert.deepEqual( result.details, [ 'visibility: visible -> hidden' ] ); + // The regression this guards: before the fix, this printed "width: 1120px -> 0px" and + // similar bogus deltas for every rect field, since getBoundingClientRect() on a + // display:none element is all zeros. + assert.ok( ! result.details.some( line => /width:|height:|^x:|^y:/.test( line ) ) ); + } ); + + it( 'reports a hidden-to-visible transition the other direction', () => { + const before = { + footer: { label: 'Footer', hidden: true, rect: { x: 0, y: 0, width: 0, height: 0 } }, + }; + const after = { + footer: { label: 'Footer', hidden: false, rect: { x: 160, y: 880, width: 1120, height: 20 } }, + }; + const [ result ] = diffGeometry( before, after ); + assert.equal( result.status, 'hidden-changed' ); + assert.deepEqual( result.details, [ 'visibility: hidden -> visible' ] ); + } ); + + it( 'reports ok, not a finding, when a target is hidden on both sides', () => { + const before = { + footer: { label: 'Footer', hidden: true, rect: { x: 0, y: 0, width: 0, height: 0 } }, + }; + const after = { + footer: { label: 'Footer', hidden: true, rect: { x: 0, y: 0, width: 0, height: 0 } }, + }; + const [ result ] = diffGeometry( before, after ); + assert.equal( result.status, 'ok' ); + } ); } ); describe( 'normalizeRequestKey', () => { @@ -83,6 +129,23 @@ describe( 'normalizeRequestKey', () => { assert.notEqual( normalizeRequestKey( a ), normalizeRequestKey( b ) ); } ); + it( "does not ignore 'v' or 't' by default -- generic enough to carry real state (an API version, a tab)", () => { + const v1 = { url: 'https://site.test/wp-json/x?v=1', method: 'GET' }; + const v2 = { url: 'https://site.test/wp-json/x?v=2', method: 'GET' }; + assert.notEqual( normalizeRequestKey( v1 ), normalizeRequestKey( v2 ) ); + + const t1 = { url: 'https://site.test/wp-json/x?t=inbox', method: 'GET' }; + const t2 = { url: 'https://site.test/wp-json/x?t=sent', method: 'GET' }; + assert.notEqual( normalizeRequestKey( t1 ), normalizeRequestKey( t2 ) ); + } ); + + it( 'ignores an extra param only when the caller opts in via ignoreQueryParams', () => { + const a = { url: 'https://site.test/wp-json/x?v=1', method: 'GET' }; + const b = { url: 'https://site.test/wp-json/x?v=2', method: 'GET' }; + const options = { ignoreQueryParams: [ 'v' ] }; + assert.equal( normalizeRequestKey( a, options ), normalizeRequestKey( b, options ) ); + } ); + it( 'falls back to a raw compare for a non-absolute URL', () => { assert.equal( normalizeRequestKey( { url: '/relative/path', method: 'GET' } ), @@ -91,6 +154,23 @@ describe( 'normalizeRequestKey', () => { } ); } ); +describe( 'redactUrl', () => { + it( 'strips a nonce from a URL before it would be shown in a report', () => { + const redacted = redactUrl( 'https://site.test/wp-json/x?_wpnonce=deadbeef1234&foo=1' ); + assert.ok( ! redacted.includes( 'deadbeef1234' ) ); + assert.ok( redacted.includes( 'foo=1' ) ); + } ); + + it( 'keeps a param that was not marked as ignorable', () => { + const redacted = redactUrl( 'https://site.test/wp-json/x?type=post' ); + assert.ok( redacted.includes( 'type=post' ) ); + } ); + + it( 'returns a non-absolute URL unchanged', () => { + assert.equal( redactUrl( '/relative/path?_wpnonce=aaa' ), '/relative/path?_wpnonce=aaa' ); + } ); +} ); + describe( 'diffNetwork', () => { it( 'finds no differences for identical request lists', () => { const requests = [ { url: 'https://site.test/a.js', method: 'GET', status: 200 } ]; @@ -128,10 +208,12 @@ describe( 'diffNetwork', () => { } ); describe( 'diffSnapshots (fixtures)', () => { - it( 'a clean port reports no geometry or network findings beyond the accepted root inset', () => { + it( 'a clean port reports no geometry or network findings beyond the accepted root inset and the footer going hidden', () => { const { geometry, network } = diffSnapshots( flagOffSnapshot(), flagOnSnapshot() ); - const nonRootFindings = geometry.filter( g => g.key !== 'root' && g.status !== 'ok' ); - assert.deepEqual( nonRootFindings, [] ); + const otherFindings = geometry.filter( + g => g.key !== 'root' && g.key !== 'footer' && g.status !== 'ok' + ); + assert.deepEqual( otherFindings, [] ); assert.deepEqual( network.onlyBefore, [] ); assert.deepEqual( network.onlyAfter, [] ); assert.deepEqual( network.statusChanged, [] ); @@ -139,9 +221,16 @@ describe( 'diffSnapshots (fixtures)', () => { const root = geometry.find( g => g.key === 'root' ); assert.equal( root.status, 'changed' ); assert.match( root.details.join( ' ' ), /x: 0px -> 8px/ ); + + // #wpfooter goes display:none by design (see fixtures.js) -- a raw hidden-changed fact + // at this pure-diff level; report.js's allowHidden config is what excludes it from + // counting as a finding for a reviewer. + const footer = geometry.find( g => g.key === 'footer' ); + assert.equal( footer.status, 'hidden-changed' ); + assert.deepEqual( footer.details, [ 'visibility: visible -> hidden' ] ); } ); - it( 'flags the pilot-style design-tokens.css 404 as a network-only finding', () => { + it( 'flags the pilot-style design-tokens.css 404 as a network-only finding, with the nonce stripped from the match', () => { const { network } = diffSnapshots( flagOffSnapshot(), flagOnSnapshotWith404() ); assert.equal( network.onlyAfter.length, 1 ); assert.match( network.onlyAfter[ 0 ].url, /design-tokens\.css/ ); @@ -154,4 +243,14 @@ describe( 'diffSnapshots (fixtures)', () => { assert.equal( finding.status, 'changed' ); assert.match( finding.details.join( ' ' ), /width: 1120px -> 1132px \(Δ12\.0px\)/ ); } ); + + it( 'flags an unexpectedly hidden header as hidden-changed (raw diff fact, regardless of allowHidden policy)', () => { + const { geometry } = diffSnapshots( + flagOffSnapshot(), + flagOnSnapshotWithUnexpectedlyHiddenHeader() + ); + const finding = geometry.find( g => g.key === 'header' ); + assert.equal( finding.status, 'hidden-changed' ); + assert.deepEqual( finding.details, [ 'visibility: visible -> hidden' ] ); + } ); } ); diff --git a/tools/port-verification/src/fixtures.js b/tools/port-verification/src/fixtures.js index fd3bd8207e6e..402bd7be74a1 100644 --- a/tools/port-verification/src/fixtures.js +++ b/tools/port-verification/src/fixtures.js @@ -17,26 +17,31 @@ export function flagOffSnapshot() { geometry: { root: { label: 'Page root (#wpwrap)', + hidden: false, rect: { x: 0, y: 0, width: 1280, height: 900 }, style: { fontFamily: 'Arial, sans-serif' }, }, wpbodyContent: { label: '#wpbody-content', + hidden: false, rect: { x: 160, y: 32, width: 1120, height: 860 }, style: { fontFamily: 'Arial, sans-serif' }, }, header: { label: 'Header (#wpadminbar)', + hidden: false, rect: { x: 0, y: 0, width: 1280, height: 32 }, style: { fontFamily: 'Arial, sans-serif' }, }, footer: { label: 'Footer (#wpfooter)', + hidden: false, rect: { x: 160, y: 880, width: 1120, height: 20 }, style: { fontFamily: 'Arial, sans-serif' }, }, control: { label: 'Control', + hidden: false, rect: { width: 100, height: 36 }, style: { fontFamily: 'Arial, sans-serif', @@ -74,7 +79,11 @@ export function flagOffSnapshot() { }; } -/** A clean port: the page root shifts by the one accepted 8px stage-gutter inset, nothing else moves. */ +/** + * A clean port: the page root shifts by the one accepted 8px stage-gutter inset, and + * #wpfooter goes `display: none` (boot hides it by design -- see + * projects/js-packages/base-styles/admin-page-layout.scss). Nothing else moves. + */ export function flagOnSnapshot() { const snapshot = flagOffSnapshot(); snapshot.meta.capturedAt = '2026-09-21T12:03:00.000Z'; @@ -82,6 +91,12 @@ export function flagOnSnapshot() { ...snapshot.geometry.root, rect: { x: 8, y: 8, width: 1264, height: 884 }, }; + // A real display:none collapses getBoundingClientRect() to all zeros. + snapshot.geometry.footer = { + ...snapshot.geometry.footer, + hidden: true, + rect: { x: 0, y: 0, width: 0, height: 0 }, + }; snapshot.network[ 1 ] = { ...snapshot.network[ 1 ], url: snapshot.network[ 1 ].url.replace( 'aaa', 'bbb' ), @@ -89,11 +104,11 @@ export function flagOnSnapshot() { return snapshot; } -/** Reproduces the pilot's design-tokens.css 404: a new request, same everything else. */ +/** Reproduces the pilot's design-tokens.css 404: a new request carrying a nonce, same everything else. */ export function flagOnSnapshotWith404() { const snapshot = flagOnSnapshot(); snapshot.network.push( { - url: 'https://example.jurassic.ninja/wp-content/plugins/jetpack/design-tokens.css', + url: 'https://example.jurassic.ninja/wp-content/plugins/jetpack/design-tokens.css?_wpnonce=deadbeef1234', method: 'GET', status: 404, resourceType: 'stylesheet', @@ -110,3 +125,18 @@ export function flagOnSnapshotWithGeometryShift() { }; return snapshot; } + +/** + * An UNEXPECTED visibility regression: the header goes `display: none` too. Unlike the + * footer, `header` has no `allowHidden` in `DEFAULT_GEOMETRY_TARGETS`, so this must surface + * as a finding. + */ +export function flagOnSnapshotWithUnexpectedlyHiddenHeader() { + const snapshot = flagOnSnapshot(); + snapshot.geometry.header = { + ...snapshot.geometry.header, + hidden: true, + rect: { x: 0, y: 0, width: 0, height: 0 }, + }; + return snapshot; +} diff --git a/tools/port-verification/src/report.js b/tools/port-verification/src/report.js index b5100e6e1962..2aaa79a24e0f 100644 --- a/tools/port-verification/src/report.js +++ b/tools/port-verification/src/report.js @@ -3,6 +3,7 @@ * port PR. No browser here either -- plain data in, a string out. */ +import { redactUrl } from './diff.js'; import { DEFAULT_GEOMETRY_TARGETS } from './selectors.js'; const STATUS_LABEL = { @@ -15,14 +16,21 @@ const STATUS_LABEL = { * @param {object} geometryDiff - One entry from `diffGeometry()`. * @param {string} geometryDiff.key - Target key (e.g. `wpbodyContent`). * @param {string} geometryDiff.label - Human-readable target name. - * @param {string} geometryDiff.status - `'ok'` | `'changed'` | `'missing'`. + * @param {string} geometryDiff.status - `'ok'` | `'changed'` | `'missing'` | `'hidden-changed'`. * @param {?object} geometryDiff.before - The flag-off capture's entry for this target, or null. * @param {string[]} geometryDiff.details - Lines describing what changed. - * @param {object} targets - Selector config, for `required`. + * @param {object} targets - Selector config, for `required` and `allowHidden`. * @return {string} A single Markdown table row. */ function geometryRow( { key, label, status, before, details }, targets ) { const required = targets[ key ]?.required ?? false; + + if ( status === 'hidden-changed' ) { + const allowed = targets[ key ]?.allowHidden ?? false; + const effectiveStatus = allowed ? 'OK (hidden by design)' : 'CHANGED'; + return `| ${ label } | ${ effectiveStatus } | ${ details.join( '
' ) } |`; + } + const effectiveStatus = status === 'missing' && ! required ? 'ok (not present, optional)' : STATUS_LABEL[ status ]; let detailText = '—'; @@ -37,13 +45,16 @@ function geometryRow( { key, label, status, before, details }, targets ) { /** * @param {object[]} requests + * @param {object} [options] - Passed to `redactUrl()`, so displayed URLs match the ignore list used for matching. * @return {string} One Markdown bullet list line per request, or a single "none" line. */ -function requestList( requests ) { +function requestList( requests, options ) { if ( requests.length === 0 ) { return ' - none'; } - return requests.map( r => ` - \`${ r.method } ${ r.url }\` -> ${ r.status }` ).join( '\n' ); + return requests + .map( r => ` - \`${ r.method } ${ redactUrl( r.url, options ) }\` -> ${ r.status }` ) + .join( '\n' ); } /** @@ -67,15 +78,28 @@ function statusChangeList( changes ) { * @param {string} [meta.beforeCapturedAt] - ISO timestamp of the flag-off capture. * @param {string} [meta.afterCapturedAt] - ISO timestamp of the flag-on capture. * @param {object} [targets] - Selector config; defaults to `DEFAULT_GEOMETRY_TARGETS`. + * @param {object} [options] - Passed to `redactUrl()` for the network section; also accepted by `diffSnapshots()`. * @return {string} Markdown, ready to paste into a PR description. */ -export function formatReport( diffResult, meta = {}, targets = DEFAULT_GEOMETRY_TARGETS ) { +export function formatReport( + diffResult, + meta = {}, + targets = DEFAULT_GEOMETRY_TARGETS, + options = {} +) { const { geometry, network } = diffResult; - const geometryFindings = geometry.filter( - g => - g.status === 'changed' || - ( g.status === 'missing' && ( targets[ g.key ]?.required ?? false ) ) - ).length; + const geometryFindings = geometry.filter( g => { + if ( g.status === 'changed' ) { + return true; + } + if ( g.status === 'missing' ) { + return targets[ g.key ]?.required ?? false; + } + if ( g.status === 'hidden-changed' ) { + return ! ( targets[ g.key ]?.allowHidden ?? false ); + } + return false; + } ).length; const networkFindings = network.onlyBefore.length + network.onlyAfter.length + network.statusChanged.length; @@ -96,9 +120,9 @@ export function formatReport( diffResult, meta = {}, targets = DEFAULT_GEOMETRY_ '### Step 3 -- network panel', '', `- Only with flag off (${ network.onlyBefore.length }):`, - requestList( network.onlyBefore ), + requestList( network.onlyBefore, options ), `- Only with flag on (${ network.onlyAfter.length }):`, - requestList( network.onlyAfter ), + requestList( network.onlyAfter, options ), `- Status code changed (${ network.statusChanged.length }):`, statusChangeList( network.statusChanged ), '', diff --git a/tools/port-verification/src/report.test.js b/tools/port-verification/src/report.test.js index 319b3c24b13b..87c81034004d 100644 --- a/tools/port-verification/src/report.test.js +++ b/tools/port-verification/src/report.test.js @@ -6,6 +6,7 @@ import { flagOnSnapshot, flagOnSnapshotWith404, flagOnSnapshotWithGeometryShift, + flagOnSnapshotWithUnexpectedlyHiddenHeader, } from './fixtures.js'; import { formatReport } from './report.js'; @@ -30,10 +31,35 @@ describe( 'formatReport', () => { assert.match( report, /\| Page root \(#wpwrap\) \| CHANGED \|/ ); assert.match( report, /\| #wpbody-content \| OK \|/ ); assert.match( report, /\| Header \(#wpadminbar\) \| OK \|/ ); - assert.match( report, /\| Footer \(#wpfooter\) \| OK \|/ ); assert.match( report, /\| Control \| OK \|/ ); } ); + it( 'renders the footer going hidden by design as OK, not a bogus geometry CHANGED row', () => { + const report = formatReport( diffSnapshots( flagOffSnapshot(), flagOnSnapshot() ), { + url: 'x', + } ); + assert.match( + report, + /\| Footer \(#wpfooter\) \| OK \(hidden by design\) \| visibility: visible -> hidden \|/ + ); + assert.doesNotMatch( report, /Footer \(#wpfooter\) \| CHANGED/ ); + // The bug this guards: before the fix this row showed bogus px deltas like "width: 1120px -> 0px". + assert.doesNotMatch( report, /width: 1120px -> 0px/ ); + } ); + + it( 'counts an unexpectedly hidden header (no allowHidden) as a geometry finding, unlike the footer', () => { + const report = formatReport( + diffSnapshots( flagOffSnapshot(), flagOnSnapshotWithUnexpectedlyHiddenHeader() ), + { url: 'x' } + ); + assert.match( + report, + /\| Header \(#wpadminbar\) \| CHANGED \| visibility: visible -> hidden \|/ + ); + // root's accepted 8px inset (1) + the unexpected header hidden-change (1) = 2. + assert.match( report, /2 geometry finding\(s\)/ ); + } ); + it( 'surfaces a geometry regression as a CHANGED row with the pixel delta', () => { const report = formatReport( diffSnapshots( flagOffSnapshot(), flagOnSnapshotWithGeometryShift() ), @@ -50,6 +76,14 @@ describe( 'formatReport', () => { assert.match( report, /design-tokens\.css` -> 404/ ); } ); + it( 'never prints a nonce value from a network URL -- this report is pasted into a public PR', () => { + const report = formatReport( diffSnapshots( flagOffSnapshot(), flagOnSnapshotWith404() ), { + url: 'x', + } ); + assert.ok( ! report.includes( 'deadbeef1234' ) ); + assert.match( report, /design-tokens\.css` -> 404/ ); + } ); + it( 'prints "none" for an empty network bucket', () => { const report = formatReport( diffSnapshots( flagOffSnapshot(), flagOnSnapshot() ), { url: 'x', diff --git a/tools/port-verification/src/selectors.js b/tools/port-verification/src/selectors.js index 22c0db4ec2f1..f169e9121801 100644 --- a/tools/port-verification/src/selectors.js +++ b/tools/port-verification/src/selectors.js @@ -11,13 +11,18 @@ export const DEFAULT_GEOMETRY_TARGETS = { root: { label: 'Page root (#wpwrap)', selector: '#wpwrap', required: true }, wpbodyContent: { label: '#wpbody-content', selector: '#wpbody-content', required: true }, header: { label: 'Header (#wpadminbar)', selector: '#wpadminbar', required: true }, - footer: { label: 'Footer (#wpfooter)', selector: '#wpfooter', required: true }, + // allowHidden: boot's admin-page-layout.scss sets `#wpfooter { display: none }` by design + // (replaced with a pinned JetpackFooter inside the layout), so going hidden on flag-on is + // expected, not a finding -- see projects/js-packages/base-styles/admin-page-layout.scss. + footer: { label: 'Footer (#wpfooter)', selector: '#wpfooter', required: true, allowHidden: true }, control: { label: 'Control', selector: null, required: false }, }; // Query params that legitimately differ between two page loads (nonces, cache busters) -// without the port having changed anything. --ignore-query-param adds more. -export const DEFAULT_IGNORED_QUERY_PARAMS = [ '_wpnonce', 'ver', 'v', 't', '_', '_locale' ]; +// without the port having changed anything. Kept short and specific on purpose: a generic +// name (a `v` or `t` param) can carry real state -- e.g. an API version or a tab filter -- +// that step 3 exists to catch. --ignore-query-param adds more for a site that needs them. +export const DEFAULT_IGNORED_QUERY_PARAMS = [ '_wpnonce', 'ver', '_', '_locale' ]; // Below this, a geometry delta is rounding noise, not a real shift. export const DEFAULT_TOLERANCE_PX = 0.5; From 87d3999e827cb198321c8a1c54b09f9ee10ba593 Mon Sep 17 00:00:00 2001 From: Christian Gastrell Date: Mon, 21 Sep 2026 19:17:17 -0300 Subject: [PATCH 3/7] Address review: don't diff login traffic, don't collapse repeat requests Review of #52579 found three ways the tool reports a clean pass on a page it should flag: * The response listener started before `login()`, so step 3 diffed wp-login.php and the Dashboard alongside the target page. Clear the buffer after login. * `diffNetwork` keyed a Map by method + path, so a repeated request kept only its last occurrence -- a 404 refetched into a 200 reported nothing. Group every occurrence, compare the distinct statuses, and report a changed occurrence count on its own. * A non-numeric `--tolerance` became `NaN`, and `delta > NaN` is false for every delta, so step 2 reported OK for every target. Reject it instead. Also: abort when a capture lands on wp-login.php, since a fresh browser profile has no cookies to arrive authenticated with; add `_ajax_nonce` and `_nonce` to the ignored params, so admin-ajax calls match between loads and their nonces stay out of a report pasted into a public PR; give an unmeasured target its own row, so a skipped `--control-selector` no longer reads as a passing check. Option parsing moves to `src/options.js` to be testable without running `main()`. 55 tests, up from 37. Co-Authored-By: Claude Opus 5 (1M context) --- tools/port-verification/README.md | 32 ++++---- tools/port-verification/bin/verify-port.js | 48 +----------- tools/port-verification/src/capture.js | 20 +++-- tools/port-verification/src/diff.js | 84 +++++++++++++++------ tools/port-verification/src/diff.test.js | 79 +++++++++++++++++-- tools/port-verification/src/fixtures.js | 28 ++++--- tools/port-verification/src/options.js | 68 +++++++++++++++++ tools/port-verification/src/options.test.js | 73 ++++++++++++++++++ tools/port-verification/src/report.js | 82 ++++++++++++++++---- tools/port-verification/src/report.test.js | 79 +++++++++++++++++-- tools/port-verification/src/selectors.js | 23 +++--- 11 files changed, 482 insertions(+), 134 deletions(-) create mode 100644 tools/port-verification/src/options.js create mode 100644 tools/port-verification/src/options.test.js diff --git a/tools/port-verification/README.md b/tools/port-verification/README.md index 11fe4a3c9233..773c20e1b11d 100644 --- a/tools/port-verification/README.md +++ b/tools/port-verification/README.md @@ -18,9 +18,11 @@ that rule stay manual -- see "Not covered" below. - **Step 3, network.** Every request the page fires, matched flag-off to flag-on by method + path (nonces and cache-busting query params -- see `DEFAULT_IGNORED_QUERY_PARAMS` in `src/selectors.js` -- are stripped before matching and before display, so the report never - quotes a live nonce). Reports requests that only fired one time, and requests whose status - code changed. This is what caught a `design-tokens.css` 404 and a renamed JITM message path - in the My Jetpack pilot -- both zero-pixel changes. + quotes a live nonce). Reports requests that fired on one side only, requests whose set of + status codes changed, and requests that fired a different number of times. This is what + caught a `design-tokens.css` 404 and a renamed JITM message path in the My Jetpack pilot -- + both zero-pixel changes. The login and Dashboard page loads are dropped before the diff, so + only the page you pointed it at is compared. The one difference JETPACK-2573 accepts is a uniform 8px inset from boot's stage gutter on the page root. The report says so in its footer; it does not try to auto-approve that one row. @@ -60,17 +62,17 @@ above). ### Options -| Flag | Meaning | -| ---------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `--url` | Page to capture. Required. | -| `--flag` | Feature flag name, printed in the pause prompt and the report header. Optional. | -| `--user` / `--pass` | wp-admin login. Falls back to `WP_ADMIN_USER` / `WP_ADMIN_PASS`. Omit to capture without logging in (e.g. the site is already authenticated). | -| `--control-selector` | CSS selector for the one control to box-model (step 2). Skipped if omitted. | -| `--wait-selector` | Extra selector to wait for after navigation, e.g. the boot mount, so a slow-hydrating page isn't captured mid-render. | -| `--tolerance` | Geometry tolerance in px. Default `0.5` -- rounding noise, not a real shift. | -| `--ignore-query-param` | Repeatable. Adds a query param to the default ignore list for step 3 matching and display (`_wpnonce`, `ver`, `_`, `_locale`). Use it for a site-specific volatile param -- generic names like `v` or `t` are deliberately not ignored by default, since they can carry real state (an API version, a tab filter). | -| `--out` | Write the report to a file (in addition to stdout). | -| `--headed` | Run the browser headed, for watching the capture happen. | +| Flag | Meaning | +| ---------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `--url` | Page to capture. Required. | +| `--flag` | Feature flag name, printed in the pause prompt and the report header. Optional. | +| `--user` / `--pass` | wp-admin login. Falls back to `WP_ADMIN_USER` / `WP_ADMIN_PASS`. Each capture starts a fresh browser profile with no cookies, so omit these only when the URL authenticates by itself -- a Jurassic Ninja autologin link. Otherwise the capture aborts on landing at `wp-login.php`. | +| `--control-selector` | CSS selector for the one control to box-model (step 2). Omit it and the report says the control was skipped. | +| `--wait-selector` | Extra selector to wait for after navigation, e.g. the boot mount, so a slow-hydrating page isn't captured mid-render. | +| `--tolerance` | Geometry tolerance in px. Default `0.5` -- rounding noise, not a real shift. A non-numeric value is rejected rather than silently disabling step 2. | +| `--ignore-query-param` | Repeatable. Adds a query param to the default ignore list for step 3 matching and display (`_wpnonce`, `_ajax_nonce`, `_nonce`, `ver`, `_`, `_locale`). Use it for a site-specific volatile param -- generic names like `v` or `t` are deliberately not ignored by default, since they can carry real state (an API version, a tab filter). | +| `--out` | Write the report to a file (in addition to stdout). | +| `--headed` | Run the browser headed, for watching the capture happen. | ### Two-step alternative @@ -112,6 +114,8 @@ node bin/verify-port.js diff --before off.json --after on.json --out report.md - `GET https://example.jurassic.ninja/wp-content/plugins/jetpack/design-tokens.css` -> 404 - Status code changed (0): - none +- Request count changed (0): + - none ### Summary diff --git a/tools/port-verification/bin/verify-port.js b/tools/port-verification/bin/verify-port.js index 0ad5d236200a..255a6548e116 100644 --- a/tools/port-verification/bin/verify-port.js +++ b/tools/port-verification/bin/verify-port.js @@ -8,10 +8,10 @@ import fs from 'fs'; import readline from 'readline/promises'; -import { parseArgs } from 'util'; import { diffSnapshots } from '../src/diff.js'; +import { parseOptions } from '../src/options.js'; import { formatReport } from '../src/report.js'; -import { DEFAULT_IGNORED_QUERY_PARAMS, DEFAULT_TOLERANCE_PX } from '../src/selectors.js'; +import { DEFAULT_IGNORED_QUERY_PARAMS } from '../src/selectors.js'; // Loaded lazily (only by `capture` and `run`) so `--help` and `diff` -- which need no // browser -- still work without Playwright installed. @@ -36,48 +36,6 @@ Credentials also read from WP_ADMIN_USER / WP_ADMIN_PASS. (${ DEFAULT_IGNORED_QUERY_PARAMS.join( ', ' ) }) for step 3's request matching and display. See README.md for the full walkthrough, including how to flip the flag between captures.`; -const OPTION_SPEC = { - url: { type: 'string' }, - flag: { type: 'string' }, - user: { type: 'string' }, - pass: { type: 'string' }, - 'control-selector': { type: 'string' }, - 'wait-selector': { type: 'string' }, - tolerance: { type: 'string' }, - 'ignore-query-param': { type: 'string', multiple: true, default: [] }, - out: { type: 'string' }, - before: { type: 'string' }, - after: { type: 'string' }, - headed: { type: 'boolean', default: false }, - help: { type: 'boolean', default: false }, -}; - -/** - * @param {string[]} argv - Everything after the subcommand. - * @return {object} Parsed options, camel-cased from `OPTION_SPEC`'s kebab-case keys. - */ -function parseOptions( argv ) { - const { values } = parseArgs( { args: argv, options: OPTION_SPEC, allowPositionals: false } ); - const extraIgnoreParams = values[ 'ignore-query-param' ]; - return { - url: values.url, - flag: values.flag, - username: values.user ?? process.env.WP_ADMIN_USER, - password: values.pass ?? process.env.WP_ADMIN_PASS, - controlSelector: values[ 'control-selector' ], - waitForSelector: values[ 'wait-selector' ], - tolerancePx: values.tolerance ? Number( values.tolerance ) : DEFAULT_TOLERANCE_PX, - ignoreQueryParams: extraIgnoreParams.length - ? [ ...DEFAULT_IGNORED_QUERY_PARAMS, ...extraIgnoreParams ] - : undefined, - out: values.out, - before: values.before, - after: values.after, - headless: ! values.headed, - help: values.help, - }; -} - /** * @param {string} path * @param {object} data @@ -195,6 +153,6 @@ async function main() { } main().catch( err => { - console.error( err.stack ?? err.message ); + console.error( process.env.DEBUG ? ( err.stack ?? err.message ) : err.message ); process.exit( 1 ); } ); diff --git a/tools/port-verification/src/capture.js b/tools/port-verification/src/capture.js index b33603300ec5..490fb9dbaf8e 100644 --- a/tools/port-verification/src/capture.js +++ b/tools/port-verification/src/capture.js @@ -50,8 +50,8 @@ async function login( page, siteUrl, username, password ) { function extractGeometryInPage( targetsArg ) { /** * `display: none` (or `visibility: hidden`) collapses `getBoundingClientRect()` to all - * zeros, which would otherwise read as a bogus geometry shift rather than a visibility - * change (see the #wpfooter case selectors.js documents). + * zeros. Recording the fact separately lets the diff report a visibility change instead + * of four bogus px deltas. * * @param {Element} el * @return {boolean} @@ -126,8 +126,8 @@ function extractGeometryInPage( targetsArg ) { /** * Capture one snapshot: navigate (logging in first if credentials are given), wait for - * the page to settle, then read geometry for `targets` and every response since navigation - * started. + * the page to settle, then read geometry for `targets` and every response the target page + * fired. * * @param {object} options * @param {string} options.url - Page to capture. @@ -173,18 +173,24 @@ export async function capturePage( options ) { if ( username && password ) { await login( page, url, username, password ); } + // Step 3 compares the target page's requests. Drop everything wp-login.php and the + // post-login Dashboard fired, or their (partly non-deterministic) traffic is diffed too. + network.length = 0; await page.goto( url, { waitUntil: 'networkidle' } ); + if ( page.url().includes( 'wp-login.php' ) ) { + throw new Error( + `Landed on wp-login.php instead of ${ url } -- pass --user/--pass, or use a URL that carries its own autologin token.` + ); + } if ( waitForSelector ) { await page.waitForSelector( waitForSelector, { state: 'visible' } ); await page.waitForLoadState( 'networkidle' ); } - const geometry = await page.evaluate( extractGeometryInPage, resolvedTargets ); - return { meta: { url, capturedAt: new Date().toISOString() }, - geometry, + geometry: await page.evaluate( extractGeometryInPage, resolvedTargets ), network, }; } finally { diff --git a/tools/port-verification/src/diff.js b/tools/port-verification/src/diff.js index 9c9bd02b4e9b..affbc253b2e9 100644 --- a/tools/port-verification/src/diff.js +++ b/tools/port-verification/src/diff.js @@ -65,14 +65,10 @@ function diffStyle( before, after, tolerancePx ) { } /** - * Diff step 2's geometry snapshots: one entry per configured target (root, header, - * footer, #wpbody-content, control). + * Diff step 2's geometry snapshots, one entry per target captured on either side. * - * A target hidden on either side (`display: none` / `visibility: hidden`) skips the rect - * and style diff entirely -- `getBoundingClientRect()` on a hidden element is all zeros, - * which would otherwise read as a bogus geometry shift. A visibility change is reported as - * its own `'hidden-changed'` status instead; the caller decides whether that particular - * target is allowed to do that (see report.js: `allowHidden`, for #wpfooter). + * A target hidden on either side skips the rect and style diff and reports + * `'hidden-changed'` instead; report.js decides whether that target is allowed to do that. * * @param {object} before - `{ [targetKey]: { label, hidden?, rect?, style? } }`. * @param {object} after - Same shape, from the flag-on capture. @@ -183,34 +179,80 @@ export function redactUrl( url, options = {} ) { } /** - * Diff step 3's network captures: which requests only fired with the flag off, which - * only fired with it on, and which fired both times but with a different status code. + * Group requests by normalized key, keeping every occurrence rather than the last one: + * a path fetched twice in one load can carry two different statuses, and collapsing them + * hides exactly the 404 step 3 exists to find. + * + * @param {object[]} requests + * @param {object} [options] - Passed to `normalizeRequestKey()`. + * @return {Map} + */ +function groupByKey( requests, options ) { + const groups = new Map(); + for ( const request of requests ) { + const key = normalizeRequestKey( request, options ); + const group = groups.get( key ); + if ( group ) { + group.push( request ); + } else { + groups.set( key, [ request ] ); + } + } + return groups; +} + +/** + * Distinct, not per-occurrence: a differing occurrence count would otherwise always read as + * a status change and hide the count difference behind it. + * + * @param {object[]} group - One key's requests. + * @return {number[]} The status codes it saw, ascending. + */ +function statusesOf( group ) { + return [ ...new Set( group.map( r => r.status ) ) ].sort( ( x, y ) => x - y ); +} + +/** + * Diff step 3's network captures: which requests only fired with the flag off, which only + * fired with it on, which fired both times with a different set of status codes, and which + * fired a different number of times. * * @param {Array} before - `[{ url, method, status, resourceType }]`. * @param {Array} after - Same shape, from the flag-on capture. * @param {object} [options] - * @return {{onlyBefore: object[], onlyAfter: object[], statusChanged: object[]}} + * @return {{onlyBefore: object[], onlyAfter: object[], statusChanged: object[], countChanged: object[]}} */ export function diffNetwork( before = [], after = [], options = {} ) { - const beforeMap = new Map( before.map( r => [ normalizeRequestKey( r, options ), r ] ) ); - const afterMap = new Map( after.map( r => [ normalizeRequestKey( r, options ), r ] ) ); + const beforeGroups = groupByKey( before, options ); + const afterGroups = groupByKey( after, options ); const onlyBefore = []; + const onlyAfter = []; const statusChanged = []; - for ( const [ key, b ] of beforeMap ) { - const a = afterMap.get( key ); + const countChanged = []; + + for ( const [ key, b ] of beforeGroups ) { + const a = afterGroups.get( key ); if ( ! a ) { - onlyBefore.push( b ); - } else if ( a.status !== b.status ) { - statusChanged.push( { key, before: b, after: a } ); + onlyBefore.push( { ...b[ 0 ], count: b.length } ); + continue; + } + const beforeStatuses = statusesOf( b ); + const afterStatuses = statusesOf( a ); + if ( beforeStatuses.join( ',' ) !== afterStatuses.join( ',' ) ) { + statusChanged.push( { key, beforeStatuses, afterStatuses } ); + } else if ( b.length !== a.length ) { + countChanged.push( { key, beforeCount: b.length, afterCount: a.length } ); } } - const onlyAfter = [ ...afterMap.entries() ] - .filter( ( [ key ] ) => ! beforeMap.has( key ) ) - .map( ( [ , r ] ) => r ); + for ( const [ key, a ] of afterGroups ) { + if ( ! beforeGroups.has( key ) ) { + onlyAfter.push( { ...a[ 0 ], count: a.length } ); + } + } - return { onlyBefore, onlyAfter, statusChanged }; + return { onlyBefore, onlyAfter, statusChanged, countChanged }; } /** diff --git a/tools/port-verification/src/diff.test.js b/tools/port-verification/src/diff.test.js index 5065b0219d73..f33fcb3ca946 100644 --- a/tools/port-verification/src/diff.test.js +++ b/tools/port-verification/src/diff.test.js @@ -12,6 +12,7 @@ import { flagOnSnapshot, flagOnSnapshotWith404, flagOnSnapshotWithGeometryShift, + flagOnSnapshotWithRetried404, flagOnSnapshotWithUnexpectedlyHiddenHeader, } from './fixtures.js'; @@ -86,10 +87,6 @@ describe( 'diffGeometry', () => { const [ result ] = diffGeometry( before, after ); assert.equal( result.status, 'hidden-changed' ); assert.deepEqual( result.details, [ 'visibility: visible -> hidden' ] ); - // The regression this guards: before the fix, this printed "width: 1120px -> 0px" and - // similar bogus deltas for every rect field, since getBoundingClientRect() on a - // display:none element is all zeros. - assert.ok( ! result.details.some( line => /width:|height:|^x:|^y:/.test( line ) ) ); } ); it( 'reports a hidden-to-visible transition the other direction', () => { @@ -146,6 +143,18 @@ describe( 'normalizeRequestKey', () => { assert.equal( normalizeRequestKey( a, options ), normalizeRequestKey( b, options ) ); } ); + it( 'strips _ajax_nonce too -- admin-ajax.php carries its nonce under that name', () => { + const a = { + url: 'https://site.test/wp-admin/admin-ajax.php?action=x&_ajax_nonce=aaa', + method: 'POST', + }; + const b = { + url: 'https://site.test/wp-admin/admin-ajax.php?action=x&_ajax_nonce=bbb', + method: 'POST', + }; + assert.equal( normalizeRequestKey( a ), normalizeRequestKey( b ) ); + } ); + it( 'falls back to a raw compare for a non-absolute URL', () => { assert.equal( normalizeRequestKey( { url: '/relative/path', method: 'GET' } ), @@ -161,6 +170,14 @@ describe( 'redactUrl', () => { assert.ok( redacted.includes( 'foo=1' ) ); } ); + it( 'strips an _ajax_nonce value as well', () => { + const redacted = redactUrl( + 'https://site.test/wp-admin/admin-ajax.php?action=x&_ajax_nonce=cafebabe5678' + ); + assert.ok( ! redacted.includes( 'cafebabe5678' ) ); + assert.ok( redacted.includes( 'action=x' ) ); + } ); + it( 'keeps a param that was not marked as ignorable', () => { const redacted = redactUrl( 'https://site.test/wp-json/x?type=post' ); assert.ok( redacted.includes( 'type=post' ) ); @@ -175,7 +192,12 @@ describe( 'diffNetwork', () => { it( 'finds no differences for identical request lists', () => { const requests = [ { url: 'https://site.test/a.js', method: 'GET', status: 200 } ]; const result = diffNetwork( requests, requests ); - assert.deepEqual( result, { onlyBefore: [], onlyAfter: [], statusChanged: [] } ); + assert.deepEqual( result, { + onlyBefore: [], + onlyAfter: [], + statusChanged: [], + countChanged: [], + } ); } ); it( 'catches a request that only fires after the flag flips (the design-tokens.css 404 case)', () => { @@ -195,8 +217,43 @@ describe( 'diffNetwork', () => { const after = [ { url: 'https://site.test/a.js', method: 'GET', status: 500 } ]; const result = diffNetwork( before, after ); assert.equal( result.statusChanged.length, 1 ); - assert.equal( result.statusChanged[ 0 ].before.status, 200 ); - assert.equal( result.statusChanged[ 0 ].after.status, 500 ); + assert.deepEqual( result.statusChanged[ 0 ].beforeStatuses, [ 200 ] ); + assert.deepEqual( result.statusChanged[ 0 ].afterStatuses, [ 500 ] ); + } ); + + it( 'does not let a later 200 on the same path hide an earlier 404', () => { + const url = 'https://site.test/design-tokens.css'; + const before = [ { url, method: 'GET', status: 200 } ]; + const after = [ + { url, method: 'GET', status: 404 }, + { url, method: 'GET', status: 200 }, + ]; + const result = diffNetwork( before, after ); + assert.equal( result.statusChanged.length, 1 ); + assert.deepEqual( result.statusChanged[ 0 ].beforeStatuses, [ 200 ] ); + assert.deepEqual( result.statusChanged[ 0 ].afterStatuses, [ 200, 404 ] ); + } ); + + it( 'reports a request that fires a different number of times', () => { + const url = 'https://site.test/wp-json/jetpack/v4/settings'; + const before = [ 1, 2, 3 ].map( () => ( { url, method: 'GET', status: 200 } ) ); + const after = [ { url, method: 'GET', status: 200 } ]; + const result = diffNetwork( before, after ); + assert.deepEqual( result.countChanged, [ + { key: 'GET /wp-json/jetpack/v4/settings', beforeCount: 3, afterCount: 1 }, + ] ); + assert.equal( result.statusChanged.length, 0 ); + } ); + + it( 'counts repeats of a request that fires on one side only', () => { + const url = 'https://site.test/legacy.js'; + const before = [ + { url, method: 'GET', status: 200 }, + { url, method: 'GET', status: 200 }, + ]; + const result = diffNetwork( before, [] ); + assert.equal( result.onlyBefore.length, 1 ); + assert.equal( result.onlyBefore[ 0 ].count, 2 ); } ); it( 'catches a request that disappears after the flag flips', () => { @@ -217,6 +274,7 @@ describe( 'diffSnapshots (fixtures)', () => { assert.deepEqual( network.onlyBefore, [] ); assert.deepEqual( network.onlyAfter, [] ); assert.deepEqual( network.statusChanged, [] ); + assert.deepEqual( network.countChanged, [] ); const root = geometry.find( g => g.key === 'root' ); assert.equal( root.status, 'changed' ); @@ -237,6 +295,13 @@ describe( 'diffSnapshots (fixtures)', () => { assert.equal( network.onlyAfter[ 0 ].status, 404 ); } ); + it( 'still flags the 404 when the same path is refetched successfully right after', () => { + const { network } = diffSnapshots( flagOffSnapshot(), flagOnSnapshotWithRetried404() ); + assert.equal( network.onlyAfter.length, 1 ); + assert.match( network.onlyAfter[ 0 ].url, /design-tokens\.css/ ); + assert.equal( network.onlyAfter[ 0 ].count, 2 ); + } ); + it( 'flags a #wpbody-content geometry shift beyond tolerance', () => { const { geometry } = diffSnapshots( flagOffSnapshot(), flagOnSnapshotWithGeometryShift() ); const finding = geometry.find( g => g.key === 'wpbodyContent' ); diff --git a/tools/port-verification/src/fixtures.js b/tools/port-verification/src/fixtures.js index 402bd7be74a1..e193a6e288af 100644 --- a/tools/port-verification/src/fixtures.js +++ b/tools/port-verification/src/fixtures.js @@ -5,9 +5,7 @@ * matching the two real bugs JETPACK-2573 cites from the My Jetpack pilot. */ -/** - * - */ +/** The flag-off baseline every other fixture is built from. */ export function flagOffSnapshot() { return { meta: { @@ -81,8 +79,7 @@ export function flagOffSnapshot() { /** * A clean port: the page root shifts by the one accepted 8px stage-gutter inset, and - * #wpfooter goes `display: none` (boot hides it by design -- see - * projects/js-packages/base-styles/admin-page-layout.scss). Nothing else moves. + * #wpfooter goes hidden (which boot does by design -- see selectors.js). Nothing else moves. */ export function flagOnSnapshot() { const snapshot = flagOffSnapshot(); @@ -91,7 +88,6 @@ export function flagOnSnapshot() { ...snapshot.geometry.root, rect: { x: 8, y: 8, width: 1264, height: 884 }, }; - // A real display:none collapses getBoundingClientRect() to all zeros. snapshot.geometry.footer = { ...snapshot.geometry.footer, hidden: true, @@ -116,6 +112,20 @@ export function flagOnSnapshotWith404() { return snapshot; } +/** + * The same 404, but on a path that is then fetched again successfully. Keying the diff by + * method + path alone would let the 200 overwrite the 404 and report nothing. + */ +export function flagOnSnapshotWithRetried404() { + const snapshot = flagOnSnapshot(); + const url = 'https://example.jurassic.ninja/wp-content/plugins/jetpack/design-tokens.css'; + snapshot.network.push( + { url, method: 'GET', status: 404, resourceType: 'stylesheet' }, + { url, method: 'GET', status: 200, resourceType: 'stylesheet' } + ); + return snapshot; +} + /** Reproduces a real geometry regression: #wpbody-content grows 12px wider than trunk. */ export function flagOnSnapshotWithGeometryShift() { const snapshot = flagOnSnapshot(); @@ -127,9 +137,9 @@ export function flagOnSnapshotWithGeometryShift() { } /** - * An UNEXPECTED visibility regression: the header goes `display: none` too. Unlike the - * footer, `header` has no `allowHidden` in `DEFAULT_GEOMETRY_TARGETS`, so this must surface - * as a finding. + * An UNEXPECTED visibility regression: the header goes hidden too. Unlike the footer, + * `header` has no `allowHidden` in `DEFAULT_GEOMETRY_TARGETS`, so this must surface as a + * finding. */ export function flagOnSnapshotWithUnexpectedlyHiddenHeader() { const snapshot = flagOnSnapshot(); diff --git a/tools/port-verification/src/options.js b/tools/port-verification/src/options.js new file mode 100644 index 000000000000..fa4a9de19975 --- /dev/null +++ b/tools/port-verification/src/options.js @@ -0,0 +1,68 @@ +/** + * CLI option parsing, kept out of bin/verify-port.js so it can be unit-tested -- importing + * the entry point would run `main()`. + */ + +import { parseArgs } from 'util'; +import { DEFAULT_IGNORED_QUERY_PARAMS, DEFAULT_TOLERANCE_PX } from './selectors.js'; + +export const OPTION_SPEC = { + url: { type: 'string' }, + flag: { type: 'string' }, + user: { type: 'string' }, + pass: { type: 'string' }, + 'control-selector': { type: 'string' }, + 'wait-selector': { type: 'string' }, + tolerance: { type: 'string' }, + 'ignore-query-param': { type: 'string', multiple: true, default: [] }, + out: { type: 'string' }, + before: { type: 'string' }, + after: { type: 'string' }, + headed: { type: 'boolean', default: false }, + help: { type: 'boolean', default: false }, +}; + +/** + * An unparseable value must not reach the diff as `NaN`: every `delta > NaN` is false, so + * step 2 would report OK for every target however far it moved. + * + * @param {string|undefined} raw + * @return {number} + */ +function parseTolerance( raw ) { + if ( raw === undefined ) { + return DEFAULT_TOLERANCE_PX; + } + const value = Number( raw ); + if ( ! Number.isFinite( value ) || value < 0 ) { + throw new Error( `--tolerance must be a non-negative number of pixels, got "${ raw }".` ); + } + return value; +} + +/** + * @param {string[]} argv - Everything after the subcommand. + * @param {object} [env] - Defaults to `process.env`. + * @return {object} Parsed options, camel-cased from `OPTION_SPEC`'s kebab-case keys. + */ +export function parseOptions( argv, env = process.env ) { + const { values } = parseArgs( { args: argv, options: OPTION_SPEC, allowPositionals: false } ); + const extraIgnoreParams = values[ 'ignore-query-param' ]; + return { + url: values.url, + flag: values.flag, + username: values.user ?? env.WP_ADMIN_USER, + password: values.pass ?? env.WP_ADMIN_PASS, + controlSelector: values[ 'control-selector' ], + waitForSelector: values[ 'wait-selector' ], + tolerancePx: parseTolerance( values.tolerance ), + ignoreQueryParams: extraIgnoreParams.length + ? [ ...DEFAULT_IGNORED_QUERY_PARAMS, ...extraIgnoreParams ] + : undefined, + out: values.out, + before: values.before, + after: values.after, + headless: ! values.headed, + help: values.help, + }; +} diff --git a/tools/port-verification/src/options.test.js b/tools/port-verification/src/options.test.js new file mode 100644 index 000000000000..217aa6e32bde --- /dev/null +++ b/tools/port-verification/src/options.test.js @@ -0,0 +1,73 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { parseOptions } from './options.js'; +import { DEFAULT_IGNORED_QUERY_PARAMS, DEFAULT_TOLERANCE_PX } from './selectors.js'; + +describe( 'parseOptions', () => { + it( 'camel-cases the kebab-case flags', () => { + const options = parseOptions( + [ + '--url', + 'https://site.test/wp-admin/', + '--control-selector', + '.x', + '--wait-selector', + '#y', + ], + {} + ); + assert.equal( options.url, 'https://site.test/wp-admin/' ); + assert.equal( options.controlSelector, '.x' ); + assert.equal( options.waitForSelector, '#y' ); + } ); + + it( 'defaults the tolerance and inverts --headed into headless', () => { + const options = parseOptions( [], {} ); + assert.equal( options.tolerancePx, DEFAULT_TOLERANCE_PX ); + assert.equal( options.headless, true ); + assert.equal( parseOptions( [ '--headed' ], {} ).headless, false ); + } ); + + it( 'accepts a numeric tolerance', () => { + assert.equal( parseOptions( [ '--tolerance', '4' ], {} ).tolerancePx, 4 ); + assert.equal( parseOptions( [ '--tolerance', '0' ], {} ).tolerancePx, 0 ); + } ); + + it( 'rejects a tolerance that is not a number, instead of passing NaN to the diff', () => { + // `delta > NaN` is false for every delta, so accepting one would report OK for every target. + for ( const bad of [ '8px', '0,5', 'wide', '-1' ] ) { + assert.throws( () => parseOptions( [ '--tolerance', bad ], {} ), /--tolerance/ ); + } + } ); + + it( 'falls back to WP_ADMIN_USER / WP_ADMIN_PASS, and prefers the flags', () => { + const env = { WP_ADMIN_USER: 'envuser', WP_ADMIN_PASS: 'envpass' }; + const fromEnv = parseOptions( [], env ); + assert.equal( fromEnv.username, 'envuser' ); + assert.equal( fromEnv.password, 'envpass' ); + + const fromFlags = parseOptions( [ '--user', 'flaguser', '--pass', 'flagpass' ], env ); + assert.equal( fromFlags.username, 'flaguser' ); + assert.equal( fromFlags.password, 'flagpass' ); + } ); + + it( 'leaves ignoreQueryParams undefined unless --ignore-query-param is given', () => { + assert.equal( parseOptions( [], {} ).ignoreQueryParams, undefined ); + } ); + + it( 'appends repeated --ignore-query-param values to the defaults', () => { + const options = parseOptions( + [ '--ignore-query-param', 'cb', '--ignore-query-param', 'rand' ], + {} + ); + assert.deepEqual( options.ignoreQueryParams, [ + ...DEFAULT_IGNORED_QUERY_PARAMS, + 'cb', + 'rand', + ] ); + } ); + + it( 'rejects an unknown flag rather than ignoring it', () => { + assert.throws( () => parseOptions( [ '--bogus' ], {} ) ); + } ); +} ); diff --git a/tools/port-verification/src/report.js b/tools/port-verification/src/report.js index 2aaa79a24e0f..31d79e52f431 100644 --- a/tools/port-verification/src/report.js +++ b/tools/port-verification/src/report.js @@ -43,6 +43,29 @@ function geometryRow( { key, label, status, before, details }, targets ) { return `| ${ label } | ${ effectiveStatus } | ${ detailText } |`; } +/** + * Targets that produced no diff entry at all, because their selector matched in neither + * capture. Without a row the report reads the same as a passing check -- most often when + * --control-selector was left off, so step 2's box model was never measured. + * + * @param {object[]} geometry - From `diffGeometry()`. + * @param {object} targets - Selector config. + * @return {Array<{key: string, row: string, required: boolean}>} + */ +function unmeasuredTargets( geometry, targets ) { + const measured = new Set( geometry.map( g => g.key ) ); + return Object.entries( targets ) + .filter( ( [ key ] ) => ! measured.has( key ) ) + .map( ( [ key, target ] ) => { + const required = target.required ?? false; + const reason = target.selector + ? 'selector matched in neither capture' + : 'no selector given (--control-selector)'; + const status = required ? 'NOT MEASURED' : 'skipped (optional)'; + return { key, required, row: `| ${ target.label } | ${ status } | ${ reason } |` }; + } ); +} + /** * @param {object[]} requests * @param {object} [options] - Passed to `redactUrl()`, so displayed URLs match the ignore list used for matching. @@ -53,12 +76,15 @@ function requestList( requests, options ) { return ' - none'; } return requests - .map( r => ` - \`${ r.method } ${ redactUrl( r.url, options ) }\` -> ${ r.status }` ) + .map( r => { + const repeats = r.count > 1 ? ` (fired ${ r.count }x)` : ''; + return ` - \`${ r.method } ${ redactUrl( r.url, options ) }\` -> ${ r.status }${ repeats }`; + } ) .join( '\n' ); } /** - * @param {{key: string, before: object, after: object}[]} changes - From `diffNetwork().statusChanged`. + * @param {{key: string, beforeStatuses: number[], afterStatuses: number[]}[]} changes - From `diffNetwork().statusChanged`. * @return {string} One Markdown bullet list line per changed request, or a single "none" line. */ function statusChangeList( changes ) { @@ -66,7 +92,23 @@ function statusChangeList( changes ) { return ' - none'; } return changes - .map( c => ` - \`${ c.key }\`: ${ c.before.status } -> ${ c.after.status }` ) + .map( + c => + ` - \`${ c.key }\`: ${ c.beforeStatuses.join( ', ' ) } -> ${ c.afterStatuses.join( ', ' ) }` + ) + .join( '\n' ); +} + +/** + * @param {{key: string, beforeCount: number, afterCount: number}[]} changes - From `diffNetwork().countChanged`. + * @return {string} One Markdown bullet list line per request, or a single "none" line. + */ +function countChangeList( changes ) { + if ( changes.length === 0 ) { + return ' - none'; + } + return changes + .map( c => ` - \`${ c.key }\`: fired ${ c.beforeCount }x -> ${ c.afterCount }x` ) .join( '\n' ); } @@ -88,20 +130,25 @@ export function formatReport( options = {} ) { const { geometry, network } = diffResult; - const geometryFindings = geometry.filter( g => { - if ( g.status === 'changed' ) { - return true; - } - if ( g.status === 'missing' ) { - return targets[ g.key ]?.required ?? false; - } - if ( g.status === 'hidden-changed' ) { - return ! ( targets[ g.key ]?.allowHidden ?? false ); - } - return false; - } ).length; + const unmeasured = unmeasuredTargets( geometry, targets ); + const geometryFindings = + geometry.filter( g => { + if ( g.status === 'changed' ) { + return true; + } + if ( g.status === 'missing' ) { + return targets[ g.key ]?.required ?? false; + } + if ( g.status === 'hidden-changed' ) { + return ! ( targets[ g.key ]?.allowHidden ?? false ); + } + return false; + } ).length + unmeasured.filter( u => u.required ).length; const networkFindings = - network.onlyBefore.length + network.onlyAfter.length + network.statusChanged.length; + network.onlyBefore.length + + network.onlyAfter.length + + network.statusChanged.length + + network.countChanged.length; const lines = [ '## Port verification -- steps 2 & 3 (JETPACK-2685)', @@ -116,6 +163,7 @@ export function formatReport( '| Element | Status | Details |', '| --- | --- | --- |', ...geometry.map( g => geometryRow( g, targets ) ), + ...unmeasured.map( u => u.row ), '', '### Step 3 -- network panel', '', @@ -125,6 +173,8 @@ export function formatReport( requestList( network.onlyAfter, options ), `- Status code changed (${ network.statusChanged.length }):`, statusChangeList( network.statusChanged ), + `- Request count changed (${ network.countChanged.length }):`, + countChangeList( network.countChanged ), '', '### Summary', '', diff --git a/tools/port-verification/src/report.test.js b/tools/port-verification/src/report.test.js index 87c81034004d..5cc038bf7660 100644 --- a/tools/port-verification/src/report.test.js +++ b/tools/port-verification/src/report.test.js @@ -6,9 +6,11 @@ import { flagOnSnapshot, flagOnSnapshotWith404, flagOnSnapshotWithGeometryShift, + flagOnSnapshotWithRetried404, flagOnSnapshotWithUnexpectedlyHiddenHeader, } from './fixtures.js'; import { formatReport } from './report.js'; +import { DEFAULT_GEOMETRY_TARGETS } from './selectors.js'; describe( 'formatReport', () => { it( 'includes the page, flag and capture timestamps in the header', () => { @@ -43,7 +45,6 @@ describe( 'formatReport', () => { /\| Footer \(#wpfooter\) \| OK \(hidden by design\) \| visibility: visible -> hidden \|/ ); assert.doesNotMatch( report, /Footer \(#wpfooter\) \| CHANGED/ ); - // The bug this guards: before the fix this row showed bogus px deltas like "width: 1120px -> 0px". assert.doesNotMatch( report, /width: 1120px -> 0px/ ); } ); @@ -91,10 +92,16 @@ describe( 'formatReport', () => { assert.match( report, /Only with flag off \(0\):\n {2}- none/ ); } ); - it( 'summary counts findings, excluding the missing-but-optional control', () => { + it( 'summary counts nothing when the only configured target is the optional control', () => { const before = { geometry: {}, network: [] }; const after = { geometry: {}, network: [] }; - const report = formatReport( diffSnapshots( before, after ), { url: 'x' } ); + const report = formatReport( + diffSnapshots( before, after ), + { url: 'x' }, + { + control: DEFAULT_GEOMETRY_TARGETS.control, + } + ); assert.match( report, /0 geometry finding\(s\), 0 network finding\(s\)\./ ); } ); @@ -104,7 +111,13 @@ describe( 'formatReport', () => { network: [], }; const after = { geometry: {}, network: [] }; - const report = formatReport( diffSnapshots( before, after ), { url: 'x' } ); + const report = formatReport( + diffSnapshots( before, after ), + { url: 'x' }, + { + header: DEFAULT_GEOMETRY_TARGETS.header, + } + ); assert.match( report, /\| Header \(#wpadminbar\) \| MISSING \|/ ); assert.match( report, /1 geometry finding\(s\)/ ); } ); @@ -115,8 +128,64 @@ describe( 'formatReport', () => { network: [], }; const after = { geometry: {}, network: [] }; - const report = formatReport( diffSnapshots( before, after ), { url: 'x' } ); + const report = formatReport( + diffSnapshots( before, after ), + { url: 'x' }, + { + control: DEFAULT_GEOMETRY_TARGETS.control, + } + ); assert.match( report, /\| Control \| ok \(not present, optional\) \|/ ); assert.match( report, /0 geometry finding\(s\)/ ); } ); + + it( 'says the control was skipped rather than leaving its row out', () => { + const before = flagOffSnapshot(); + const after = flagOnSnapshot(); + delete before.geometry.control; + delete after.geometry.control; + const report = formatReport( diffSnapshots( before, after ), { url: 'x' } ); + assert.match( + report, + /\| Control \| skipped \(optional\) \| no selector given \(--control-selector\) \|/ + ); + // Only root's accepted inset; a skipped optional target is not a finding. + assert.match( report, /1 geometry finding\(s\)/ ); + } ); + + it( 'counts a required target that matched in neither capture as NOT MEASURED', () => { + const before = flagOffSnapshot(); + const after = flagOnSnapshot(); + delete before.geometry.header; + delete after.geometry.header; + const report = formatReport( diffSnapshots( before, after ), { url: 'x' } ); + assert.match( + report, + /\| Header \(#wpadminbar\) \| NOT MEASURED \| selector matched in neither capture \|/ + ); + assert.match( report, /2 geometry finding\(s\)/ ); + } ); + + it( 'renders a status-code change as the two status lists', () => { + const before = flagOffSnapshot(); + const after = flagOnSnapshotWithRetried404(); + before.network.push( { + url: 'https://example.jurassic.ninja/wp-content/plugins/jetpack/design-tokens.css', + method: 'GET', + status: 200, + resourceType: 'stylesheet', + } ); + const report = formatReport( diffSnapshots( before, after ), { url: 'x' } ); + assert.match( report, /Status code changed \(1\):/ ); + assert.match( report, /design-tokens\.css`: 200 -> 200, 404/ ); + } ); + + it( 'reports a request that fired a different number of times', () => { + const before = flagOffSnapshot(); + const after = flagOnSnapshot(); + before.network.push( { ...before.network[ 0 ] } ); + const report = formatReport( diffSnapshots( before, after ), { url: 'x' } ); + assert.match( report, /Request count changed \(1\):/ ); + assert.match( report, /dashboard\.js`: fired 2x -> 1x/ ); + } ); } ); diff --git a/tools/port-verification/src/selectors.js b/tools/port-verification/src/selectors.js index f169e9121801..185334221826 100644 --- a/tools/port-verification/src/selectors.js +++ b/tools/port-verification/src/selectors.js @@ -1,19 +1,15 @@ /** - * Default targets for step 2 (JETPACK-2685): the WP admin frame elements that must - * stay put when a wp-build port is only a chassis swap. `#wpwrap`, `#wpadminbar` and - * `#wpfooter` are core wp-admin markup present on every page, flag on or off, so they - * catch the port shifting the frame around its own content -- not the content itself. + * Default step-2 targets (JETPACK-2685): core wp-admin frame markup, present whether the + * port's flag is off or on, so a diff catches the frame moving rather than its content. * - * `control` has no default: a representative interactive control (a button, a toggle) - * is page-specific. Pass one with --control-selector. + * `control` is page-specific -- pass one with --control-selector. */ export const DEFAULT_GEOMETRY_TARGETS = { root: { label: 'Page root (#wpwrap)', selector: '#wpwrap', required: true }, wpbodyContent: { label: '#wpbody-content', selector: '#wpbody-content', required: true }, header: { label: 'Header (#wpadminbar)', selector: '#wpadminbar', required: true }, - // allowHidden: boot's admin-page-layout.scss sets `#wpfooter { display: none }` by design - // (replaced with a pinned JetpackFooter inside the layout), so going hidden on flag-on is - // expected, not a finding -- see projects/js-packages/base-styles/admin-page-layout.scss. + // allowHidden: boot hides #wpfooter by design, replacing it with a pinned JetpackFooter + // inside the layout -- see projects/js-packages/base-styles/admin-page-layout.scss. footer: { label: 'Footer (#wpfooter)', selector: '#wpfooter', required: true, allowHidden: true }, control: { label: 'Control', selector: null, required: false }, }; @@ -22,7 +18,14 @@ export const DEFAULT_GEOMETRY_TARGETS = { // without the port having changed anything. Kept short and specific on purpose: a generic // name (a `v` or `t` param) can carry real state -- e.g. an API version or a tab filter -- // that step 3 exists to catch. --ignore-query-param adds more for a site that needs them. -export const DEFAULT_IGNORED_QUERY_PARAMS = [ '_wpnonce', 'ver', '_', '_locale' ]; +export const DEFAULT_IGNORED_QUERY_PARAMS = [ + '_wpnonce', + '_ajax_nonce', + '_nonce', + 'ver', + '_', + '_locale', +]; // Below this, a geometry delta is rounding noise, not a real shift. export const DEFAULT_TOLERANCE_PX = 0.5; From 78196a2de97cc7626c4b3a1b92794460e3898992 Mon Sep 17 00:00:00 2001 From: Christian Gastrell Date: Tue, 22 Sep 2026 09:15:58 -0300 Subject: [PATCH 4/7] Address dhasilva review: keep stdout for the report, enforce what was asked for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three cases where the tool produced a clean result it had not earned: * The prompt and progress lines went to stdout, so the documented `run … > report.md` wrote the "flip the flag" prompt into the report and hid it from the terminal. stdout now carries the report alone. * `run` accepted a bare Enter, so EOF (a pipeline, a non-TTY shell) exited 0 with no report, and a keypress buffered during the first capture answered the prompt early and captured flag-off twice. It now refuses a non-TTY, asks for the flag name, and fails if input closes first. * The report judged every run against the default targets, where `control` is optional and has no selector. A control that a port renamed away read as "ok (not present, optional)". Captures now record the targets they used, and a control with a selector is required. Also from that review: record `requestfailed`, so a bad host or a CSP block is not invisible; use `checkVisibility` so an ancestor's `display:none` is a visibility change rather than four px deltas; report when a control selector matches more than one element; keep rects unrounded so a sub-pixel tolerance means something; assert the landed URL still matches the requested path, with `--autologin-url` for hosts whose link logs you in; add `--load-state` for a page whose requests never settle; apply `--wait-selector` to the flag-on capture only and require its absence flag-off, which proves the flip landed; keep the host in the request key and drop per-event hosts such as `pixel.wp.com`; report per-side request totals and count an empty side; warn when the two snapshots are of different pages. Docs and wiring: `.gitignore` now matches the filenames the README uses; the colour-scheme regression is #51619, not #52096; root `composer.json` `test-js` runs these tests, the hook `tools/performance` already used. `capture.js` is no longer untested. `test/stub-wp-admin.js` is a `node:http` stub of wp-login.php, a Dashboard and one flag-dependent admin page, and `pnpm test:smoke` drives real headless Chromium against it — 11 tests covering login, the buffer clear, both redirect guards, ancestor-aware hidden detection and the failed-request path. It skips itself when no browser is installed. The README's claim that this needed a GPU was wrong. 70 unit tests, up from 55, plus the 11 smoke tests. Co-Authored-By: Claude Opus 5 (1M context) --- composer.json | 3 +- tools/port-verification/.gitignore | 1 + tools/port-verification/README.md | 60 +++++-- tools/port-verification/bin/verify-port.js | 82 +++++++-- tools/port-verification/package.json | 2 + tools/port-verification/src/capture.js | 108 +++++++---- tools/port-verification/src/diff.js | 83 ++++++++- tools/port-verification/src/diff.test.js | 57 +++++- tools/port-verification/src/options.js | 30 +++- tools/port-verification/src/options.test.js | 26 ++- tools/port-verification/src/report.js | 27 ++- tools/port-verification/src/report.test.js | 75 +++++++- tools/port-verification/src/selectors.js | 4 + .../test/capture.smoke.test.js | 169 ++++++++++++++++++ tools/port-verification/test/stub-wp-admin.js | 98 ++++++++++ 15 files changed, 740 insertions(+), 85 deletions(-) create mode 100644 tools/port-verification/test/capture.smoke.test.js create mode 100644 tools/port-verification/test/stub-wp-admin.js diff --git a/composer.json b/composer.json index f42fdd4a19b9..306746818856 100644 --- a/composer.json +++ b/composer.json @@ -70,7 +70,8 @@ ], "test-js": [ "cd tools/cli && pnpm test", - "cd tools/performance && pnpm test:unit" + "cd tools/performance && pnpm test:unit", + "cd tools/port-verification && pnpm test" ], "post-install-cmd": [ "cd tools/php-test-env && composer install" diff --git a/tools/port-verification/.gitignore b/tools/port-verification/.gitignore index cdcc4192deb4..1cb2211af332 100644 --- a/tools/port-verification/.gitignore +++ b/tools/port-verification/.gitignore @@ -1,3 +1,4 @@ node_modules/ +# Snapshots keep raw request URLs, nonces included -- only the report is redacted. *.snapshot.json *-report.md diff --git a/tools/port-verification/README.md b/tools/port-verification/README.md index 773c20e1b11d..ac8f2545bcf1 100644 --- a/tools/port-verification/README.md +++ b/tools/port-verification/README.md @@ -12,17 +12,23 @@ that rule stay manual -- see "Not covered" below. `box-sizing`, `font-size`). These are core wp-admin markup present whether the flag is off or on, so a diff catches the port shifting the frame around its content -- the failure mode JETPACK-2573 calls out: "a 4px shift is invisible in a screenshot and obvious in a number." - A target that goes `display: none` (e.g. `#wpfooter`, which boot hides by design -- see + A target that goes invisible (e.g. `#wpfooter`, which boot hides by design -- see `projects/js-packages/base-styles/admin-page-layout.scss`) is reported as a visibility - change, not a bogus zero-rect geometry shift. + change, not a bogus zero-rect geometry shift; the check walks ancestors, so a wrapper that + goes `display: none` counts too. Rects are stored unrounded, so `--tolerance` means + something below 1px. A control selector that matches more than one element is reported, + since each side then compares its own first match. - **Step 3, network.** Every request the page fires, matched flag-off to flag-on by method + path (nonces and cache-busting query params -- see `DEFAULT_IGNORED_QUERY_PARAMS` in `src/selectors.js` -- are stripped before matching and before display, so the report never quotes a live nonce). Reports requests that fired on one side only, requests whose set of status codes changed, and requests that fired a different number of times. This is what caught a `design-tokens.css` 404 and a renamed JITM message path in the My Jetpack pilot -- - both zero-pixel changes. The login and Dashboard page loads are dropped before the diff, so - only the page you pointed it at is compared. + both zero-pixel changes. Requests that never get a response -- a bad host, a refused + connection, a CSP block -- are recorded too, with status `0`. The login and Dashboard page + loads are dropped before the diff, so only the page you pointed it at is compared, and the + report states how many requests each side contributed so an empty capture cannot read as + agreement. The one difference JETPACK-2573 accepts is a uniform 8px inset from boot's stage gutter on the page root. The report says so in its footer; it does not try to auto-approve that one row. @@ -32,7 +38,7 @@ page root. The report says so in its footer; it does not try to auto-approve tha ``` cd tools/port-verification pnpm install -pnpm exec playwright install chromium +pnpm setup:browsers ``` ## Usage @@ -44,10 +50,15 @@ node bin/verify-port.js run \ --url https://.jurassic.ninja/wp-admin/admin.php?page= \ --flag \ --user admin --pass \ - --control-selector '.components-button' \ + --control-selector '.jetpack-backup__primary-action' \ > port-verification-report.md ``` +The report goes to stdout; progress and the flag prompt go to stderr, so redirecting stdout +keeps the prompt visible. `run` needs an interactive terminal and asks you to type the flag +name, not just press Enter -- a stray keypress buffered during the first capture would +otherwise answer the prompt and leave both captures on the same side of the flag. + It captures the page with the flag off, then pauses: ``` @@ -68,9 +79,12 @@ above). | `--flag` | Feature flag name, printed in the pause prompt and the report header. Optional. | | `--user` / `--pass` | wp-admin login. Falls back to `WP_ADMIN_USER` / `WP_ADMIN_PASS`. Each capture starts a fresh browser profile with no cookies, so omit these only when the URL authenticates by itself -- a Jurassic Ninja autologin link. Otherwise the capture aborts on landing at `wp-login.php`. | | `--control-selector` | CSS selector for the one control to box-model (step 2). Omit it and the report says the control was skipped. | -| `--wait-selector` | Extra selector to wait for after navigation, e.g. the boot mount, so a slow-hydrating page isn't captured mid-render. | +| `--wait-selector` | Selector that must be visible after navigation, e.g. the boot mount. In `run` it applies to the flag-on capture only, and the flag-off capture asserts the same selector is _absent_ -- which is what proves the flag actually flipped. | | `--tolerance` | Geometry tolerance in px. Default `0.5` -- rounding noise, not a real shift. A non-numeric value is rejected rather than silently disabling step 2. | | `--ignore-query-param` | Repeatable. Adds a query param to the default ignore list for step 3 matching and display (`_wpnonce`, `_ajax_nonce`, `_nonce`, `ver`, `_`, `_locale`). Use it for a site-specific volatile param -- generic names like `v` or `t` are deliberately not ignored by default, since they can carry real state (an API version, a tab filter). | +| `--autologin-url` | Visited before the target, for a host whose link logs you in (Jurassic Ninja's `?auto_login`). Use instead of `--user`/`--pass`. | +| `--load-state` | `load`, `domcontentloaded` or `networkidle` (default). Drop to `load` for a page with a request that never settles, as `tools/performance` had to for the Forms dashboard. | +| `--ignore-host` | Repeatable. Adds a host to the ignored list (`pixel.wp.com`). Traffic from these is dropped before the diff, because a per-event URL produces a new key on every load. | | `--out` | Write the report to a file (in addition to stdout). | | `--headed` | Run the browser headed, for watching the capture happen. | @@ -80,12 +94,16 @@ If pausing mid-command inside one process is awkward (e.g. flipping the flag nee terminal), capture and diff separately: ``` -node bin/verify-port.js capture --url --user admin --pass --out off.json +node bin/verify-port.js capture --url --user admin --pass --out off.snapshot.json # flip the flag -node bin/verify-port.js capture --url --user admin --pass --out on.json -node bin/verify-port.js diff --before off.json --after on.json --out report.md +node bin/verify-port.js capture --url --user admin --pass --out on.snapshot.json +node bin/verify-port.js diff --before off.snapshot.json --after on.snapshot.json \ + --out port-verification-report.md ``` +Keep the `.snapshot.json` suffix: `.gitignore` matches it, and a snapshot stores raw request +URLs, nonces included. Only the report is redacted. + ## Sample output ``` @@ -108,6 +126,8 @@ node bin/verify-port.js diff --before off.json --after on.json --out report.md ### Step 3 -- network panel +Compared 14 request(s) with the flag off against 15 with it on. + - Only with flag off (0): - none - Only with flag on (1): @@ -132,8 +152,8 @@ the page, not a diff. Steps 5 and 6 are the ones most worth automating next: both known post-ship regressions on already-ported dashboards ([#51963](https://github.com/Automattic/jetpack/pull/51963), -[#52096](https://github.com/Automattic/jetpack/pull/52096)) were in RTL and colour-scheme, not -in anything steps 2 or 3 here would have caught. The geometry side extends cheaply: capture the +[#51619](https://github.com/Automattic/jetpack/pull/51619)) were in RTL and colour-scheme, +which steps 2 and 3 do not cover as written. The geometry side extends cheaply: capture the same targets with the page in `dir="rtl"` (content column must not clip under the admin menu) and with a non-default admin colour scheme active (the backdrop must follow the menu colour, per `class-wp-build-admin-frame.php`), and diff against the LTR / default-scheme capture instead @@ -144,10 +164,16 @@ network check. ## Testing ``` -pnpm test +pnpm test # src/*.test.js -- diff, report and option parsing, no browser +pnpm test:smoke # test/*.test.js -- capture.js against a local wp-admin stub ``` -Runs `src/*.test.js` against the fixtures in `src/fixtures.js` -- the diff logic (`diff.js`) and -report formatting (`report.js`), which need no browser. `capture.js` (the Playwright half) is -not unit-tested: it needs a real Chromium against a real site, which this repo's sandbox can't -run (no GPU). Verify it by hand against a Jurassic Ninja site. +`pnpm test` runs in CI through the root `composer.json` `test-js` script. `pnpm test:smoke` +drives a real headless Chromium against `test/stub-wp-admin.js`, a ~100-line `node:http` +server that serves wp-login.php, a Dashboard and one admin page whose markup changes with a +query flag. It covers login, the post-login buffer clear, the redirect guards, ancestor-aware +hidden detection and the failed-request path. It skips itself when no Chromium is installed, +so it stays out of `pnpm test`. + +What neither covers: the default selectors against a real wp-admin page. Verify that by hand +against a Jurassic Ninja site. diff --git a/tools/port-verification/bin/verify-port.js b/tools/port-verification/bin/verify-port.js index 255a6548e116..820bf17ba09e 100644 --- a/tools/port-verification/bin/verify-port.js +++ b/tools/port-verification/bin/verify-port.js @@ -11,7 +11,7 @@ import readline from 'readline/promises'; import { diffSnapshots } from '../src/diff.js'; import { parseOptions } from '../src/options.js'; import { formatReport } from '../src/report.js'; -import { DEFAULT_IGNORED_QUERY_PARAMS } from '../src/selectors.js'; +import { DEFAULT_IGNORED_HOSTS, DEFAULT_IGNORED_QUERY_PARAMS } from '../src/selectors.js'; // Loaded lazily (only by `capture` and `run`) so `--help` and `diff` -- which need no // browser -- still work without Playwright installed. @@ -22,10 +22,11 @@ async function loadCapturePage() { const USAGE = `Usage: verify-port run --url [--flag ] [--user --pass ] [--control-selector ] [--wait-selector ] - [--tolerance ] [--out ] + [--load-state ] [--tolerance ] [--out ] verify-port capture --url [--user --pass ] - [--control-selector ] [--wait-selector ] + [--autologin-url ] [--control-selector ] + [--wait-selector ] [--load-state ] --out verify-port diff --before --after @@ -34,6 +35,8 @@ const USAGE = `Usage: Credentials also read from WP_ADMIN_USER / WP_ADMIN_PASS. --ignore-query-param (repeatable) adds to the default ignored list (${ DEFAULT_IGNORED_QUERY_PARAMS.join( ', ' ) }) for step 3's request matching and display. +--ignore-host (repeatable) adds to the ignored hosts (${ DEFAULT_IGNORED_HOSTS.join( ', ' ) }). +The report goes to stdout; progress and prompts go to stderr, so '> report.md' is safe. See README.md for the full walkthrough, including how to flip the flag between captures.`; /** @@ -56,7 +59,7 @@ async function runCapture( options ) { const capturePage = await loadCapturePage(); const snapshot = await capturePage( options ); writeJson( options.out, snapshot ); - console.log( `Captured ${ options.url } -> ${ options.out }` ); + console.error( `Captured ${ options.url } -> ${ options.out }` ); } /** @@ -80,22 +83,26 @@ function runDiff( options ) { */ function printReport( before, after, options ) { const diffResult = diffSnapshots( before, after, options ); + // The captures know which selectors they were actually given; the defaults call every + // control optional, which would let a control that vanished pass as "not present". + const targets = after.meta?.targets ?? before.meta?.targets ?? undefined; const report = formatReport( diffResult, { url: after.meta?.url ?? before.meta?.url ?? options.url, + beforeUrl: before.meta?.url, + afterUrl: after.meta?.url, flag: options.flag, beforeCapturedAt: before.meta?.capturedAt, afterCapturedAt: after.meta?.capturedAt, }, - undefined, + targets, options ); if ( options.out ) { fs.writeFileSync( options.out, report + '\n' ); - console.log( `Report written to ${ options.out }` ); + console.error( `Report written to ${ options.out }` ); } - console.log( '' ); console.log( report ); } @@ -110,23 +117,62 @@ async function runFull( options ) { if ( ! options.url ) { throw new Error( '--url is required for `run`.' ); } + if ( ! process.stdin.isTTY ) { + throw new Error( + '`run` needs an interactive terminal for the flag prompt. Use `capture` twice and `diff`.' + ); + } const capturePage = await loadCapturePage(); - console.log( `Capturing with the flag OFF: ${ options.url }` ); - const before = await capturePage( options ); + // A --wait-selector names boot's mount, which exists only with the flag on. Requiring it + // flag-off would time out the first capture; requiring its absence proves the flip landed. + console.error( `Capturing with the flag OFF: ${ options.url }` ); + const before = await capturePage( { + ...options, + waitForSelector: undefined, + absentSelector: options.waitForSelector, + } ); - const rl = readline.createInterface( { input: process.stdin, output: process.stdout } ); - await rl.question( - `\nFlip ${ options.flag ?? 'the port flag' } ON on the site now, then press Enter to continue... ` - ); - rl.close(); + await confirmFlagFlipped( options.flag ); - console.log( `Capturing with the flag ON: ${ options.url }` ); - const after = await capturePage( options ); + console.error( `Capturing with the flag ON: ${ options.url }` ); + const after = await capturePage( { ...options, absentSelector: undefined } ); printReport( before, after, options ); } +/** + * Ask for a word rather than a bare Enter, so a stray keypress buffered during the first + * capture cannot answer the prompt and leave both captures on the same side of the flag. + * + * @param {string} [flag] - Flag name, which doubles as the confirmation word. + * @return {Promise} + */ +async function confirmFlagFlipped( flag ) { + const token = ( flag ?? 'on' ).toLowerCase(); + const rl = readline.createInterface( { input: process.stdin, output: process.stderr } ); + try { + const closedEarly = new Promise( ( _, reject ) => { + rl.once( 'close', () => + reject( new Error( 'Input closed before the flag was confirmed.' ) ) + ); + } ); + const answer = await Promise.race( [ + rl.question( + `\nFlip ${ flag ?? 'the port flag' } ON now, then type "${ token }" and press Enter: ` + ), + closedEarly, + ] ); + if ( answer.trim().toLowerCase() !== token ) { + throw new Error( + `Expected "${ token }", got "${ answer.trim() }" -- stopping rather than capturing the same state twice.` + ); + } + } finally { + rl.close(); + } +} + async function main() { const argv = process.argv.slice( 2 ); // A leading option (e.g. bare `--help`) has no subcommand; anything else does. @@ -135,7 +181,7 @@ async function main() { const options = parseOptions( hasCommand ? argv.slice( 1 ) : argv ); if ( options.help || ! command ) { - console.log( USAGE ); + ( options.help ? console.log : console.error )( USAGE ); process.exit( options.help ? 0 : 1 ); } @@ -147,7 +193,7 @@ async function main() { await runFull( options ); } else { console.error( `Unknown command: ${ command }\n` ); - console.log( USAGE ); + console.error( USAGE ); process.exit( 1 ); } } diff --git a/tools/port-verification/package.json b/tools/port-verification/package.json index 6537102b1ca9..92f42ad9b127 100644 --- a/tools/port-verification/package.json +++ b/tools/port-verification/package.json @@ -10,7 +10,9 @@ "verify-port": "./bin/verify-port.js" }, "scripts": { + "setup:browsers": "playwright install chromium", "test": "node --test src/*.test.js", + "test:smoke": "node --test test/*.test.js", "verify-port": "node bin/verify-port.js" }, "dependencies": { diff --git a/tools/port-verification/src/capture.js b/tools/port-verification/src/capture.js index 490fb9dbaf8e..8764abaa99b7 100644 --- a/tools/port-verification/src/capture.js +++ b/tools/port-verification/src/capture.js @@ -1,8 +1,7 @@ /** * Browser side of JETPACK-2685 steps 2 and 3: loads a page with Playwright and pulls a - * snapshot of its geometry and network traffic. Needs a real Chromium -- not covered by - * the unit tests in this directory, which exercise diff.js and report.js against fixtures - * instead. See README.md for how a reviewer runs this against a live site. + * snapshot of its geometry and network traffic. Needs a real Chromium; test/capture.smoke.test.js + * exercises it against a local stub of wp-admin. See README.md for a run against a live site. */ import { chromium } from 'playwright'; @@ -15,17 +14,18 @@ import { DEFAULT_GEOMETRY_TARGETS } from './selectors.js'; * @param {string} siteUrl * @param {string} username * @param {string} password + * @param {string} loadState * @return {Promise} */ -async function login( page, siteUrl, username, password ) { +async function login( page, siteUrl, username, password, loadState ) { const origin = new URL( siteUrl ).origin; - await page.goto( `${ origin }/wp-login.php`, { waitUntil: 'networkidle' } ); + await page.goto( `${ origin }/wp-login.php`, { waitUntil: loadState } ); await page.fill( '#user_login', username ); await page.fill( '#user_pass', password ); try { await Promise.all( [ - page.waitForURL( '**/wp-admin/**', { waitUntil: 'networkidle' } ), + page.waitForURL( '**/wp-admin/**', { waitUntil: loadState } ), page.click( '#wp-submit' ), ] ); // Confirm login landed in wp-admin (the Dashboard is the post-login screen) rather than @@ -39,40 +39,62 @@ async function login( page, siteUrl, username, password ) { } } +/** + * A redirect that is not wp-login.php is just as bad: the capture measures whatever it landed + * on, and two captures of the same wrong page report no differences at all. + * + * @param {import('playwright').Page} page + * @param {string} url - What was requested. + * @return {void} + */ +function assertLanded( page, url ) { + const landed = page.url(); + if ( landed.includes( 'wp-login.php' ) ) { + throw new Error( + `Landed on wp-login.php instead of ${ url } -- pass --user/--pass, or an --autologin-url.` + ); + } + const want = new URL( url ); + const got = new URL( landed ); + if ( + got.pathname !== want.pathname || + got.searchParams.get( 'page' ) !== want.searchParams.get( 'page' ) + ) { + throw new Error( `Landed on ${ landed } instead of ${ url } -- a redirect moved the capture.` ); + } +} + /** * Read one target's rect + computed style out of the page. Runs inside the browser via * `page.evaluate`, so it can only use DOM APIs -- no imports from this module reach it. * * @param {object} targetsArg - `{ [key]: { label, selector } }`, control's selector resolved by the caller. - * @return {object} `{ [key]: { label, hidden, rect, style } }` for every target whose selector matched. + * @return {object} `{ [key]: { label, hidden, matchCount, rect, style } }` for every target whose selector matched. */ /* c8 ignore start -- runs inside the browser context; exercised only by a live capture. */ function extractGeometryInPage( targetsArg ) { /** - * `display: none` (or `visibility: hidden`) collapses `getBoundingClientRect()` to all - * zeros. Recording the fact separately lets the diff report a visibility change instead - * of four bogus px deltas. + * `checkVisibility` walks ancestors, which a computed style on the element alone does not: + * a wrapper with `display: none` leaves the child's own `display` untouched while collapsing + * its rect to zeros, which would otherwise read as four px deltas. * * @param {Element} el * @return {boolean} */ function isHidden( el ) { - const cs = window.getComputedStyle( el ); - return cs.display === 'none' || cs.visibility === 'hidden'; + return ! el.checkVisibility( { visibilityProperty: true } ); } /** + * Unrounded: rounding here would make every delta a whole number, so any tolerance below + * 1px would behave exactly like 0. diff.js rounds for display. + * * @param {Element} el * @return {{x: number, y: number, width: number, height: number}} */ function readRect( el ) { const r = el.getBoundingClientRect(); - return { - x: Math.round( r.x ), - y: Math.round( r.y ), - width: Math.round( r.width ), - height: Math.round( r.height ), - }; + return { x: r.x, y: r.y, width: r.width, height: r.height }; } /** @@ -109,13 +131,17 @@ function extractGeometryInPage( targetsArg ) { if ( ! target.selector ) { continue; } - const el = document.querySelector( target.selector ); - if ( ! el ) { + const matches = document.querySelectorAll( target.selector ); + if ( matches.length === 0 ) { continue; } + const el = matches[ 0 ]; out[ key ] = { label: target.label, hidden: isHidden( el ), + // querySelector takes the first match, which can be a different element on each + // side; the diff reports anything above 1 rather than comparing silently. + matchCount: matches.length, rect: readRect( el ), style: readStyle( el, key === 'control' ), }; @@ -126,15 +152,18 @@ function extractGeometryInPage( targetsArg ) { /** * Capture one snapshot: navigate (logging in first if credentials are given), wait for - * the page to settle, then read geometry for `targets` and every response the target page + * the page to settle, then read geometry for `targets` and every request the target page * fired. * * @param {object} options * @param {string} options.url - Page to capture. * @param {string} [options.username] - wp-admin username; skips login if omitted. * @param {string} [options.password] - wp-admin password. + * @param {string} [options.autologinUrl] - Visited before the target, for a host whose link logs you in (Jurassic Ninja). * @param {string} [options.controlSelector] - CSS selector for the one control to box-model. Skipped if omitted. - * @param {string} [options.waitForSelector] - Extra selector to wait for (visible) after navigation, e.g. the boot mount when the flag is on. + * @param {string} [options.waitForSelector] - Selector that must be visible after navigation, e.g. the boot mount. + * @param {string} [options.absentSelector] - Selector that must NOT be present; proves the flag is off. + * @param {string} [options.loadState] - Playwright load state to wait for. Defaults to `networkidle`. * @param {object} [options.targets] - Selector config; defaults to `DEFAULT_GEOMETRY_TARGETS`. * @param {boolean} [options.headless] - Defaults to true. * @return {Promise<{meta: object, geometry: object, network: object[]}>} The captured snapshot. @@ -144,14 +173,19 @@ export async function capturePage( options ) { url, username, password, + autologinUrl, controlSelector, waitForSelector, + absentSelector, + loadState = 'networkidle', targets = DEFAULT_GEOMETRY_TARGETS, headless = true, } = options; + // A control that was asked for is required: without this the report calls a control that + // vanished between captures "optional", which is exactly the renaming a port produces. const resolvedTargets = controlSelector - ? { ...targets, control: { ...targets.control, selector: controlSelector } } + ? { ...targets, control: { ...targets.control, selector: controlSelector, required: true } } : targets; const browser = await chromium.launch( { headless } ); @@ -169,27 +203,41 @@ export async function capturePage( options ) { resourceType: request.resourceType(), } ); } ); + // A request that never gets a response -- bad host, refused connection, CSP block -- + // fires only here. Status 0 keeps it in the same shape as a response. + page.on( 'requestfailed', request => { + network.push( { + url: request.url(), + method: request.method(), + status: 0, + failure: request.failure()?.errorText, + resourceType: request.resourceType(), + } ); + } ); if ( username && password ) { - await login( page, url, username, password ); + await login( page, url, username, password, loadState ); + } else if ( autologinUrl ) { + await page.goto( autologinUrl, { waitUntil: loadState } ); } - // Step 3 compares the target page's requests. Drop everything wp-login.php and the + // Step 3 compares the target page's requests. Drop everything the login flow and the // post-login Dashboard fired, or their (partly non-deterministic) traffic is diffed too. network.length = 0; - await page.goto( url, { waitUntil: 'networkidle' } ); - if ( page.url().includes( 'wp-login.php' ) ) { + await page.goto( url, { waitUntil: loadState } ); + assertLanded( page, url ); + if ( absentSelector && ( await page.locator( absentSelector ).count() ) > 0 ) { throw new Error( - `Landed on wp-login.php instead of ${ url } -- pass --user/--pass, or use a URL that carries its own autologin token.` + `${ absentSelector } is present, but this capture expected it to be absent -- did the flag really change?` ); } if ( waitForSelector ) { await page.waitForSelector( waitForSelector, { state: 'visible' } ); - await page.waitForLoadState( 'networkidle' ); + await page.waitForLoadState( loadState ); } return { - meta: { url, capturedAt: new Date().toISOString() }, + meta: { url, capturedAt: new Date().toISOString(), targets: resolvedTargets }, geometry: await page.evaluate( extractGeometryInPage, resolvedTargets ), network, }; diff --git a/tools/port-verification/src/diff.js b/tools/port-verification/src/diff.js index affbc253b2e9..51806c91246d 100644 --- a/tools/port-verification/src/diff.js +++ b/tools/port-verification/src/diff.js @@ -3,7 +3,22 @@ * takes and returns plain data, so it can run against fixtures without Playwright. */ -import { DEFAULT_IGNORED_QUERY_PARAMS, DEFAULT_TOLERANCE_PX } from './selectors.js'; +import { + DEFAULT_IGNORED_HOSTS, + DEFAULT_IGNORED_QUERY_PARAMS, + DEFAULT_TOLERANCE_PX, +} from './selectors.js'; + +/** + * Rects are stored unrounded so a sub-pixel tolerance means something. Whole numbers still + * print as whole numbers. + * + * @param {number} value + * @return {string} + */ +function px( value ) { + return `${ Number.isInteger( value ) ? value : value.toFixed( 1 ) }px`; +} /** * Compare one geometry target's `rect` (numeric px fields) between before and after. @@ -26,7 +41,7 @@ function diffRect( before, after, tolerancePx ) { } const delta = Math.abs( a - b ); if ( delta > tolerancePx ) { - lines.push( `${ field }: ${ b }px -> ${ a }px (Δ${ delta.toFixed( 1 ) }px)` ); + lines.push( `${ field }: ${ px( b ) } -> ${ px( a ) } (Δ${ delta.toFixed( 1 ) }px)` ); } } return lines; @@ -53,7 +68,7 @@ function diffStyle( before, after, tolerancePx ) { if ( typeof b === 'number' && typeof a === 'number' ) { const delta = Math.abs( a - b ); if ( delta > tolerancePx ) { - lines.push( `${ prop }: ${ b }px -> ${ a }px (Δ${ delta.toFixed( 1 ) }px)` ); + lines.push( `${ prop }: ${ px( b ) } -> ${ px( a ) } (Δ${ delta.toFixed( 1 ) }px)` ); } continue; } @@ -76,6 +91,23 @@ function diffStyle( before, after, tolerancePx ) { * @param {number} [options.tolerancePx] - See `DEFAULT_TOLERANCE_PX`. * @return {Array} `{ key, label, status: 'ok'|'changed'|'missing'|'hidden-changed', details }`, one per target present on either side. */ +/** + * `querySelectorAll` found more than one element, so each side compared its own first match + * and the row may be measuring two different elements. + * + * @param {object} before + * @param {object} after + * @return {string[]} + */ +function ambiguousSelector( before, after ) { + const b = before.matchCount ?? 1; + const a = after.matchCount ?? 1; + if ( b > 1 || a > 1 ) { + return [ `selector matched ${ b } element(s) -> ${ a }; only the first is compared` ]; + } + return []; +} + export function diffGeometry( before = {}, after = {}, options = {} ) { const tolerancePx = options.tolerancePx ?? DEFAULT_TOLERANCE_PX; const keys = new Set( [ ...Object.keys( before ), ...Object.keys( after ) ] ); @@ -116,6 +148,7 @@ export function diffGeometry( before = {}, after = {}, options = {} ) { } const details = [ + ...ambiguousSelector( b, a ), ...diffRect( b.rect, a.rect, tolerancePx ), ...diffStyle( b.style, a.style, tolerancePx ), ]; @@ -152,7 +185,9 @@ export function normalizeRequestKey( request, options = {} ) { try { const parsed = stripQueryParams( new URL( request.url ), ignoreQueryParams ); parsed.searchParams.sort(); - return `${ request.method } ${ parsed.pathname }${ parsed.search }`; + // Host included: the same path on two hosts is not the same request, and a key that + // drops it cannot say which host a status change belongs to. + return `${ request.method } ${ parsed.host }${ parsed.pathname }${ parsed.search }`; } catch { // Not an absolute URL (e.g. already a bare path in a fixture). Compare as-is. return `${ request.method } ${ request.url }`; @@ -178,6 +213,29 @@ export function redactUrl( url, options = {} ) { } } +/** + * Drop traffic from hosts whose URLs are per-event by design, so they cannot flood both + * "only with flag off" and "only with flag on" with the same events under new keys. + * + * @param {object[]} requests + * @param {object} [options] + * @param {string[]} [options.ignoreHosts] - See `DEFAULT_IGNORED_HOSTS`. + * @return {object[]} + */ +function dropIgnoredHosts( requests, options = {} ) { + const ignoreHosts = options.ignoreHosts ?? DEFAULT_IGNORED_HOSTS; + if ( ignoreHosts.length === 0 ) { + return requests; + } + return requests.filter( request => { + try { + return ! ignoreHosts.includes( new URL( request.url ).host ); + } catch { + return true; + } + } ); +} + /** * Group requests by normalized key, keeping every occurrence rather than the last one: * a path fetched twice in one load can carry two different statuses, and collapsing them @@ -220,11 +278,13 @@ function statusesOf( group ) { * @param {Array} before - `[{ url, method, status, resourceType }]`. * @param {Array} after - Same shape, from the flag-on capture. * @param {object} [options] - * @return {{onlyBefore: object[], onlyAfter: object[], statusChanged: object[], countChanged: object[]}} + * @return {{onlyBefore: object[], onlyAfter: object[], statusChanged: object[], countChanged: object[], beforeTotal: number, afterTotal: number}} */ export function diffNetwork( before = [], after = [], options = {} ) { - const beforeGroups = groupByKey( before, options ); - const afterGroups = groupByKey( after, options ); + const keptBefore = dropIgnoredHosts( before, options ); + const keptAfter = dropIgnoredHosts( after, options ); + const beforeGroups = groupByKey( keptBefore, options ); + const afterGroups = groupByKey( keptAfter, options ); const onlyBefore = []; const onlyAfter = []; @@ -252,7 +312,14 @@ export function diffNetwork( before = [], after = [], options = {} ) { } } - return { onlyBefore, onlyAfter, statusChanged, countChanged }; + return { + onlyBefore, + onlyAfter, + statusChanged, + countChanged, + beforeTotal: keptBefore.length, + afterTotal: keptAfter.length, + }; } /** diff --git a/tools/port-verification/src/diff.test.js b/tools/port-verification/src/diff.test.js index f33fcb3ca946..db00a2061790 100644 --- a/tools/port-verification/src/diff.test.js +++ b/tools/port-verification/src/diff.test.js @@ -36,6 +36,23 @@ describe( 'diffGeometry', () => { assert.equal( result.status, 'ok' ); } ); + it( 'honours a sub-pixel tolerance, since rects are stored unrounded', () => { + const before = { root: { label: 'r', rect: { width: 100.4 } } }; + const after = { root: { label: 'r', rect: { width: 100.6 } } }; + assert.equal( diffGeometry( before, after, { tolerancePx: 0.5 } )[ 0 ].status, 'ok' ); + const [ tight ] = diffGeometry( before, after, { tolerancePx: 0.1 } ); + assert.equal( tight.status, 'changed' ); + assert.match( tight.details[ 0 ], /width: 100\.4px -> 100\.6px \(Δ0\.2px\)/ ); + } ); + + it( 'flags a control whose selector matched more than one element', () => { + const before = { control: { label: 'Control', matchCount: 2, rect: { width: 10 } } }; + const after = { control: { label: 'Control', matchCount: 1, rect: { width: 10 } } }; + const [ result ] = diffGeometry( before, after ); + assert.equal( result.status, 'changed' ); + assert.match( result.details[ 0 ], /matched 2 element\(s\) -> 1; only the first is compared/ ); + } ); + it( 'flags a rect delta beyond tolerance', () => { const before = { wpbodyContent: { label: 'x', rect: { width: 100 } } }; const after = { wpbodyContent: { label: 'x', rect: { width: 104 } } }; @@ -161,6 +178,13 @@ describe( 'normalizeRequestKey', () => { 'GET /relative/path' ); } ); + + it( 'keeps the host, so the same path on two hosts is two requests', () => { + const a = { url: 'https://site.test/boot.js', method: 'GET' }; + const b = { url: 'https://cdn.test/boot.js', method: 'GET' }; + assert.notEqual( normalizeRequestKey( a ), normalizeRequestKey( b ) ); + assert.match( normalizeRequestKey( a ), /site\.test/ ); + } ); } ); describe( 'redactUrl', () => { @@ -197,6 +221,8 @@ describe( 'diffNetwork', () => { onlyAfter: [], statusChanged: [], countChanged: [], + beforeTotal: 1, + afterTotal: 1, } ); } ); @@ -240,11 +266,40 @@ describe( 'diffNetwork', () => { const after = [ { url, method: 'GET', status: 200 } ]; const result = diffNetwork( before, after ); assert.deepEqual( result.countChanged, [ - { key: 'GET /wp-json/jetpack/v4/settings', beforeCount: 3, afterCount: 1 }, + { key: 'GET site.test/wp-json/jetpack/v4/settings', beforeCount: 3, afterCount: 1 }, ] ); assert.equal( result.statusChanged.length, 0 ); } ); + it( 'drops per-event tracking traffic that would flood both sides', () => { + const before = [ { url: 'https://pixel.wp.com/t.gif?_ts=1', method: 'GET', status: 200 } ]; + const after = [ { url: 'https://pixel.wp.com/t.gif?_ts=2', method: 'GET', status: 200 } ]; + const result = diffNetwork( before, after ); + assert.deepEqual( result.onlyBefore, [] ); + assert.deepEqual( result.onlyAfter, [] ); + assert.equal( result.beforeTotal, 0 ); + } ); + + it( 'keeps a host the caller did not ask to ignore', () => { + const before = [ { url: 'https://stats.test/t.gif?_ts=1', method: 'GET', status: 200 } ]; + const result = diffNetwork( before, [] ); + assert.equal( result.onlyBefore.length, 1 ); + } ); + + it( 'records a request that never got a response, from its failure entry', () => { + const after = [ + { + url: 'http://nonexistent.invalid/boot.js', + method: 'GET', + status: 0, + failure: 'net::ERR_NAME_NOT_RESOLVED', + }, + ]; + const result = diffNetwork( [], after ); + assert.equal( result.onlyAfter.length, 1 ); + assert.equal( result.onlyAfter[ 0 ].status, 0 ); + } ); + it( 'counts repeats of a request that fires on one side only', () => { const url = 'https://site.test/legacy.js'; const before = [ diff --git a/tools/port-verification/src/options.js b/tools/port-verification/src/options.js index fa4a9de19975..411c6ddf9cbf 100644 --- a/tools/port-verification/src/options.js +++ b/tools/port-verification/src/options.js @@ -4,7 +4,13 @@ */ import { parseArgs } from 'util'; -import { DEFAULT_IGNORED_QUERY_PARAMS, DEFAULT_TOLERANCE_PX } from './selectors.js'; +import { + DEFAULT_IGNORED_HOSTS, + DEFAULT_IGNORED_QUERY_PARAMS, + DEFAULT_TOLERANCE_PX, +} from './selectors.js'; + +const LOAD_STATES = [ 'load', 'domcontentloaded', 'networkidle' ]; export const OPTION_SPEC = { url: { type: 'string' }, @@ -15,6 +21,9 @@ export const OPTION_SPEC = { 'wait-selector': { type: 'string' }, tolerance: { type: 'string' }, 'ignore-query-param': { type: 'string', multiple: true, default: [] }, + 'ignore-host': { type: 'string', multiple: true, default: [] }, + 'autologin-url': { type: 'string' }, + 'load-state': { type: 'string' }, out: { type: 'string' }, before: { type: 'string' }, after: { type: 'string' }, @@ -40,6 +49,20 @@ function parseTolerance( raw ) { return value; } +/** + * @param {string|undefined} raw + * @return {string} + */ +function parseLoadState( raw ) { + if ( raw === undefined ) { + return 'networkidle'; + } + if ( ! LOAD_STATES.includes( raw ) ) { + throw new Error( `--load-state must be one of ${ LOAD_STATES.join( ', ' ) }, got "${ raw }".` ); + } + return raw; +} + /** * @param {string[]} argv - Everything after the subcommand. * @param {object} [env] - Defaults to `process.env`. @@ -53,12 +76,17 @@ export function parseOptions( argv, env = process.env ) { flag: values.flag, username: values.user ?? env.WP_ADMIN_USER, password: values.pass ?? env.WP_ADMIN_PASS, + autologinUrl: values[ 'autologin-url' ], controlSelector: values[ 'control-selector' ], waitForSelector: values[ 'wait-selector' ], + loadState: parseLoadState( values[ 'load-state' ] ), tolerancePx: parseTolerance( values.tolerance ), ignoreQueryParams: extraIgnoreParams.length ? [ ...DEFAULT_IGNORED_QUERY_PARAMS, ...extraIgnoreParams ] : undefined, + ignoreHosts: values[ 'ignore-host' ].length + ? [ ...DEFAULT_IGNORED_HOSTS, ...values[ 'ignore-host' ] ] + : undefined, out: values.out, before: values.before, after: values.after, diff --git a/tools/port-verification/src/options.test.js b/tools/port-verification/src/options.test.js index 217aa6e32bde..f48d1b582015 100644 --- a/tools/port-verification/src/options.test.js +++ b/tools/port-verification/src/options.test.js @@ -1,7 +1,11 @@ import assert from 'node:assert/strict'; import { describe, it } from 'node:test'; import { parseOptions } from './options.js'; -import { DEFAULT_IGNORED_QUERY_PARAMS, DEFAULT_TOLERANCE_PX } from './selectors.js'; +import { + DEFAULT_IGNORED_HOSTS, + DEFAULT_IGNORED_QUERY_PARAMS, + DEFAULT_TOLERANCE_PX, +} from './selectors.js'; describe( 'parseOptions', () => { it( 'camel-cases the kebab-case flags', () => { @@ -67,6 +71,26 @@ describe( 'parseOptions', () => { ] ); } ); + it( 'defaults the load state and accepts the Playwright ones', () => { + assert.equal( parseOptions( [], {} ).loadState, 'networkidle' ); + assert.equal( parseOptions( [ '--load-state', 'load' ], {} ).loadState, 'load' ); + } ); + + it( 'rejects a load state Playwright does not have', () => { + assert.throws( () => parseOptions( [ '--load-state', 'idle' ], {} ), /--load-state/ ); + } ); + + it( 'appends repeated --ignore-host values to the defaults', () => { + const options = parseOptions( [ '--ignore-host', 'stats.test' ], {} ); + assert.deepEqual( options.ignoreHosts, [ ...DEFAULT_IGNORED_HOSTS, 'stats.test' ] ); + assert.equal( parseOptions( [], {} ).ignoreHosts, undefined ); + } ); + + it( 'reads --autologin-url', () => { + const options = parseOptions( [ '--autologin-url', 'https://s.test/?auto_login' ], {} ); + assert.equal( options.autologinUrl, 'https://s.test/?auto_login' ); + } ); + it( 'rejects an unknown flag rather than ignoring it', () => { assert.throws( () => parseOptions( [ '--bogus' ], {} ) ); } ); diff --git a/tools/port-verification/src/report.js b/tools/port-verification/src/report.js index 31d79e52f431..2adf5f6e5db7 100644 --- a/tools/port-verification/src/report.js +++ b/tools/port-verification/src/report.js @@ -116,6 +116,8 @@ function countChangeList( changes ) { * @param {{geometry: object[], network: object}} diffResult - From `diffSnapshots()`. * @param {object} meta - Run context to print in the header. * @param {string} meta.url - Page URL that was captured. + * @param {string} [meta.beforeUrl] - URL the flag-off snapshot recorded. + * @param {string} [meta.afterUrl] - URL the flag-on snapshot recorded. * @param {string} [meta.flag] - Feature flag name. * @param {string} [meta.beforeCapturedAt] - ISO timestamp of the flag-off capture. * @param {string} [meta.afterCapturedAt] - ISO timestamp of the flag-on capture. @@ -144,11 +146,30 @@ export function formatReport( } return false; } ).length + unmeasured.filter( u => u.required ).length; + // A side that recorded nothing is not agreement, it is a capture that did not happen. + const emptySides = []; + if ( network.beforeTotal === 0 ) { + emptySides.push( 'flag off' ); + } + if ( network.afterTotal === 0 ) { + emptySides.push( 'flag on' ); + } const networkFindings = network.onlyBefore.length + network.onlyAfter.length + network.statusChanged.length + - network.countChanged.length; + network.countChanged.length + + emptySides.length; + + const warnings = []; + if ( emptySides.length ) { + warnings.push( `**No requests recorded with the ${ emptySides.join( ' or ' ) }.**` ); + } + if ( meta.beforeUrl && meta.afterUrl && meta.beforeUrl !== meta.afterUrl ) { + warnings.push( + `**The two captures are of different pages:** \`${ meta.beforeUrl }\` vs \`${ meta.afterUrl }\`.` + ); + } const lines = [ '## Port verification -- steps 2 & 3 (JETPACK-2685)', @@ -167,6 +188,8 @@ export function formatReport( '', '### Step 3 -- network panel', '', + `Compared ${ network.beforeTotal ?? 0 } request(s) with the flag off against ${ network.afterTotal ?? 0 } with it on.`, + '', `- Only with flag off (${ network.onlyBefore.length }):`, requestList( network.onlyBefore, options ), `- Only with flag on (${ network.onlyAfter.length }):`, @@ -178,6 +201,8 @@ export function formatReport( '', '### Summary', '', + ...warnings, + warnings.length ? '' : null, `${ geometryFindings } geometry finding(s), ${ networkFindings } network finding(s).`, "The only difference JETPACK-2573 accepts is a uniform 8px inset from boot's stage gutter on the page root. Anything else above needs a look before merging.", ].filter( line => line !== null ); diff --git a/tools/port-verification/src/report.test.js b/tools/port-verification/src/report.test.js index 5cc038bf7660..cabd3f0deb86 100644 --- a/tools/port-verification/src/report.test.js +++ b/tools/port-verification/src/report.test.js @@ -12,6 +12,19 @@ import { import { formatReport } from './report.js'; import { DEFAULT_GEOMETRY_TARGETS } from './selectors.js'; +/** + * What `capturePage()` writes into `meta.targets` once --control-selector is given. + * + * @param {string} selector + * @return {object} + */ +function withControlSelector( selector ) { + return { + ...DEFAULT_GEOMETRY_TARGETS, + control: { ...DEFAULT_GEOMETRY_TARGETS.control, selector, required: true }, + }; +} + describe( 'formatReport', () => { it( 'includes the page, flag and capture timestamps in the header', () => { const report = formatReport( diffSnapshots( flagOffSnapshot(), flagOnSnapshot() ), { @@ -92,17 +105,65 @@ describe( 'formatReport', () => { assert.match( report, /Only with flag off \(0\):\n {2}- none/ ); } ); - it( 'summary counts nothing when the only configured target is the optional control', () => { - const before = { geometry: {}, network: [] }; - const after = { geometry: {}, network: [] }; + it( 'summary counts nothing for geometry when the only target is the optional control', () => { + const onlyControl = { control: DEFAULT_GEOMETRY_TARGETS.control }; + const empty = { geometry: {}, network: [] }; + const report = formatReport( diffSnapshots( empty, empty ), { url: 'x' }, onlyControl ); + assert.match( report, /0 geometry finding\(s\)/ ); + } ); + + it( 'treats a side that recorded no requests as a finding, not as agreement', () => { + const onlyControl = { control: DEFAULT_GEOMETRY_TARGETS.control }; + const empty = { geometry: {}, network: [] }; + const report = formatReport( diffSnapshots( empty, empty ), { url: 'x' }, onlyControl ); + assert.match( report, /No requests recorded with the flag off or flag on/ ); + assert.match( report, /2 network finding\(s\)/ ); + } ); + + it( 'prints how many requests each side contributed', () => { + const report = formatReport( diffSnapshots( flagOffSnapshot(), flagOnSnapshot() ), { + url: 'x', + } ); + assert.match( report, /Compared 2 request\(s\) with the flag off against 2 with it on\./ ); + } ); + + it( 'warns when the two snapshots are of different pages', () => { + const report = formatReport( diffSnapshots( flagOffSnapshot(), flagOnSnapshot() ), { + url: 'x', + beforeUrl: 'https://site.test/wp-admin/admin.php?page=jetpack', + afterUrl: 'https://site.test/wp-admin/admin.php?page=jetpack-backup', + } ); + assert.match( report, /The two captures are of different pages/ ); + } ); + + it( 'treats a control that was asked for and then vanished as a finding', () => { + const before = flagOffSnapshot(); + const after = flagOnSnapshot(); + delete after.geometry.control; const report = formatReport( diffSnapshots( before, after ), { url: 'x' }, - { - control: DEFAULT_GEOMETRY_TARGETS.control, - } + withControlSelector( '.real-button' ) + ); + assert.match( + report, + /\| Control \| MISSING \| not found in after \(flag on\) -- required target \|/ ); - assert.match( report, /0 geometry finding\(s\), 0 network finding\(s\)\./ ); + assert.match( report, /2 geometry finding\(s\)/ ); + } ); + + it( 'says a control selector matched nothing, rather than that none was given', () => { + const before = flagOffSnapshot(); + const after = flagOnSnapshot(); + delete before.geometry.control; + delete after.geometry.control; + const report = formatReport( + diffSnapshots( before, after ), + { url: 'x' }, + withControlSelector( '.typo-button' ) + ); + assert.match( report, /\| Control \| NOT MEASURED \| selector matched in neither capture \|/ ); + assert.match( report, /2 geometry finding\(s\)/ ); } ); it( 'escalates a required target missing from one side to a geometry finding', () => { diff --git a/tools/port-verification/src/selectors.js b/tools/port-verification/src/selectors.js index 185334221826..30c7320dc653 100644 --- a/tools/port-verification/src/selectors.js +++ b/tools/port-verification/src/selectors.js @@ -27,5 +27,9 @@ export const DEFAULT_IGNORED_QUERY_PARAMS = [ '_locale', ]; +// Hosts whose traffic is per-event by design: a Tracks pixel carries a timestamp in every +// URL, so each load produces new keys and buries the real findings. --ignore-host adds more. +export const DEFAULT_IGNORED_HOSTS = [ 'pixel.wp.com' ]; + // Below this, a geometry delta is rounding noise, not a real shift. export const DEFAULT_TOLERANCE_PX = 0.5; diff --git a/tools/port-verification/test/capture.smoke.test.js b/tools/port-verification/test/capture.smoke.test.js new file mode 100644 index 000000000000..d17345c64027 --- /dev/null +++ b/tools/port-verification/test/capture.smoke.test.js @@ -0,0 +1,169 @@ +/** + * Browser-backed smoke test for src/capture.js against test/stub-wp-admin.js. Skipped when + * no Chromium is installed (`pnpm exec playwright install chromium`), so `pnpm test` stays + * browser-free; run it with `pnpm test:smoke`. + */ + +import assert from 'node:assert/strict'; +import { after, before, describe, it } from 'node:test'; +import { capturePage } from '../src/capture.js'; +import { diffSnapshots } from '../src/diff.js'; +import { startStub } from './stub-wp-admin.js'; + +const CREDENTIALS = { username: 'admin', password: 'password' }; + +const hasBrowser = await ( async () => { + try { + const { chromium } = await import( 'playwright' ); + const browser = await chromium.launch( { headless: true } ); + await browser.close(); + return true; + } catch { + return false; + } +} )(); + +describe( + 'capturePage against a wp-admin stub', + { skip: hasBrowser ? false : 'no Chromium installed' }, + () => { + let stub; + + before( async () => { + stub = await startStub(); + } ); + after( async () => { + await stub.close(); + } ); + + it( 'logs in and captures the target page, not the Dashboard', async () => { + const snapshot = await capturePage( { + ...CREDENTIALS, + url: `${ stub.url }/wp-admin/admin.php?page=jetpack`, + loadState: 'load', + } ); + assert.equal( snapshot.meta.url, `${ stub.url }/wp-admin/admin.php?page=jetpack` ); + assert.ok( snapshot.geometry.root, '#wpwrap was measured' ); + assert.ok( snapshot.geometry.header, '#wpadminbar was measured' ); + } ); + + it( 'drops the login and Dashboard traffic from the snapshot', async () => { + const snapshot = await capturePage( { + ...CREDENTIALS, + url: `${ stub.url }/wp-admin/admin.php?page=jetpack`, + loadState: 'load', + } ); + const paths = snapshot.network.map( r => new URL( r.url ).pathname ); + assert.ok( paths.includes( '/common.js' ), 'kept the target page request' ); + assert.ok( ! paths.includes( '/dashboard-only.js' ), 'dropped the Dashboard request' ); + assert.ok( ! paths.includes( '/wp-login.php' ), 'dropped the login request' ); + } ); + + it( 'reports an element hidden by an ancestor as hidden, not as a zero rect', async () => { + const snapshot = await capturePage( { + ...CREDENTIALS, + url: `${ stub.url }/wp-admin/admin.php?page=jetpack&flag=on`, + controlSelector: '.only-real-button', + loadState: 'load', + } ); + assert.equal( snapshot.geometry.footer.hidden, true, '#wpfooter is display:none' ); + assert.equal( snapshot.geometry.control.hidden, false ); + assert.equal( snapshot.geometry.control.matchCount, 1 ); + } ); + + it( 'counts every element a control selector matches', async () => { + const snapshot = await capturePage( { + ...CREDENTIALS, + url: `${ stub.url }/wp-admin/admin.php?page=jetpack`, + controlSelector: '.components-button', + loadState: 'load', + } ); + assert.equal( snapshot.geometry.control.matchCount, 2 ); + // The first match is inside a display:none wrapper, which a computed style on the + // element alone would miss. + assert.equal( snapshot.geometry.control.hidden, true ); + } ); + + it( 'records a request that never gets a response', async () => { + const snapshot = await capturePage( { + ...CREDENTIALS, + url: `${ stub.url }/wp-admin/admin.php?page=jetpack&flag=on`, + loadState: 'load', + } ); + const missing = snapshot.network.find( r => r.url.endsWith( '/boot.js' ) ); + assert.ok( missing, 'boot.js appears in the snapshot' ); + } ); + + it( 'marks the control required once a selector is given', async () => { + const snapshot = await capturePage( { + ...CREDENTIALS, + url: `${ stub.url }/wp-admin/admin.php?page=jetpack`, + controlSelector: '.only-real-button', + loadState: 'load', + } ); + assert.equal( snapshot.meta.targets.control.required, true ); + assert.equal( snapshot.meta.targets.control.selector, '.only-real-button' ); + } ); + + it( 'refuses to capture wp-login.php when no credentials are given', async () => { + await assert.rejects( + capturePage( { url: `${ stub.url }/wp-admin/admin.php?page=jetpack`, loadState: 'load' } ), + /wp-login\.php/ + ); + } ); + + it( 'refuses to capture a page a redirect moved it away from', async () => { + await assert.rejects( + capturePage( { ...CREDENTIALS, url: `${ stub.url }/elsewhere`, loadState: 'load' } ), + /a redirect moved the capture/ + ); + } ); + + it( 'follows an autologin URL instead of the login form', async () => { + const snapshot = await capturePage( { + url: `${ stub.url }/wp-admin/admin.php?page=jetpack`, + autologinUrl: `${ stub.url }/auto-login`, + loadState: 'load', + } ); + assert.ok( snapshot.geometry.root ); + } ); + + it( 'fails the flag-off capture when the flag-on marker is already present', async () => { + await assert.rejects( + capturePage( { + ...CREDENTIALS, + url: `${ stub.url }/wp-admin/admin.php?page=jetpack&flag=on`, + absentSelector: '#boot-mount', + loadState: 'load', + } ), + /did the flag really change/ + ); + } ); + + it( 'diffs two real captures into the accepted inset and nothing else', async () => { + const shared = { ...CREDENTIALS, controlSelector: '.only-real-button', loadState: 'load' }; + const off = await capturePage( { + ...shared, + url: `${ stub.url }/wp-admin/admin.php?page=jetpack`, + } ); + const on = await capturePage( { + ...shared, + url: `${ stub.url }/wp-admin/admin.php?page=jetpack&flag=on`, + } ); + const { geometry, network } = diffSnapshots( off, on ); + + const root = geometry.find( g => g.key === 'root' ); + assert.equal( root.status, 'changed' ); + // The stub's body has a default margin, so assert the 8px shift, not absolute x. + assert.match( root.details.join( ' ' ), /x: \d+px -> \d+px \(Δ8\.0px\)/ ); + + const footer = geometry.find( g => g.key === 'footer' ); + assert.equal( footer.status, 'hidden-changed' ); + + assert.equal( geometry.find( g => g.key === 'header' ).status, 'ok' ); + + const bootJs = network.onlyAfter.find( r => r.url.endsWith( '/boot.js' ) ); + assert.ok( bootJs, 'boot.js fires only with the flag on' ); + } ); + } +); diff --git a/tools/port-verification/test/stub-wp-admin.js b/tools/port-verification/test/stub-wp-admin.js new file mode 100644 index 000000000000..652342b9fbd8 --- /dev/null +++ b/tools/port-verification/test/stub-wp-admin.js @@ -0,0 +1,98 @@ +/** + * A wp-admin stub small enough to serve from this repo: wp-login.php, a Dashboard, and one + * admin page whose markup changes with a query flag. Enough to exercise capture.js's login, + * buffer clear, redirect guards and hidden/ambiguous-selector detection without a WordPress. + */ + +import http from 'node:http'; + +const COOKIE = 'stub_logged_in=1'; + +const LOGIN_PAGE = `
+ +
`; + +const DASHBOARD = `
bar
+
widgets
+`; + +/** + * @param {boolean} flagOn + * @return {string} + */ +function adminPage( flagOn ) { + // Flag-on hides #wpfooter and insets #wpwrap by 8px, the one difference the rule accepts. + const footerStyle = flagOn ? 'display:none' : ''; + const wrapStyle = flagOn ? 'margin:8px;width:292px' : 'margin:0;width:300px'; + const mount = flagOn ? '
mounted
' : ''; + const extra = flagOn ? '' : ''; + return `
bar
+
+
+
+ + +
+ ${ mount } +
+ +${ extra }`; +} + +/** + * @param {object} [options] + * @param {boolean} [options.requireLogin] - 302 to wp-login.php without the cookie. Default true. + * @return {Promise<{url: string, close: () => Promise, requests: string[]}>} + */ +export async function startStub( options = {} ) { + const { requireLogin = true } = options; + const requests = []; + + const server = http.createServer( ( req, res ) => { + requests.push( req.url ); + const url = new URL( req.url, 'http://127.0.0.1' ); + const authed = ! requireLogin || ( req.headers.cookie ?? '' ).includes( COOKIE ); + + if ( url.pathname === '/wp-login.php' ) { + if ( req.method === 'POST' ) { + res.writeHead( 302, { 'Set-Cookie': COOKIE, Location: '/wp-admin/index.php' } ); + return res.end(); + } + res.writeHead( 200, { 'Content-Type': 'text/html' } ); + return res.end( LOGIN_PAGE ); + } + if ( url.pathname === '/auto-login' ) { + res.writeHead( 302, { 'Set-Cookie': COOKIE, Location: '/wp-admin/index.php' } ); + return res.end(); + } + if ( ! authed ) { + res.writeHead( 302, { Location: '/wp-login.php' } ); + return res.end(); + } + if ( url.pathname === '/wp-admin/index.php' ) { + res.writeHead( 200, { 'Content-Type': 'text/html' } ); + return res.end( DASHBOARD ); + } + if ( url.pathname === '/wp-admin/admin.php' ) { + res.writeHead( 200, { 'Content-Type': 'text/html' } ); + return res.end( adminPage( url.searchParams.get( 'flag' ) === 'on' ) ); + } + if ( url.pathname === '/elsewhere' ) { + res.writeHead( 302, { Location: '/wp-admin/index.php' } ); + return res.end(); + } + if ( url.pathname.endsWith( '.js' ) ) { + res.writeHead( 200, { 'Content-Type': 'application/javascript' } ); + return res.end( '/* */' ); + } + res.writeHead( 404, { 'Content-Type': 'text/plain' } ); + res.end( 'not found' ); + } ); + + await new Promise( resolve => server.listen( 0, '127.0.0.1', resolve ) ); + return { + url: `http://127.0.0.1:${ server.address().port }`, + requests, + close: () => new Promise( resolve => server.close( resolve ) ), + }; +} From 86f977247cbd006a134801b9b290c3b5df426e64 Mon Sep 17 00:00:00 2001 From: Christian Gastrell Date: Tue, 22 Sep 2026 09:36:44 -0300 Subject: [PATCH 5/7] Fix two defects a live Jurassic Ninja run found First run of capture.js against a real site, on Jetpack 16.3-a.1: * Login never worked on a site with Jetpack SSO, which is most Jetpack sites. SSO leaves the classic form in the DOM but hidden behind "Log in with username and password", so filling it timed out. Click the toggle first. Pick it by visibility, not by class: the visible one on the test site was `.jetpack-sso-toggle.wpcom`, which is not the one its name suggests. * `_cacheBuster` was not an ignored query param, so every `wp-json` call appeared under both "only with flag off" and "only with flag on". On the Search dashboard that was 4 of 24 network findings. Verified against the `search-wp-build` flag on a real Search dashboard: the four default selectors all resolve, geometry comes back clean on all four frame targets, and step 3 shows the port swapping `jp-search-dashboard.js` for boot's module graph. After the `_cacheBuster` fix, 20 findings, all real. 71 unit tests, 11 smoke tests. Co-Authored-By: Claude Opus 5 (1M context) --- tools/port-verification/src/capture.js | 10 ++++++++++ tools/port-verification/src/diff.test.js | 12 ++++++++++++ tools/port-verification/src/selectors.js | 3 +++ 3 files changed, 25 insertions(+) diff --git a/tools/port-verification/src/capture.js b/tools/port-verification/src/capture.js index 8764abaa99b7..9a722605e4aa 100644 --- a/tools/port-verification/src/capture.js +++ b/tools/port-verification/src/capture.js @@ -20,6 +20,16 @@ import { DEFAULT_GEOMETRY_TARGETS } from './selectors.js'; async function login( page, siteUrl, username, password, loadState ) { const origin = new URL( siteUrl ).origin; await page.goto( `${ origin }/wp-login.php`, { waitUntil: loadState } ); + + // Jetpack SSO leaves the classic form in the DOM but hidden behind "Log in with username + // and password", so filling it blind times out on most Jetpack sites. Which class carries + // that link is not stable -- the visible one is the one to click. + const ssoToggle = page.locator( '.jetpack-sso-toggle:visible' ).first(); + if ( await ssoToggle.isVisible().catch( () => false ) ) { + await ssoToggle.click(); + await page.waitForSelector( '#user_login', { state: 'visible' } ); + } + await page.fill( '#user_login', username ); await page.fill( '#user_pass', password ); diff --git a/tools/port-verification/src/diff.test.js b/tools/port-verification/src/diff.test.js index db00a2061790..b9ad1e262ddd 100644 --- a/tools/port-verification/src/diff.test.js +++ b/tools/port-verification/src/diff.test.js @@ -179,6 +179,18 @@ describe( 'normalizeRequestKey', () => { ); } ); + it( "strips Jetpack's _cacheBuster, which changes on every REST call", () => { + const a = { + url: 'https://s.test/wp-json/jetpack/v4/search/plan?_cacheBuster=1', + method: 'GET', + }; + const b = { + url: 'https://s.test/wp-json/jetpack/v4/search/plan?_cacheBuster=2', + method: 'GET', + }; + assert.equal( normalizeRequestKey( a ), normalizeRequestKey( b ) ); + } ); + it( 'keeps the host, so the same path on two hosts is two requests', () => { const a = { url: 'https://site.test/boot.js', method: 'GET' }; const b = { url: 'https://cdn.test/boot.js', method: 'GET' }; diff --git a/tools/port-verification/src/selectors.js b/tools/port-verification/src/selectors.js index 30c7320dc653..254c314b25f6 100644 --- a/tools/port-verification/src/selectors.js +++ b/tools/port-verification/src/selectors.js @@ -25,6 +25,9 @@ export const DEFAULT_IGNORED_QUERY_PARAMS = [ 'ver', '_', '_locale', + // Jetpack's own REST cache buster; without it every wp-json call lands in both + // "only with flag off" and "only with flag on". Seen on the Search dashboard. + '_cacheBuster', ]; // Hosts whose traffic is per-event by design: a Tracks pixel carries a timestamp in every From e7901d7c07987b6fc1a39ae7ac7fe1300f8d9603 Mon Sep 17 00:00:00 2001 From: Christian Gastrell Date: Tue, 22 Sep 2026 11:55:02 -0300 Subject: [PATCH 6/7] Stop the login flaking on networkidle Re-running the live check at 86f977247cb failed with "Login did not land in wp-admin", then passed twice. About one run in four. `login()` gated its navigation on the caller's --load-state, which defaults to `networkidle`. wp-admin never really goes idle -- heartbeat, JITM -- so the wait sometimes timed out, and the catch reported it as bad credentials, which is a misleading error on top of a flaky one. The login hop now uses `domcontentloaded` and leans on the selector waits, which are the real evidence of being logged in. --load-state still applies to the page capture, where settling is the point. 8 of 8 live logins clean afterwards, against roughly 1 in 4 failing before. That is evidence rather than proof at this sample size, but the mechanism matches: a page that never goes idle cannot be waited on that way. Adding a `#loginform` wait then broke 9 of the 11 smoke tests, because the stub's form had no `id`, which real wp-login.php does have. Fixed the stub rather than dropping the wait -- that gap is the same kind of divergence that hid the SSO bug until a real site ran it. Co-Authored-By: Claude Opus 5 (1M context) --- tools/port-verification/src/capture.js | 13 ++++++++----- tools/port-verification/test/stub-wp-admin.js | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tools/port-verification/src/capture.js b/tools/port-verification/src/capture.js index 9a722605e4aa..8e9141b891c1 100644 --- a/tools/port-verification/src/capture.js +++ b/tools/port-verification/src/capture.js @@ -14,12 +14,15 @@ import { DEFAULT_GEOMETRY_TARGETS } from './selectors.js'; * @param {string} siteUrl * @param {string} username * @param {string} password - * @param {string} loadState * @return {Promise} */ -async function login( page, siteUrl, username, password, loadState ) { +async function login( page, siteUrl, username, password ) { const origin = new URL( siteUrl ).origin; - await page.goto( `${ origin }/wp-login.php`, { waitUntil: loadState } ); + // Not the caller's --load-state: wp-admin keeps polling (heartbeat, JITM), so gating the + // login hop on networkidle failed about one run in four. The selector waits below are the + // real gate, and they say more about being logged in than any load state does. + await page.goto( `${ origin }/wp-login.php`, { waitUntil: 'domcontentloaded' } ); + await page.waitForSelector( '#loginform', { state: 'attached' } ); // Jetpack SSO leaves the classic form in the DOM but hidden behind "Log in with username // and password", so filling it blind times out on most Jetpack sites. Which class carries @@ -35,7 +38,7 @@ async function login( page, siteUrl, username, password, loadState ) { try { await Promise.all( [ - page.waitForURL( '**/wp-admin/**', { waitUntil: loadState } ), + page.waitForURL( '**/wp-admin/**', { waitUntil: 'domcontentloaded' } ), page.click( '#wp-submit' ), ] ); // Confirm login landed in wp-admin (the Dashboard is the post-login screen) rather than @@ -226,7 +229,7 @@ export async function capturePage( options ) { } ); if ( username && password ) { - await login( page, url, username, password, loadState ); + await login( page, url, username, password ); } else if ( autologinUrl ) { await page.goto( autologinUrl, { waitUntil: loadState } ); } diff --git a/tools/port-verification/test/stub-wp-admin.js b/tools/port-verification/test/stub-wp-admin.js index 652342b9fbd8..97426c6e2b6f 100644 --- a/tools/port-verification/test/stub-wp-admin.js +++ b/tools/port-verification/test/stub-wp-admin.js @@ -8,7 +8,7 @@ import http from 'node:http'; const COOKIE = 'stub_logged_in=1'; -const LOGIN_PAGE = `
+const LOGIN_PAGE = `
`; From 3ee7fe4b6192a3bd6a71e6aab4527a878a55815e Mon Sep 17 00:00:00 2001 From: Christian Gastrell Date: Wed, 23 Sep 2026 14:58:49 -0300 Subject: [PATCH 7/7] Own tools/port-verification in CODEOWNERS @Automattic/jetpack-monorepo owns /tools by default and does not have the bandwidth for a special-purpose tool, so name an owner explicitly. Follows the @jeherve precedent for a single-owner tool; last match wins, so this takes it off the monorepo team. Co-Authored-By: Claude Opus 5 (1M context) --- .github/CODEOWNERS | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index db5523a5d194..5f5a91a52f3f 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -52,6 +52,7 @@ # Tools owned by specific teams /tools/performance @Automattic/jetpack-performance +/tools/port-verification @CGastrell # Actions