From 93a407e45ff858dcfea09d4af866c0d1a2e8761d Mon Sep 17 00:00:00 2001 From: Danila Poyarkov Date: Thu, 10 Sep 2026 21:41:10 +0300 Subject: [PATCH] test: isolate managed browser and MCP runtimes --- AGENTS.md | 4 ++- packages/docs/development/testing.md | 15 +++++++++ playwright.config.ts | 28 +++++++++++++--- src/app/automation/bridge/portless-route.ts | 5 +-- src/app/automation/bridge/vite-plugin.ts | 26 ++++++++++++--- .../app/automation/mcp-vite-plugin.test.ts | 33 +++++++++++++++++++ .../app/automation/portless-route.test.ts | 9 +++++ vite.config.ts | 10 +++--- vite/automation.ts | 22 +++++++++---- 9 files changed, 130 insertions(+), 22 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0b84afee8..10028b174 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -76,6 +76,8 @@ App dialogs compose the Reka-backed components under `src/components/ui/dialog/` Prefer `dev:portless`, especially in worktrees. It assigns branch-specific app and `mcp.open-pencil` sibling URLs with isolated runtime discovery. Use fixed-port `dev` only for Playwright, Tauri, and Dev Container flows. +Browser tests use the canonical `playwright.config.ts`; do not create task-specific config copies or server runners. Managed runs must start the intended checkout, with server reuse explicitly opted into only for local development, never baseline comparisons or CI. Isolate the app URL and MCP endpoint, CORS origin, socket, and discovery path together. Playwright owns Vite; the existing Vite automation plugin owns MCP startup and cleanup; browser fixtures own interactions, not server processes. See `packages/docs/development/testing.md` for configuration and commands. + ## Releases & CI For releases, update versions in the root and publishable package manifests plus `desktop/tauri.conf.json` and `desktop/Cargo.toml`; move `Unreleased` into `## x.y.z — YYYY-MM-DD`; commit `Release vX.Y.Z`; then tag and push `vX.Y.Z`. @@ -136,7 +138,7 @@ Use Conventional Commits (`feat`, `fix`, `refactor`, `perf`, `docs`, `test`, `bu Private tooling belongs under `tools//{src,tests}`, with kebab-case domains and focused tests. `scripts/` may contain only tiny compatibility entrypoints; put real workflow, release, architecture, package, or visual tooling in `tools/`. -- Use `@/` for app cross-directory imports. Package aliases are `#vue/*`, `#cli/*`, `#dom-css/*`, `#mcp/*`, and `#core/*`; prefer clear relative imports nearby. +- Use `@/` for app cross-directory imports. Never escape an alias root with `../` (for example `#tests/../vite`); fix module ownership instead. Package aliases are `#vue/*`, `#cli/*`, `#dom-css/*`, `#mcp/*`, and `#core/*`; prefer clear relative imports nearby. - No `any`, non-null assertions, or `Math.random()`; use precise types, guards, and `crypto.getRandomValues()`. - Reuse named types and primitives from `@open-pencil/scene-graph`; do not respell `Color`, `Vector`, `SceneNode`, `Effect`, `Fill`, or `Stroke` shapes. - Window API augmentations belong in the owning compilation boundary: app declarations in `src/global.d.ts`, package DOM gaps in the owning package's `global.d.ts`, and native-test declarations in `tests/helpers/tauri/native-global.d.ts`. Never put `declare global` in specs or implementation modules. Include canonical declarations through tsconfig instead of duplicating them. diff --git a/packages/docs/development/testing.md b/packages/docs/development/testing.md index d4c754252..eeb2b3761 100644 --- a/packages/docs/development/testing.md +++ b/packages/docs/development/testing.md @@ -17,6 +17,21 @@ bun run test # Run tests, compare against baselines bun run test:update # Regenerate baseline screenshots ``` +### Server ownership and worktrees + +The canonical `playwright.config.ts` starts Vite from the current checkout and waits for its HTTP URL. Vite starts and stops its MCP companion. Server reuse is off by default and always off in CI, so a test run cannot silently attach to another checkout on the default port. + +Defaults are app port `1420` and MCP port `7600`. For concurrent worktrees, choose a free, distinct pair: + +```sh +OPENPENCIL_TEST_PORT=1482 OPENPENCIL_TEST_MCP_PORT=7682 \ + bunx playwright test tests/e2e/settings --project=openpencil +``` + +The configuration passes the app origin and MCP port to Vite; the companion receives matching CORS configuration and a port-specific socket/discovery directory. Port conflicts fail rather than silently selecting another endpoint. Do not reuse ports across concurrent runs. + +For intentional local debugging against an already-running matching server, set `OPENPENCIL_TEST_REUSE_SERVER=1`. Do not use reuse for baseline comparisons: HTTP readiness does not establish checkout identity. Start a matching custom-port preview with `OPENPENCIL_DEV_ORIGIN=http://localhost:1482 OPENPENCIL_DEV_MCP_PORT=7682 bun run dev --port 1482`. Portless remains the preferred interactive worktree preview workflow, separate from managed fixed-port tests. + ### How It Works 1. Tests load the editor in a headless browser diff --git a/playwright.config.ts b/playwright.config.ts index 4bc563a78..3af85fec8 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -1,5 +1,19 @@ import { defineConfig } from '@playwright/test' +const appPort = process.env.OPENPENCIL_TEST_PORT ?? '1420' +const mcpPort = process.env.OPENPENCIL_TEST_MCP_PORT ?? '7600' +for (const port of [appPort, mcpPort]) { + if (!/^\d+$/.test(port) || Number(port) < 1024 || Number(port) > 65535) { + throw new Error('Browser test ports must be integers between 1024 and 65535') + } +} +if (Number(appPort) === Number(mcpPort)) throw new Error('App and MCP test ports must differ') +const origin = `http://localhost:${appPort}` +const reuse = process.env.OPENPENCIL_TEST_REUSE_SERVER +if (reuse !== undefined && reuse !== '0' && reuse !== '1') { + throw new Error('OPENPENCIL_TEST_REUSE_SERVER must be 0 or 1') +} + export default defineConfig({ testDir: './tests', timeout: 15_000, @@ -15,7 +29,7 @@ export default defineConfig({ } }, use: { - baseURL: 'http://localhost:1420', + baseURL: origin, testIdAttribute: 'data-test-id', viewport: { width: 1280, height: 800 }, deviceScaleFactor: 2, @@ -50,8 +64,14 @@ export default defineConfig({ } ], webServer: { - command: 'bun run dev', - port: 1420, - reuseExistingServer: true + command: `bun run dev --port ${appPort} --strictPort`, + cwd: import.meta.dirname, + url: origin, + env: { + OPENPENCIL_DEV_ORIGIN: origin, + OPENPENCIL_DEV_MCP_PORT: mcpPort, + PORTLESS_URL: '' + }, + reuseExistingServer: !process.env.CI && reuse === '1' } }) diff --git a/src/app/automation/bridge/portless-route.ts b/src/app/automation/bridge/portless-route.ts index 6858037a6..9e51e438d 100644 --- a/src/app/automation/bridge/portless-route.ts +++ b/src/app/automation/bridge/portless-route.ts @@ -10,12 +10,13 @@ const MCP_SERVICE_NAME = `mcp.${APP_NAME}` export function devAutomationRoute( portlessURL: string | undefined, - fallbackPort: number + fallbackPort: number, + fallbackOrigin = 'http://localhost:1420' ): DevAutomationRoute { if (!portlessURL) { return { browserURL: `ws://127.0.0.1:${fallbackPort}`, - corsOrigin: 'http://localhost:1420', + corsOrigin: fallbackOrigin, portlessServiceName: null, runtimeId: `localhost-${fallbackPort}` } diff --git a/src/app/automation/bridge/vite-plugin.ts b/src/app/automation/bridge/vite-plugin.ts index fb5af73aa..93082df21 100644 --- a/src/app/automation/bridge/vite-plugin.ts +++ b/src/app/automation/bridge/vite-plugin.ts @@ -102,13 +102,24 @@ export async function readDevMCPConfiguration(request: IncomingMessage): Promise export async function waitForAutomationHealth( browserURL: string, - fetcher: typeof fetch = fetch + fetcher: typeof fetch = fetch, + options: { authToken?: string | null; assertRunning?: () => void } = {} ): Promise { const healthURL = `${browserURL.replace(/^ws/, 'http')}/health` for (let attempt = 0; attempt < CHILD_HEALTH_ATTEMPTS; attempt++) { + options.assertRunning?.() try { - const response = await fetcher(healthURL) - if (response.ok) return + const response = await fetcher(healthURL, { + signal: AbortSignal.timeout(2000), + headers: options.authToken ? { Authorization: `Bearer ${options.authToken}` } : undefined + }) + if (response.ok) { + const authenticated = + !options.authToken || + Array.isArray(((await response.json()) as { tools?: unknown }).tools) + options.assertRunning?.() + if (authenticated) return + } } catch (error) { if (attempt === CHILD_HEALTH_ATTEMPTS - 1) { console.warn(`[MCP] Health check failed at ${healthURL}`, error) @@ -231,7 +242,14 @@ export function automationPlugin( if (child === spawned) child = null }) - await waitForAutomationHealth(options.browserURL) + await waitForAutomationHealth(options.browserURL, fetch, { + authToken: configuration.authenticationEnabled ? authToken : null, + assertRunning() { + if (child !== spawned || spawned.exitCode !== null || spawned.signalCode !== null) { + throw new Error('MCP child exited before becoming ready') + } + } + }) } async function restartChild(nextConfiguration: DevMCPConfiguration): Promise { diff --git a/tests/engine/app/automation/mcp-vite-plugin.test.ts b/tests/engine/app/automation/mcp-vite-plugin.test.ts index 1c16718a1..a16a63739 100644 --- a/tests/engine/app/automation/mcp-vite-plugin.test.ts +++ b/tests/engine/app/automation/mcp-vite-plugin.test.ts @@ -100,6 +100,39 @@ describe('MCP Vite development server', () => { ]) }) + test('does not accept an unrelated healthy endpoint during authenticated startup', async () => { + let requests = 0 + await waitForAutomationHealth( + 'ws://localhost:7682', + async (_input, init) => { + expect(new Headers(init?.headers).get('authorization')).toBe('Bearer test-owned-token') + requests++ + return Response.json(requests === 1 ? { status: 'ok' } : { status: 'no_app', tools: [] }) + }, + { authToken: 'test-owned-token' } + ) + expect(requests).toBe(2) + }) + + test('rejects a failed child before probing another endpoint', async () => { + let requested = false + await expect( + waitForAutomationHealth( + 'ws://localhost:7682', + async () => { + requested = true + return new Response(null) + }, + { + assertRunning() { + throw new Error('child exited') + } + } + ) + ).rejects.toThrow('child exited') + expect(requested).toBe(false) + }) + test('classifies malformed and oversized configuration requests', async () => { const malformed = Readable.from(['{']) const malformedError = await readDevMCPConfiguration(malformed as never).catch( diff --git a/tests/engine/app/automation/portless-route.test.ts b/tests/engine/app/automation/portless-route.test.ts index 0b0030444..6c275e193 100644 --- a/tests/engine/app/automation/portless-route.test.ts +++ b/tests/engine/app/automation/portless-route.test.ts @@ -12,6 +12,15 @@ describe('Portless MCP routing', () => { }) }) + test('uses the requested app origin and an isolated fixed-port runtime', () => { + expect(devAutomationRoute(undefined, 7682, 'http://localhost:1482')).toEqual({ + browserURL: 'ws://127.0.0.1:7682', + corsOrigin: 'http://localhost:1482', + portlessServiceName: null, + runtimeId: 'localhost-7682' + }) + }) + test('derives a sibling MCP service for the main checkout', () => { expect(devAutomationRoute('https://open-pencil.localhost', 7600)).toEqual({ browserURL: 'wss://mcp.open-pencil.localhost', diff --git a/vite.config.ts b/vite.config.ts index 2674294fa..9047edac9 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -8,17 +8,19 @@ import Components from 'unplugin-vue-components/vite' import { defineConfig } from 'vite' import packageJson from './package.json' -import { AUTOMATION_HTTP_PORT } from './packages/core/src/constants' -import { devAutomationRoute } from './src/app/automation/bridge/portless-route' import { createOpenPencilAliases } from './vite/aliases' -import { localAutomationToken, openPencilAutomationPlugin } from './vite/automation' +import { + localAutomationRoute, + localAutomationToken, + openPencilAutomationPlugin +} from './vite/automation' import { copyCanvasKitAssetsPlugin } from './vite/canvaskit-assets' import { openPencilPwaPlugin } from './vite/pwa' import { rawMarkdownPlugin } from './vite/raw-markdown' import { createDevServerOptions } from './vite/server' const host = process.env.TAURI_DEV_HOST -const automationRoute = devAutomationRoute(process.env.PORTLESS_URL, AUTOMATION_HTTP_PORT) +const automationRoute = localAutomationRoute(host) export default defineConfig(async ({ command }) => ({ resolve: { diff --git a/vite/automation.ts b/vite/automation.ts index 880a4bc4e..51961acad 100644 --- a/vite/automation.ts +++ b/vite/automation.ts @@ -16,11 +16,19 @@ export function automationCORSOrigin(host: string | undefined): string { return host ? `http://${host}:1420` : 'http://localhost:1420' } -export function openPencilAutomationPlugin(command: string, host: string | undefined) { - const route = devAutomationRoute(process.env.PORTLESS_URL, AUTOMATION_HTTP_PORT) - return automationPlugin(localAutomationToken(command), { - ...route, - corsOrigin: process.env.PORTLESS_URL ? route.corsOrigin : automationCORSOrigin(host), - httpPort: AUTOMATION_HTTP_PORT - }) +export function localAutomationRoute(host: string | undefined) { + const port = Number(process.env.OPENPENCIL_DEV_MCP_PORT ?? AUTOMATION_HTTP_PORT) + if (!Number.isInteger(port) || port < 1024 || port > 65535) { + throw new Error('OPENPENCIL_DEV_MCP_PORT must be an integer between 1024 and 65535') + } + const origin = process.env.OPENPENCIL_DEV_ORIGIN ?? automationCORSOrigin(host) + const url = new URL(origin) + if (!['http:', 'https:'].includes(url.protocol) || url.origin !== origin) { + throw new Error('OPENPENCIL_DEV_ORIGIN must be an HTTP(S) origin') + } + return { ...devAutomationRoute(process.env.PORTLESS_URL, port, origin), httpPort: port } +} + +export function openPencilAutomationPlugin(command: string, host: string | undefined) { + return automationPlugin(localAutomationToken(command), localAutomationRoute(host)) }