diff --git a/tools/performance/README.md b/tools/performance/README.md index ac97901439d5..71eeb30146d6 100644 --- a/tools/performance/README.md +++ b/tools/performance/README.md @@ -16,16 +16,16 @@ The test suite is designed to run in TeamCity. See `TEAMCITY-SETUP.md` for detai ### Environment Variables -| Variable | Description | -|----------|-------------| -| `CODEVITALS_TOKEN` | API token for posting results to CodeVitals | -| `CODEVITALS_URL` | CodeVitals API URL (default: https://www.codevitals.run) | -| `COMPOSE_PROJECT_NAME` | Unique Docker project name for build isolation | -| `GIT_COMMIT` | Git commit SHA for tracking (auto-detected from plugin) | -| `GIT_BRANCH` | Git branch for tracking (default: trunk) | -| `ITERATIONS` | Number of measurement iterations (default: 5) | -| `WP_ADMIN_USER` | WordPress admin username (default: admin) | -| `WP_ADMIN_PASS` | WordPress admin password (default: password) | +| Variable | Description | +| ---------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `CODEVITALS_TOKEN` | API token for posting results to CodeVitals | +| `CODEVITALS_URL` | CodeVitals API URL (default: https://codevitals.run). Use the apex host, not `www.`: the `www.` host 301-redirects the API and the redirect drops the POST body. Set an origin-only URL (scheme + host); the API path is appended, so any path prefix on this value is not preserved. | +| `COMPOSE_PROJECT_NAME` | Unique Docker project name for build isolation | +| `GIT_COMMIT` | Git commit SHA for tracking (auto-detected from plugin) | +| `GIT_BRANCH` | Git branch for tracking (default: trunk) | +| `ITERATIONS` | Number of measurement iterations (default: 5) | +| `WP_ADMIN_USER` | WordPress admin username (default: admin) | +| `WP_ADMIN_PASS` | WordPress admin password (default: password) | ## Metric @@ -41,11 +41,48 @@ The test suite is designed to run in TeamCity. See `TEAMCITY-SETUP.md` for detai ## Scripts -| Script | Description | -|--------|-------------| -| `pnpm test` | Run full test suite (auto-clones plugin if needed) | -| `pnpm test:quick` | Quick test with 2 iterations | -| `pnpm calibrate` | Run CPU throttling calibration | -| `pnpm measure` | Run LCP measurement only | -| `pnpm report` | Post results to CodeVitals only | -| `pnpm test -- --skip-codevitals` | Run tests without posting to CodeVitals | +| Script | Description | +| -------------------------------- | ---------------------------------------------------------------------- | +| `pnpm test` | Run full test suite (auto-clones plugin if needed) | +| `pnpm test:quick` | Quick test with 2 iterations | +| `pnpm calibrate` | Run CPU throttling calibration | +| `pnpm measure` | Run LCP measurement only | +| `pnpm report` | Post results to CodeVitals only | +| `pnpm report:dry` | Build and print the CodeVitals payload without posting (CI smoke test) | +| `pnpm test -- --skip-codevitals` | Run tests without posting to CodeVitals | + +## Safeguards + +CodeVitals is an **append-only** store with no self-service rollback. Once a bad point lands (wrong key, out-of-range value, scale error), the trend graph stays polluted until a CodeVitals admin corrects it. The safeguards below keep bad data out. + +### Dry run + +`pnpm report:dry` builds the full payload, prints it, and exits without posting. It needs no `CODEVITALS_TOKEN`, so it works as a CI smoke test. Use it to inspect a payload before a real `pnpm report`. + +### Sanity-range assertions + +`post-to-codevitals.js` checks every typed metric against `SANITY_RANGES` in `scenarios.js` before posting. A value outside its range is logged and skipped (not posted), and the script exits non-zero so CI surfaces the failure. Other valid metrics in the same run still post. + +| Metric | Min | Max | +| ------ | --- | ----- | +| `lcp` | 100 | 60000 | +| `ttfb` | 10 | 10000 | +| `fcp` | 50 | 30000 | +| `tbt` | 0 | 10000 | +| `cls` | 0 | 5 | + +Add a row when a new metric type starts being posted, and set `metricType` on the scenario so the check applies to it. + +### Staging keys + +Post a new metric to a `-staging` CodeVitals key first (e.g. `…-timeToFirstByte-staging`) for 2-3 builds. Inspect the values in the CodeVitals UI, then rename to the production key. This gives a safety window before a new metric reaches production. + +### If bad data lands anyway + +A bad point must be corrected by the CodeVitals admin. Steps: + +1. **Stop posting.** Pause the CodeVitals Jetpack Performance Scheduler build in TeamCity. +2. **Document the extent.** Record the affected metric keys, the commit range (monorepo hashes), the time window (build start to end), and whether the values are isolated or systematic. +3. **Request a correction.** CodeVitals runs outside this project; send the request through the team channel named in the FORMS-696 runbook. Include metric ID 113, the affected keys, the commit/timestamp range, and the root cause. +4. **Fix the root cause.** Add or tighten a sanity range or staging gate. Don't re-enable the Scheduler until the fix merges. +5. **Record the incident.** Add the failure mode, detection timing, and prevention measures to the FORMS-696 maintenance runbook. diff --git a/tools/performance/package.json b/tools/performance/package.json index 13ab3cecc321..02e716d12c18 100644 --- a/tools/performance/package.json +++ b/tools/performance/package.json @@ -13,9 +13,11 @@ "docker:up": "docker compose -f docker/docker-compose.yml up -d", "measure": "node scripts/measure-lcp.js", "report": "node scripts/post-to-codevitals.js", + "report:dry": "node scripts/post-to-codevitals.js --dry-run", "setup:browsers": "playwright install chromium", - "test": "node scripts/run-performance-tests.js", - "test:quick": "ITERATIONS=2 node scripts/run-performance-tests.js" + "test": "node --test && node scripts/run-performance-tests.js", + "test:quick": "ITERATIONS=2 node scripts/run-performance-tests.js", + "test:unit": "node --test" }, "dependencies": { "dotenv": "^16.3.1", diff --git a/tools/performance/scripts/post-to-codevitals.js b/tools/performance/scripts/post-to-codevitals.js index d0e78b76f2c4..784cdf845c0c 100644 --- a/tools/performance/scripts/post-to-codevitals.js +++ b/tools/performance/scripts/post-to-codevitals.js @@ -2,27 +2,178 @@ import fs from 'fs'; import path from 'path'; -import { SCENARIOS } from './scenarios.js'; +import { SCENARIOS, SANITY_RANGES } from './scenarios.js'; -/** Extract metrics for a single scenario. */ -function extractScenarioMetrics( scenario, summary ) { - const metrics = {}; +/** + * Exit code for a LOCAL data-integrity failure: a metric failed a sanity check or + * a scenario is misconfigured. This is distinct from exit 1 (a remote CodeVitals + * transport/API failure). run-performance-tests.js treats this code as always + * fatal and never suppresses it with `--allow-codevitals-failure`, which exists + * only to tolerate CodeVitals network outages, not bad local data. + */ +const VALIDATION_FAILED_EXIT_CODE = 2; + +/** + * A local data-integrity or scenario-configuration failure. A thrown error of this + * type must always fail the build (exit VALIDATION_FAILED_EXIT_CODE), so the runner + * can never suppress it under `--allow-codevitals-failure`. It is distinct from a + * CodeVitals transport/API error, which exits 1 and that flag may tolerate. + */ +class ValidationError extends Error { + constructor( message ) { + super( message ); + this.name = 'ValidationError'; + } +} + +/** + * Map a thrown error to a process exit code. A local data-integrity failure + * (ValidationError, e.g. a misconfigured scenario) exits VALIDATION_FAILED_EXIT_CODE + * so the runner always fails the build on it; any other error (a CodeVitals + * transport/API failure) exits 1, which `--allow-codevitals-failure` may tolerate. + * + * @param {*} error - The caught error. + * @return {number} The exit code to use. + */ +function exitCodeForError( error ) { + return error instanceof ValidationError ? VALIDATION_FAILED_EXIT_CODE : 1; +} +/** + * Extract metric entries for a single scenario. + * + * Each entry carries its CodeVitals key, value, and (optional) type. The type + * drives the sanity-range check; untyped legacy entries are posted unchecked. + */ +function extractScenarioMetrics( scenario, summary ) { // Use explicit metricKey if defined, otherwise fall back to prefix-based keys if ( scenario.metricKey ) { + // A posted exact-key metric must declare a metricType so checkSanityRange can + // range-check it. Without one it would fall through as an untyped "legacy" + // entry and post any finite value unchecked — the exact hole this fail-closed + // guard exists to close. A missing type is a scenario misconfiguration, so + // fail loud here (the dry-run CI smoke test catches it before any post). + if ( ! scenario.metricType ) { + throw new ValidationError( + `Scenario "${ + scenario.key ?? scenario.metricKey + }" sets metricKey but no metricType; refusing to post an unchecked keyed metric` + ); + } // Single metric with exact key - metrics[ scenario.metricKey ] = summary.median; - } else { - // Legacy: prefix-based keys with suffixes - const prefix = scenario.metricPrefix; - metrics[ `${ prefix }_ms` ] = summary.median; - metrics[ `${ prefix }_mean_ms` ] = summary.mean; - metrics[ `${ prefix }_min_ms` ] = summary.min; - metrics[ `${ prefix }_max_ms` ] = summary.max; - metrics[ `${ prefix }_stddev_ms` ] = summary.stdDev; - } - - return metrics; + return [ { key: scenario.metricKey, value: summary.median, type: scenario.metricType } ]; + } + + // Legacy: prefix-based keys with suffixes (untyped — not range-checked) + const prefix = scenario.metricPrefix; + return [ + { key: `${ prefix }_ms`, value: summary.median }, + { key: `${ prefix }_mean_ms`, value: summary.mean }, + { key: `${ prefix }_min_ms`, value: summary.min }, + { key: `${ prefix }_max_ms`, value: summary.max }, + { key: `${ prefix }_stddev_ms`, value: summary.stdDev }, + ]; +} + +/** + * Check whether a metric value is safe to post to CodeVitals. + * + * CodeVitals is append-only, so anything uncertain fails closed. A typed metric + * whose type has no range row (a typo, or a forgotten SANITY_RANGES entry) and a + * non-finite value (null, NaN, Infinity, a numeric string) are both rejected + * rather than posted. Only a genuinely untyped legacy entry passes unchecked. + * + * @param {string|undefined} type - Metric type, or falsy for an untyped legacy entry. + * @param {*} value - Candidate metric value. + * @return {{ ok: boolean, reason?: string }} Whether the value may be posted, and why not. + */ +function checkSanityRange( type, value ) { + // Never post a non-finite value, regardless of type. null, NaN, Infinity, and + // numeric strings are always wrong for an append-only store, so this check + // comes before the untyped early return below. + if ( typeof value !== 'number' || ! Number.isFinite( value ) ) { + return { ok: false, reason: `value ${ JSON.stringify( value ) } is not a finite number` }; + } + + // Genuinely untyped legacy entry: no declared type, so no range to enforce. + if ( ! type ) { + return { ok: true }; + } + + const range = SANITY_RANGES[ type ]; + if ( ! range ) { + return { ok: false, reason: `no sanity range is defined for type "${ type }"` }; + } + + if ( value < range.min || value > range.max ) { + return { ok: false, reason: `${ value } is outside [${ range.min }, ${ range.max }]` }; + } + + return { ok: true }; +} + +/** + * Strip the CodeVitals token from a string so it can never reach a log or a + * re-thrown error. fetch and the URL parser echo the full request URL (token + * included) in their messages, and the token grants append access to an + * unrollback-able store, so scrub it before anything prints. + * + * @param {string} text - Text that may contain the token or a `token=...` query param. + * @param {string} [token] - The exact token value to strip, when known. + * @return {string} The text with the token replaced by `REDACTED`. + */ +function redactToken( text, token ) { + let safe = String( text ); + if ( token ) { + safe = safe.split( token ).join( 'REDACTED' ); + } + // Catch the query-param form too, in case the exact value isn't in hand here. + return safe.replace( /token=[^&\s]+/g, 'token=REDACTED' ); +} + +/** + * Redact the token from an error and every error in its `cause` chain, in place. + * + * A re-thrown error keeps its caught cause for debugging, but that cause (and any + * nested cause) can carry the token in its `message`, `stack`, or a custom string + * field a client stashed a URL in (e.g. `requestUrl`), or a primitive string + * `cause`. Walking the whole chain + * scrubs each one so logging the full error never leaks the secret. Native fetch + * never populates such fields, so the own-property pass is belt-and-suspenders for + * a non-native HTTP client; it stays at string fields rather than recursing into + * arbitrary nested objects. + * + * @param {*} error - The caught error (or any thrown value). + * @param {string} token - The token value to strip from messages, stacks, and string fields. + */ +function sanitizeErrorChain( error, token ) { + const seen = new Set(); + let current = error; + while ( current && typeof current === 'object' && ! seen.has( current ) ) { + seen.add( current ); + // message and stack are non-enumerable own props, so handle them explicitly. + if ( typeof current.message === 'string' ) { + current.message = redactToken( current.message, token ); + } + if ( typeof current.stack === 'string' ) { + current.stack = redactToken( current.stack, token ); + } + // Then scrub any enumerable string field (an object `cause` is non-enumerable + // and is walked on the next iteration; a string `cause` is handled just below). + for ( const key of Object.keys( current ) ) { + if ( typeof current[ key ] === 'string' ) { + current[ key ] = redactToken( current[ key ], token ); + } + } + // `cause` is non-enumerable on Error, so the pass above can't reach a primitive + // string cause (e.g. `new Error( m, { cause: someUrl } )`); it would otherwise + // survive untouched and leak the token. Redact it in place before advancing; an + // object cause is scrubbed when the loop walks into it next. + if ( typeof current.cause === 'string' ) { + current.cause = redactToken( current.cause, token ); + } + current = current.cause; + } } /** Post metrics to CodeVitals. */ @@ -34,8 +185,9 @@ async function postToCodeVitals( resultsPath, config ) { const results = JSON.parse( fs.readFileSync( resultsPath, 'utf8' ) ); - // Extract metrics from results + // Extract and sanity-check metrics from results const metrics = {}; + let validationFailed = false; // Process only scenarios marked for CodeVitals posting for ( const scenario of SCENARIOS ) { @@ -50,13 +202,25 @@ async function postToCodeVitals( resultsPath, config ) { continue; } - const scenarioMetrics = extractScenarioMetrics( scenario, measurement.summary ); - - Object.assign( metrics, scenarioMetrics ); + for ( const entry of extractScenarioMetrics( scenario, measurement.summary ) ) { + const check = checkSanityRange( entry.type, entry.value ); + if ( ! check.ok ) { + console.error( + `✗ Sanity check failed for "${ entry.key }" (${ entry.type }): ${ check.reason }. Skipping this metric.` + ); + validationFailed = true; + continue; + } + metrics[ entry.key ] = entry.value; + } } - // Validate we have metrics to post + // Nothing valid left to post. if ( Object.keys( metrics ).length === 0 ) { + if ( validationFailed ) { + // Every metric was skipped by a sanity check (failures already logged). + return { posted: false, validationFailed }; + } throw new Error( 'No metrics to post - check scenario configuration and measurement results' ); } @@ -69,13 +233,28 @@ async function postToCodeVitals( resultsPath, config ) { branch: results.git?.branch || config.gitBranch || 'trunk', }; + // Dry run: show exactly what would be posted, then stop short of the POST. + if ( config.dryRun ) { + console.log( '— DRY RUN — building payload only, not posting to CodeVitals —' ); + console.log( JSON.stringify( payload, null, 2 ) ); + return { posted: false, validationFailed, payload }; + } + console.log( 'Posting metrics to CodeVitals...' ); console.log( 'Metrics:', JSON.stringify( metrics, null, 2 ) ); - // Token passed as query param per CodeVitals API spec (don't log URL) - const url = `${ config.codeVitalsUrl }/api/log?token=${ config.codeVitalsToken }`; - const TIMEOUT_MS = 30000; // 30 second timeout + // Build the URL before attaching the token, so a malformed base host can't + // throw a parse error that echoes the secret. The token lives only on the URL + // object (via searchParams), never in a string we build or log. + let url; + try { + url = new URL( '/api/log', config.codeVitalsUrl ); + } catch { + throw new Error( 'Invalid CodeVitals URL; check the CODEVITALS_URL setting' ); + } + url.searchParams.set( 'token', config.codeVitalsToken ); + const TIMEOUT_MS = 30000; // 30 second timeout const controller = new AbortController(); const timeoutId = setTimeout( () => controller.abort(), TIMEOUT_MS ); @@ -105,9 +284,15 @@ async function postToCodeVitals( resultsPath, config ) { } ); } console.log( '✓ Metrics posted successfully to CodeVitals' ); - return data; + return { posted: true, data, validationFailed }; } catch ( error ) { clearTimeout( timeoutId ); + // Scrub the token from the caught error and its whole cause chain before we + // log it or re-use it as `cause`. fetch/undici can echo the token-bearing + // URL in a message or stack, so a caller that logs err.cause or + // util.inspect( err ) would otherwise re-expose the secret. This is defense + // in depth: safe URL construction already keeps it out of parse errors. + sanitizeErrorChain( error, config.codeVitalsToken ); const message = error.name === 'AbortError' ? `CodeVitals request timed out after ${ TIMEOUT_MS / 1000 }s` @@ -121,23 +306,29 @@ async function main() { console.log( 'CodeVitals Integration' ); console.log( '=====================\n' ); + const dryRun = process.argv.includes( '--dry-run' ); + // Configuration from environment const config = { - codeVitalsUrl: process.env.CODEVITALS_URL || 'https://www.codevitals.run', + // Default to the apex host. www.codevitals.run 301-redirects the API, and on a + // 301 fetch retries a POST as a GET with no body, so the metric never lands. + codeVitalsUrl: process.env.CODEVITALS_URL || 'https://codevitals.run', codeVitalsToken: process.env.CODEVITALS_TOKEN, gitHash: process.env.GIT_COMMIT, gitBranch: process.env.GIT_BRANCH || 'trunk', resultsPath: process.env.RESULTS_PATH || path.join( import.meta.dirname, '../results/lcp-results.json' ), + dryRun, }; - // Validate required config - if ( ! config.codeVitalsToken ) { + // A live post needs a token; a dry run does not, so CI can smoke-test it. + if ( ! dryRun && ! config.codeVitalsToken ) { console.error( 'ERROR: CODEVITALS_TOKEN environment variable is required' ); process.exit( 1 ); } console.log( 'Configuration:' ); + console.log( ` Mode: ${ dryRun ? 'DRY RUN (no POST)' : 'live post' }` ); console.log( ` CodeVitals URL: ${ config.codeVitalsUrl }` ); console.log( ` Results Path: ${ config.resultsPath }` ); console.log( ` Git Hash: ${ config.gitHash || 'unknown' }` ); @@ -145,16 +336,63 @@ async function main() { console.log( '' ); try { - await postToCodeVitals( config.resultsPath, config ); - console.log( '\n✓ All done!' ); + const result = await postToCodeVitals( config.resultsPath, config ); + if ( result.validationFailed ) { + console.error( + '\n✗ One or more metrics failed sanity checks (see above). Any valid metrics were still processed.' + ); + // Exit with the data-integrity code so the runner always fails the build + // here, even under --allow-codevitals-failure (that flag is for outages). + process.exit( VALIDATION_FAILED_EXIT_CODE ); + } + console.log( dryRun ? '\n✓ Dry run complete!' : '\n✓ All done!' ); process.exit( 0 ); } catch ( error ) { console.error( '\n✗ Failed:', error.message ); - process.exit( 1 ); + // A scenario misconfiguration (ValidationError) is local bad data and must + // always fail the build, exactly like an out-of-range metric — not a + // suppressible exit 1. exitCodeForError encodes that split. + process.exit( exitCodeForError( error ) ); } } -// Run if called directly -main(); +/** + * Whether this module was run directly (`node post-to-codevitals.js`) rather than imported. + * + * Compares real filesystem paths so spaces, non-ASCII characters, and symlinks + * (e.g. /tmp → /private/tmp) cannot make the match fail and silently skip main(). + * A raw ``file://${ process.argv[1] }`` comparison breaks on all three: Node + * percent-encodes and symlink-resolves import.meta.url but argv[1] stays raw. + * + * @param {string|undefined} moduleFilename - Absolute path of this module (import.meta.filename). + * @param {string|undefined} invokedPath - The path Node was invoked with (process.argv[1]). + * @return {boolean} True when both resolve to the same file. + */ +function isDirectInvocation( moduleFilename, invokedPath ) { + if ( ! moduleFilename || ! invokedPath ) { + return false; + } + try { + return fs.realpathSync( moduleFilename ) === fs.realpathSync( invokedPath ); + } catch { + // realpathSync throws when invokedPath does not exist (e.g. `node --test`). + return false; + } +} + +// Run only when executed directly, not when imported (e.g. by the unit tests), +// so importing the pure helpers does not trigger main()'s env checks or exits. +if ( isDirectInvocation( import.meta.filename, process.argv[ 1 ] ) ) { + main(); +} -export { postToCodeVitals }; +export { + postToCodeVitals, + checkSanityRange, + extractScenarioMetrics, + exitCodeForError, + isDirectInvocation, + redactToken, + ValidationError, + VALIDATION_FAILED_EXIT_CODE, +}; diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js new file mode 100644 index 000000000000..8dfe203b3277 --- /dev/null +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -0,0 +1,488 @@ +/** + * Unit and integration tests for the CodeVitals posting tool. + * + * CodeVitals is append-only with no rollback, so these tests pin the contract + * that keeps bad data out: checkSanityRange must fail closed on anything it + * cannot positively confirm is in range, postToCodeVitals must not let a + * rejected value into the payload, and the CLI must actually run when invoked + * directly (even from a path with a space). Run with `pnpm test:unit` + * (node's built-in runner — no Docker, no token, no network). + */ + +import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { test } from 'node:test'; +import { fileURLToPath } from 'node:url'; +import { inspect } from 'node:util'; +import { + checkSanityRange, + exitCodeForError, + extractScenarioMetrics, + isDirectInvocation, + postToCodeVitals, + redactToken, + VALIDATION_FAILED_EXIT_CODE, +} from './post-to-codevitals.js'; +import { shouldFailBuildOnPostError } from './run-performance-tests.js'; + +const SCRIPTS_DIR = path.dirname( fileURLToPath( import.meta.url ) ); +const LCP_KEY = 'wp-admin-dashboard-connection-sim-largestContentfulPaint'; + +/** Write a results fixture with the given median LCP and return its path. */ +function writeResults( median ) { + const dir = fs.mkdtempSync( path.join( os.tmpdir(), 'cv-results-' ) ); + const file = path.join( dir, 'results.json' ); + fs.writeFileSync( + file, + JSON.stringify( { + git: { hash: 'testhash', branch: 'trunk' }, + measurements: { jetpackConnected: { summary: { median } } }, + } ) + ); + return file; +} + +/** Run a function with console output suppressed to keep test output readable. */ +async function silenced( fn ) { + const orig = { log: console.log, error: console.error, warn: console.warn }; + console.log = () => {}; + console.error = () => {}; + console.warn = () => {}; + try { + return await fn(); + } finally { + Object.assign( console, orig ); + } +} + +// --- checkSanityRange (the guard) --- + +test( 'in-range typed value passes', () => { + assert.equal( checkSanityRange( 'lcp', 120 ).ok, true ); +} ); + +test( 'range boundaries are inclusive', () => { + assert.equal( checkSanityRange( 'lcp', 100 ).ok, true ); // min + assert.equal( checkSanityRange( 'lcp', 60000 ).ok, true ); // max +} ); + +test( 'out-of-range value is rejected', () => { + assert.equal( checkSanityRange( 'lcp', 99 ).ok, false ); // below min + assert.equal( checkSanityRange( 'lcp', 60001 ).ok, false ); // above max +} ); + +test( 'a typed metric with no range row fails closed (typo / forgotten row)', () => { + assert.equal( checkSanityRange( 'lcpp', 120 ).ok, false ); // typo + assert.equal( checkSanityRange( 'LCP', 120 ).ok, false ); // case mismatch, lookup is exact +} ); + +test( 'non-finite values are rejected, including on min-0 ranges', () => { + assert.equal( checkSanityRange( 'tbt', null ).ok, false ); // null coerces to 0; must not pass + assert.equal( checkSanityRange( 'cls', null ).ok, false ); + assert.equal( checkSanityRange( 'lcp', NaN ).ok, false ); + assert.equal( checkSanityRange( 'lcp', Infinity ).ok, false ); + assert.equal( checkSanityRange( 'lcp', undefined ).ok, false ); +} ); + +test( 'numeric strings are rejected rather than posted as strings', () => { + assert.equal( checkSanityRange( 'lcp', '120' ).ok, false ); +} ); + +test( 'a finite untyped legacy entry passes unchecked', () => { + assert.equal( checkSanityRange( undefined, 999999 ).ok, true ); +} ); + +test( 'a non-finite value is rejected even for an untyped legacy entry', () => { + assert.equal( checkSanityRange( undefined, null ).ok, false ); + assert.equal( checkSanityRange( undefined, NaN ).ok, false ); +} ); + +// --- extractScenarioMetrics --- + +test( 'explicit metricKey yields one typed entry', () => { + const entries = extractScenarioMetrics( + { metricKey: 'wp-admin-lcp', metricType: 'lcp' }, + { median: 120 } + ); + assert.deepEqual( entries, [ { key: 'wp-admin-lcp', value: 120, type: 'lcp' } ] ); +} ); + +test( 'legacy prefix yields five untyped entries', () => { + const entries = extractScenarioMetrics( + { metricPrefix: 'bar' }, + { median: 1, mean: 2, min: 3, max: 4, stdDev: 5 } + ); + assert.equal( entries.length, 5 ); + assert.deepEqual( entries[ 0 ], { key: 'bar_ms', value: 1 } ); + assert.equal( + entries.every( e => e.type === undefined ), + true + ); +} ); + +test( 'a keyed scenario without a metricType is rejected, not posted unchecked', () => { + // Without this guard the entry would emit type:undefined and pass the range + // check as a "legacy" metric — posting any finite value to an append-only store. + assert.throws( + () => extractScenarioMetrics( { key: 'future', metricKey: 'future-key' }, { median: 999999 } ), + /metricKey but no metricType/ + ); +} ); + +test( 'a scenario misconfiguration maps to the validation exit code, a transport error to 1', () => { + let configError; + try { + extractScenarioMetrics( { key: 'future', metricKey: 'future-key' }, { median: 1 } ); + } catch ( e ) { + configError = e; + } + // A keyed-without-metricType config error is local bad data: it must always fail + // the build (the validation code), exactly like an out-of-range metric — never a + // suppressible transport exit 1. Otherwise --allow-codevitals-failure could mask it. + assert.equal( configError?.name, 'ValidationError' ); + assert.equal( exitCodeForError( configError ), VALIDATION_FAILED_EXIT_CODE ); + assert.equal( exitCodeForError( new Error( 'CodeVitals API error (500)' ) ), 1 ); +} ); + +// --- redactToken (keeps the token out of logs and errors) --- + +test( 'redactToken strips the exact token and any token query param', () => { + assert.equal( + redactToken( 'prefix token=abc123 suffix', 'abc123' ), + 'prefix token=REDACTED suffix' + ); + assert.equal( + redactToken( 'https://h/api/log?token=zzz&n=1' ), + 'https://h/api/log?token=REDACTED&n=1' + ); + assert.equal( redactToken( 'no secret in here', 'abc123' ), 'no secret in here' ); +} ); + +// --- postToCodeVitals (the accumulation loop + exit semantics) --- + +test( 'dry-run posts an in-range metric into the payload, nothing rejected', async () => { + const file = writeResults( 120 ); + const result = await silenced( () => postToCodeVitals( file, { dryRun: true } ) ); + assert.equal( result.posted, false ); // dry run never posts + assert.equal( result.validationFailed, false ); + assert.equal( result.payload.metrics[ LCP_KEY ], 120 ); +} ); + +test( 'a dry run never calls fetch, even with a malformed URL and a token set', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + // Poison fetch: a dry run must short-circuit before any network call. + global.fetch = async () => { + throw new Error( 'fetch must not be called during a dry run' ); + }; + try { + const result = await silenced( () => + postToCodeVitals( file, { + dryRun: true, + codeVitalsUrl: 'http://[::1', // malformed; must not even be constructed + codeVitalsToken: 'tok-dry', + } ) + ); + assert.equal( result.posted, false ); + assert.equal( result.payload.metrics[ LCP_KEY ], 120 ); + } finally { + global.fetch = origFetch; + } +} ); + +test( 'an out-of-range metric is skipped and flags validationFailed', async () => { + const file = writeResults( 70000 ); + const result = await silenced( () => postToCodeVitals( file, { dryRun: true } ) ); + assert.equal( result.posted, false ); + assert.equal( result.validationFailed, true ); // the only metric was rejected +} ); + +test( 'a non-finite metric is rejected through the posting path', async () => { + const file = writeResults( null ); + const result = await silenced( () => postToCodeVitals( file, { dryRun: true } ) ); + assert.equal( result.validationFailed, true ); +} ); + +test( 'a missing results file throws', async () => { + await assert.rejects( + () => silenced( () => postToCodeVitals( '/no/such/results.json', { dryRun: true } ) ), + /Results file not found/ + ); +} ); + +// --- live POST branch (fetch stubbed; never touches the network) --- + +test( 'a live post sends the payload as POST and returns posted:true', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + let sentUrl, sentInit; + global.fetch = async ( u, init ) => { + sentUrl = u; + sentInit = init; + return { ok: true, status: 200, json: async () => ( { ok: true } ), text: async () => '' }; + }; + try { + const result = await silenced( () => + postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'https://codevitals.test', + codeVitalsToken: 'tok-success', + } ) + ); + assert.equal( result.posted, true ); + assert.equal( sentInit.method, 'POST' ); + assert.match( String( sentUrl ), /\/api\/log\?token=tok-success$/ ); + assert.equal( JSON.parse( sentInit.body ).metrics[ LCP_KEY ], 120 ); + } finally { + global.fetch = origFetch; + } +} ); + +test( 'a non-OK CodeVitals response throws without leaking the token, even from the body', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + // A hostile/echoing error body that includes the token query param must still + // be scrubbed everywhere, not just in the top-level message. + global.fetch = async () => ( { + ok: false, + status: 500, + text: async () => 'boom for token=tok-secret-500', + } ); + try { + await assert.rejects( + () => + silenced( () => + postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'https://codevitals.test', + codeVitalsToken: 'tok-secret-500', + } ) + ), + err => { + assert.match( err.message, /CodeVitals API error \(500\)/ ); + assert.ok( ! err.message.includes( 'tok-secret-500' ), 'message must be scrubbed' ); + assert.ok( + ! ( err.cause?.message ?? '' ).includes( 'tok-secret-500' ), + 'cause must be scrubbed' + ); + assert.ok( + ! inspect( err, { depth: 5 } ).includes( 'tok-secret-500' ), + 'util.inspect must be scrubbed' + ); + return true; + } + ); + } finally { + global.fetch = origFetch; + } +} ); + +test( 'a malformed CODEVITALS_URL leaks the token into neither the error nor the logs', async () => { + const file = writeResults( 120 ); + const captured = []; + const orig = { log: console.log, error: console.error, warn: console.warn }; + console.log = () => {}; + console.warn = () => {}; + console.error = ( ...a ) => captured.push( a.join( ' ' ) ); + let thrown = ''; + try { + await postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'http://[::1', // malformed on purpose + codeVitalsToken: 'tok-must-not-leak', + } ); + } catch ( e ) { + thrown = e.message; + } finally { + Object.assign( console, orig ); + } + assert.ok( ! thrown.includes( 'tok-must-not-leak' ), 'token must not be in the thrown message' ); + assert.ok( + ! captured.join( '\n' ).includes( 'tok-must-not-leak' ), + 'token must not be in console output' + ); +} ); + +test( 'a token-bearing upstream error is redacted in the message, the cause, and util.inspect', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + // Simulate an upstream (e.g. undici) error that echoes the full request URL, + // token included, in its message AND in a nested cause. + global.fetch = async u => { + const inner = new Error( `connect to ${ String( u ) } refused` ); + throw new Error( `request to ${ String( u ) } failed`, { cause: inner } ); + }; + let err; + try { + await silenced( () => + postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'https://codevitals.test', + codeVitalsToken: 'tok-cause-leak', + } ) + ); + } catch ( e ) { + err = e; + } finally { + global.fetch = origFetch; + } + assert.ok( err, 'the live post should reject' ); + assert.ok( ! err.message.includes( 'tok-cause-leak' ), 'token must not be in the message' ); + assert.ok( + ! ( err.cause?.message ?? '' ).includes( 'tok-cause-leak' ), + 'token must not be in the cause message' + ); + assert.ok( + ! inspect( err, { depth: 5 } ).includes( 'tok-cause-leak' ), + 'token must not survive in util.inspect of the whole error' + ); +} ); + +test( 'a token on a custom error property (and nested cause) is scrubbed too', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + // Native fetch never does this, but a non-native client might stash the + // token-bearing URL on a custom field instead of (or besides) the message. + global.fetch = async u => { + const inner = new Error( 'socket closed' ); + inner.url = String( u ); // token-bearing, on a nested cause + const outer = new Error( 'request failed', { cause: inner } ); + outer.requestUrl = String( u ); // token-bearing, on the top-level error + throw outer; + }; + let err; + try { + await silenced( () => + postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'https://codevitals.test', + codeVitalsToken: 'tok-on-prop', + } ) + ); + } catch ( e ) { + err = e; + } finally { + global.fetch = origFetch; + } + assert.ok( err, 'the live post should reject' ); + assert.ok( + ! inspect( err, { depth: 5 } ).includes( 'tok-on-prop' ), + 'token must not survive on any custom property across the cause chain' + ); +} ); + +test( 'a token-bearing primitive string cause is scrubbed too', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + // `Error.cause` accepts any value. A non-native client may throw + // `new Error( msg, { cause: someUrl } )` where the cause is a plain string, not an + // Error. cause is non-enumerable, so this must be caught by the chain walk, not the + // own-property pass — the gap this regression test pins. + global.fetch = async u => { + throw new Error( 'request failed', { cause: `connect to ${ String( u ) } refused` } ); + }; + let err; + try { + await silenced( () => + postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'https://codevitals.test', + codeVitalsToken: 'tok-string-cause', + } ) + ); + } catch ( e ) { + err = e; + } finally { + global.fetch = origFetch; + } + assert.ok( err, 'the live post should reject' ); + assert.ok( + ! inspect( err, { depth: 5 } ).includes( 'tok-string-cause' ), + 'token must not survive in a primitive string cause' + ); +} ); + +// --- isDirectInvocation (the run-when-direct guard) --- + +test( 'isDirectInvocation: a file matches itself', () => { + const real = fileURLToPath( import.meta.url ); + assert.equal( isDirectInvocation( real, real ), true ); +} ); + +test( 'isDirectInvocation: a non-existent invocation path is not a direct run', () => { + assert.equal( isDirectInvocation( fileURLToPath( import.meta.url ), '/no/such/path' ), false ); +} ); + +test( 'isDirectInvocation: a missing argv is not a direct run', () => { + assert.equal( isDirectInvocation( fileURLToPath( import.meta.url ), undefined ), false ); +} ); + +// --- CLI end to end (R2-C regression: must run from a path with a space) --- + +test( 'the CLI runs main() when invoked directly from a path containing a space', () => { + const dir = fs.mkdtempSync( path.join( os.tmpdir(), 'cv has space-' ) ); + fs.copyFileSync( + path.join( SCRIPTS_DIR, 'post-to-codevitals.js' ), + path.join( dir, 'post-to-codevitals.js' ) + ); + fs.copyFileSync( path.join( SCRIPTS_DIR, 'scenarios.js' ), path.join( dir, 'scenarios.js' ) ); + // Declare the temp copy as ESM. Without this, a `.js` file with `import` only + // runs as a module on Node versions where syntax detection is on by default + // (22.7+); on the lower half of this package's >=20.11 range it throws + // "Cannot use import statement outside a module" and the test fails spuriously. + fs.writeFileSync( path.join( dir, 'package.json' ), '{ "type": "module" }' ); + const results = writeResults( 120 ); + try { + const out = execFileSync( 'node', [ path.join( dir, 'post-to-codevitals.js' ), '--dry-run' ], { + encoding: 'utf8', + env: { ...process.env, RESULTS_PATH: results }, + } ); + assert.match( out, /DRY RUN/ ); // empty output would mean main() was skipped + assert.match( out, new RegExp( LCP_KEY ) ); + } finally { + fs.rmSync( dir, { recursive: true, force: true } ); + } +} ); + +test( 'the CLI exits with the validation code on an out-of-range dry run', () => { + const results = writeResults( 70000 ); // LCP far outside [100, 60000] + const env = { ...process.env, RESULTS_PATH: results }; + delete env.CODEVITALS_TOKEN; // a dry run needs no token; prove it still fails closed + let status, output; + try { + execFileSync( 'node', [ path.join( SCRIPTS_DIR, 'post-to-codevitals.js' ), '--dry-run' ], { + encoding: 'utf8', + env, + stdio: [ 'ignore', 'pipe', 'pipe' ], + } ); + status = 0; + } catch ( err ) { + status = err.status; + output = `${ err.stdout ?? '' }${ err.stderr ?? '' }`; + } + // CI keys on the exit code; an out-of-range metric must use the data-integrity + // code (not a generic 1) so the runner can never suppress it. + assert.equal( status, VALIDATION_FAILED_EXIT_CODE ); + assert.match( output, /Sanity check failed/ ); +} ); + +// --- run-performance-tests.js: the build-fail decision (cross-file contract) --- +// The poster sets the exit code; the runner decides whether it fails the build. +// These pin that the validation/outage split survives a future runner refactor. + +test( 'shouldFailBuildOnPostError: a validation failure (exit 2) is always fatal', () => { + const validation = { status: VALIDATION_FAILED_EXIT_CODE }; + // Even with --allow-codevitals-failure set, local bad data must fail the build. + assert.equal( shouldFailBuildOnPostError( validation, true ), true ); + assert.equal( shouldFailBuildOnPostError( validation, false ), true ); +} ); + +test( 'shouldFailBuildOnPostError: a transport failure is suppressible only with the flag', () => { + assert.equal( shouldFailBuildOnPostError( { status: 1 }, true ), false ); // outage tolerated + assert.equal( shouldFailBuildOnPostError( { status: 1 }, false ), true ); // fatal by default + assert.equal( shouldFailBuildOnPostError( undefined, true ), false ); // unknown exit, flag set + assert.equal( shouldFailBuildOnPostError( undefined, false ), true ); +} ); diff --git a/tools/performance/scripts/run-performance-tests.js b/tools/performance/scripts/run-performance-tests.js index 1a0216e4b81a..9c168397cdbe 100644 --- a/tools/performance/scripts/run-performance-tests.js +++ b/tools/performance/scripts/run-performance-tests.js @@ -8,6 +8,7 @@ import { execFileSync } from 'child_process'; import fs from 'fs'; import path from 'path'; import { config as dotenvConfig } from 'dotenv'; +import { isDirectInvocation, VALIDATION_FAILED_EXIT_CODE } from './post-to-codevitals.js'; import { SCENARIOS, getScenarioUrl } from './scenarios.js'; // Load .env file from the performance directory if it exists @@ -44,6 +45,23 @@ function execFile( cmd, args = [], options = {} ) { } } +/** + * Whether a failed CodeVitals post must fail the build. + * + * A local data-integrity failure — the poster exits VALIDATION_FAILED_EXIT_CODE when + * a metric is rejected or a scenario is misconfigured — is always fatal. + * `--allow-codevitals-failure` exists only to tolerate a CodeVitals network outage + * (any other non-zero exit), never to ship a build that skipped bad local data. + * + * @param {{status?: number}|undefined} err - The error the poster child threw (carries its exit code on `.status`). + * @param {boolean} allowCodeVitalsFailure - Whether `--allow-codevitals-failure` was passed. + * @return {boolean} True if the build must fail. + */ +function shouldFailBuildOnPostError( err, allowCodeVitalsFailure ) { + const isValidationFailure = err?.status === VALIDATION_FAILED_EXIT_CODE; + return isValidationFailure || ! allowCodeVitalsFailure; +} + /** Execute a docker compose command. */ function dockerCompose( args, options = {} ) { const baseArgs = [ @@ -452,16 +470,23 @@ async function main() { try { execFile( 'node', [ path.join( __dirname, 'post-to-codevitals.js' ) ] ); - } catch { - // When CODEVITALS_TOKEN is explicitly set, posting failures should fail the build - // to ensure CI doesn't silently drop metrics (unless --allow-codevitals-failure is set) + } catch ( err ) { + // A local data-integrity failure (the child exits VALIDATION_FAILED_EXIT_CODE + // when a metric is rejected or a scenario is misconfigured) must always fail + // the build. --allow-codevitals-failure is for tolerating CodeVitals network + // outages, not for shipping a build that silently skipped bad local data. + const isValidationFailure = err?.status === VALIDATION_FAILED_EXIT_CODE; console.error( '\n✗ Failed to post to CodeVitals' ); - if ( options.allowCodeVitalsFailure ) { + if ( ! shouldFailBuildOnPostError( err, options.allowCodeVitalsFailure ) ) { console.warn( ' Continuing despite failure (--allow-codevitals-failure set)' ); } else { - console.error( ' Since CODEVITALS_TOKEN is set, this is treated as a build failure.' ); - console.error( ' Use --skip-codevitals to run without posting metrics.' ); - console.error( ' Use --allow-codevitals-failure to continue on posting failures.' ); + if ( isValidationFailure ) { + console.error( ' A metric failed local validation; this always fails the build.' ); + } else { + console.error( ' Since CODEVITALS_TOKEN is set, this is treated as a build failure.' ); + console.error( ' Use --skip-codevitals to run without posting metrics.' ); + console.error( ' Use --allow-codevitals-failure to continue on posting failures.' ); + } process.exit( 1 ); } } @@ -482,15 +507,19 @@ async function main() { if ( process.env.CODEVITALS_TOKEN ) { console.log( 'View detailed results in CodeVitals:' ); - console.log( - ` ${ process.env.CODEVITALS_URL || 'https://www.codevitals.run' }/project/jetpack` - ); + console.log( ` ${ process.env.CODEVITALS_URL || 'https://codevitals.run' }/project/jetpack` ); console.log( '' ); } } -// Run -main().catch( error => { - console.error( 'Fatal error:', error ); - process.exit( 1 ); -} ); +// Run only when invoked directly, not when imported (e.g. by the unit test that +// exercises shouldFailBuildOnPostError), so importing this module never spins up +// Docker or kicks off a full perf run. +if ( isDirectInvocation( import.meta.filename, process.argv[ 1 ] ) ) { + main().catch( error => { + console.error( 'Fatal error:', error ); + process.exit( 1 ); + } ); +} + +export { shouldFailBuildOnPostError }; diff --git a/tools/performance/scripts/scenarios.js b/tools/performance/scripts/scenarios.js index 1457cc6dce86..0814f1d7503c 100644 --- a/tools/performance/scripts/scenarios.js +++ b/tools/performance/scripts/scenarios.js @@ -23,12 +23,36 @@ export const SCENARIOS = [ defaultUrl: 'http://localhost:8083', header: 'Jetpack Connected (Simulated + 200ms Latency)', metricPrefix: 'wp_admin_lcp_jetpack_connected', + // CodeVitals key for the posted metric. When introducing a NEW metric, post + // it to a `-staging` key first (e.g. `…-timeToFirstByte-staging`) for 2-3 + // builds, inspect it in the CodeVitals UI, then rename to the production key. + // See the "Safeguards" section of README.md for the full convention. metricKey: 'wp-admin-dashboard-connection-sim-largestContentfulPaint', + // Metric type — drives the sanity-range check in post-to-codevitals.js. + metricType: 'lcp', postToCodeVitals: true, isBaseline: false, }, ]; +/** + * Sanity ranges for posted metrics, keyed by metric type. + * + * post-to-codevitals.js checks every typed metric against these bounds before + * posting. A value outside its range is logged and skipped (never posted), + * because CodeVitals is append-only and bad points cannot be rolled back. + * Add a row when a new metric type starts being posted. + * + * @type {Object} + */ +export const SANITY_RANGES = { + lcp: { min: 100, max: 60000 }, // <100ms is suspicious; >60s means the page never loaded. + ttfb: { min: 10, max: 10000 }, // <10ms is unrealistic; >10s means server failure. + fcp: { min: 50, max: 30000 }, + tbt: { min: 0, max: 10000 }, // Can legitimately be 0; >10s is catastrophic. + cls: { min: 0, max: 5 }, // >5 would mean the page is unusable. +}; + /** * Get the URL for a scenario from environment or default *