-
Notifications
You must be signed in to change notification settings - Fork 54
Add a script to benchmark site editor performance #2368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Add a script to benchmark site editor performance #2368
Conversation
| results.forEach( ( result ) => { | ||
| const urlKey = result.url || 'unknown'; | ||
| // Create a short identifier from the URL (e.g., "localhost:8888" or "playground-web") | ||
| const urlIdentifier = urlKey.includes( 'playground.wordpress.net' ) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
playground.wordpress.net
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
In general, to avoid incomplete URL substring sanitization, parse the URL and make decisions based on its host (or hostname) rather than searching for substrings in the full URL string. If you need to detect a specific domain (or one of its subdomains), compare the parsed host against an explicit list or check that it matches exactly or ends with a known suffix and a dot boundary.
In this specific file, the problematic logic is the creation of urlIdentifier:
const urlIdentifier = urlKey.includes( 'playground.wordpress.net' )
? 'playground-web'
: urlKey
.replace( /^https?:\/\//, '' )
.replace( /\/$/, '' )
.replace( /[^a-z0-9]/gi, '-' );We should instead parse urlKey with the standard URL class, extract hostname, and then decide whether the hostname is exactly playground.wordpress.net or a subdomain such as foo.playground.wordpress.net. Then we construct urlIdentifier from the hostname/path as before for all other hosts. To keep behavior similar and avoid assumptions about other code, we’ll:
- Add a small helper function
getUrlIdentifier(urlKey: string): stringin this test file. - Inside it, safely parse
urlKeyusing the built-inURLconstructor when possible; on parse failure, fall back to the existing string-based transformation. - Determine whether the parsed hostname equals
playground.wordpress.netor ends with.playground.wordpress.net. If so, return'playground-web'; otherwise, apply the samereplacechain as before on the originalurlKey.
This change is entirely local to metrics/tests/site-editor-benchmark.test.ts and does not require modifying other files.
-
Copy modified lines R17-R41 -
Copy modified line R56
| @@ -14,6 +14,31 @@ | ||
| }; | ||
| } | ||
|
|
||
| function getUrlIdentifier( urlKey: string ): string { | ||
| const FALLBACK_IDENTIFIER = urlKey | ||
| .replace( /^https?:\/\//, '' ) | ||
| .replace( /\/$/, '' ) | ||
| .replace( /[^a-z0-9]/gi, '-' ); | ||
|
|
||
| try { | ||
| const parsed = new URL( urlKey ); | ||
| const hostname = parsed.hostname || ''; | ||
|
|
||
| // Treat playground.wordpress.net and its subdomains as playground-web | ||
| if ( | ||
| hostname === 'playground.wordpress.net' || | ||
| hostname.endsWith( '.playground.wordpress.net' ) | ||
| ) { | ||
| return 'playground-web'; | ||
| } | ||
|
|
||
| return FALLBACK_IDENTIFIER; | ||
| } catch { | ||
| // If urlKey is not a valid URL, fall back to the original string-based normalization | ||
| return FALLBACK_IDENTIFIER; | ||
| } | ||
| } | ||
|
|
||
| test.describe( 'Site Editor Performance Benchmark', () => { | ||
| const results: BenchmarkResults[] = []; | ||
|
|
||
| @@ -28,12 +53,7 @@ | ||
| results.forEach( ( result ) => { | ||
| const urlKey = result.url || 'unknown'; | ||
| // Create a short identifier from the URL (e.g., "localhost:8888" or "playground-web") | ||
| const urlIdentifier = urlKey.includes( 'playground.wordpress.net' ) | ||
| ? 'playground-web' | ||
| : urlKey | ||
| .replace( /^https?:\/\//, '' ) | ||
| .replace( /\/$/, '' ) | ||
| .replace( /[^a-z0-9]/gi, '-' ); | ||
| const urlIdentifier = getUrlIdentifier( urlKey ); | ||
|
|
||
| Object.entries( result.metrics ).forEach( ( [ key, value ] ) => { | ||
| if ( value !== undefined ) { |
| }; | ||
|
|
||
| // Check if this is playground.wordpress.net (runs in iframe) | ||
| const isPlaygroundWeb = wpAdminUrl.includes( 'playground.wordpress.net' ); |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
playground.wordpress.net
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
In general, to fix this kind of issue you should parse the URL string into a URL object and perform host-based checks on the parsed hostname (and possibly protocol), rather than doing substring checks on the full URL text. That ensures that you only match the actual host, not occurrences in the path, query, or a different host name (e.g., example.com.evil.com).
For this specific file, the best fix is to replace wpAdminUrl.includes('playground.wordpress.net') with a check on the parsed hostname of wpAdminUrl. Since this is Node/TypeScript test code, we can safely use the built-in URL class (new URL(...)). The logic should remain functionally equivalent for valid URLs that actually point to Playground, but avoid accidental matches. A robust, minimal-change fix is:
- Ensure
wpAdminUrlis a fully qualified URL (already done: it addshttp://if missing). - Wrap it in
new URL(wpAdminUrl)to parse. - Compare
url.hostnameto'playground.wordpress.net'(and optionally accept the bare hostname without protocol or with trailing slash, but in this case we already normalized).
Concretely:
- In
metrics/tests/site-editor-benchmark.test.ts, around line 88–90, change:
// Check if this is playground.wordpress.net (runs in iframe)
const isPlaygroundWeb = wpAdminUrl.includes( 'playground.wordpress.net' );to:
// Check if this is playground.wordpress.net (runs in iframe) using hostname, not substring
let isPlaygroundWeb = false;
try {
const parsedUrl = new URL( wpAdminUrl );
isPlaygroundWeb = parsedUrl.hostname === 'playground.wordpress.net';
} catch {
isPlaygroundWeb = false;
}This keeps existing behavior for proper Playground URLs, avoids substring pitfalls, and gracefully handles malformed URLs by treating them as non-Playground. No new imports are necessary because URL is globally available in modern Node, and we are not changing any other behavior or public API.
-
Copy modified lines R88-R95
| @@ -85,8 +85,14 @@ | ||
| metrics: {}, | ||
| }; | ||
|
|
||
| // Check if this is playground.wordpress.net (runs in iframe) | ||
| const isPlaygroundWeb = wpAdminUrl.includes( 'playground.wordpress.net' ); | ||
| // Check if this is playground.wordpress.net (runs in iframe) using hostname, not substring | ||
| let isPlaygroundWeb = false; | ||
| try { | ||
| const parsedUrl = new URL( wpAdminUrl ); | ||
| isPlaygroundWeb = parsedUrl.hostname === 'playground.wordpress.net'; | ||
| } catch { | ||
| isPlaygroundWeb = false; | ||
| } | ||
| let playgroundFrame: Frame | null = null; | ||
|
|
||
| try { |
25691e3 to
864f0b3
Compare
use a blueprint when testing playground web to unify the test flow
…tu-1199-studio-write-a-script-to-benchmark-site-editor-performance
Related issues
Proposed Changes
Add a Playwright test that benchmarks the performance of some actions within the Site Editor. It measures load times for each step of a typical site editing workflow.
The benchmark measures the time taken for the following steps:
/wp-adminwith auto-loginWe can use the reported timings to compare load times across different environments (studio, playground cli, playground.wordpress.net)
Testing Instructions
Prerequisites
npx @wp-playground/cli@latest server)Running the test
Set the
BENCHMARK_URLenvironment variable to point to the WordPress instance:BENCHMARK_URL=http://localhost:8888 npx playwright test --config=./metrics/playwright.metrics.config.ts site-editor-benchmarkOutput
After running the test, results are automatically printed to the console as a table, and also saved to
metrics/artifacts/site-editor-benchmark.results.json.Pre-merge Checklist