diff --git a/TESTING.md b/TESTING.md index 369283f..5948a2b 100644 --- a/TESTING.md +++ b/TESTING.md @@ -40,8 +40,9 @@ npm run --prefix tools/vendor test:workflows -- --project firefox --project webk ``` `PM_TEST_PYTHON` selects the Python executable; set it when Python is outside -PATH. Every Playwright worker starts its own `tests/browser/server.py` on a port the -OS picks, with its own temp root and database (`tests/browser/fixtures.mjs`), so +PATH. Every Playwright worker starts its own `tests/browser/server.py --port 0`, +which binds a port the OS picks and prints it as `PM_TEST_SERVER_PORT=`, with +its own temp root and database (`tests/browser/fixtures.mjs`), so parallel workers and concurrent runs never share state. `PM_TEST_WORKERS` sets the worker count (default 1 under `CI`, otherwise a quarter of the CPUs, at most 8). Test servers are never reused across runs. On unsupported Linux diff --git a/tests/browser/fixtures.mjs b/tests/browser/fixtures.mjs index 084d41b..c1173f5 100644 --- a/tests/browser/fixtures.mjs +++ b/tests/browser/fixtures.mjs @@ -1,17 +1,16 @@ // Shared `test` for the page workflows: every Playwright worker owns one -// tests/browser/server.py process on an OS-chosen port, with its own temp root +// tests/browser/server.py process on a port the OS chose for it, with its own temp root // and database. `baseURL` (and so `page`, `request` and `browser.newContext`) // points at that worker's server. A server that dies fails the running test // with its stderr tail, and the next test in the worker gets a fresh server. import { test as base, expect } from '../../tools/vendor/node_modules/@playwright/test/index.mjs'; -import { freePort } from '../../tools/vendor/free-port.mjs'; import { createServerSupervisor, startBrowserServer } from '../../tools/vendor/start-browser-server.mjs'; const SERVER_FIXTURE_TIMEOUT_MS = 90_000; export const test = base.extend({ testServer: [async ({}, use) => { - const supervisor = createServerSupervisor({ start: async () => startBrowserServer({ port: await freePort() }) }); + const supervisor = createServerSupervisor({ start: () => startBrowserServer() }); try { await use(supervisor); } finally { diff --git a/tests/browser/server.py b/tests/browser/server.py index 5e2888d..599d92e 100644 --- a/tests/browser/server.py +++ b/tests/browser/server.py @@ -4,6 +4,7 @@ import argparse import json import os import shutil +import socket from pathlib import Path import sys import tempfile @@ -515,14 +516,41 @@ def create_app(root: Path) -> web.Application: return app -def main() -> None: +HOST = "127.0.0.1" +PORT_ANNOUNCEMENT = "PM_TEST_SERVER_PORT=" + + +def bind_listener(port: int) -> socket.socket: + """Bind the server's socket before the app is built. + + ``port`` 0 lets the OS choose a free port, which this process then holds, + so parallel launchers never race for one between choosing and binding. + """ + listener = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + try: + if port and os.name != "nt": + # aiohttp's own default on POSIX; on Windows it would let a + # second process share the port. + listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + listener.bind((HOST, port)) + listener.listen(128) + except OSError: + listener.close() + raise + return listener + + +def main(argv: list[str] | None = None) -> None: parser = argparse.ArgumentParser() - parser.add_argument("--port", type=int, default=8765) - args = parser.parse_args() + parser.add_argument( + "--port", type=int, default=8765, help="0 binds an OS-chosen free port" + ) + args = parser.parse_args(argv) + listener = bind_listener(args.port) + # One machine-readable line tells the launcher which port this process holds. + print(f"{PORT_ANNOUNCEMENT}{listener.getsockname()[1]}", flush=True) with tempfile.TemporaryDirectory(prefix="pm-browser-") as directory: - web.run_app( - create_app(Path(directory).resolve()), host="127.0.0.1", port=args.port - ) + web.run_app(create_app(Path(directory).resolve()), sock=listener) if __name__ == "__main__": diff --git a/tests/browser/worker-server.spec.mjs b/tests/browser/worker-server.spec.mjs index 2ed4136..65d2b8d 100644 --- a/tests/browser/worker-server.spec.mjs +++ b/tests/browser/worker-server.spec.mjs @@ -2,7 +2,6 @@ // `browser.newContext()` all reach it through `baseURL`, and another server // process never sees its database. import { test, expect } from './fixtures.mjs'; -import { freePort } from '../../tools/vendor/free-port.mjs'; import { startBrowserServer } from '../../tools/vendor/start-browser-server.mjs'; const search = text => `/prompt_manager/search?text=${encodeURIComponent(text)}`; @@ -26,7 +25,7 @@ test('every fixture reaches this worker\'s own server and another server keeps i await context.close(); } - const other = await startBrowserServer({ port: await freePort() }); + const other = await startBrowserServer(); try { expect(other.url).not.toBe(baseURL); expect((await (await fetch(`${other.url}${search(marker)}`)).json()).count).toBe(0); diff --git a/tests/vendor/browser-server-fixture.test.mjs b/tests/vendor/browser-server-fixture.test.mjs index ee42e69..3c4076f 100644 --- a/tests/vendor/browser-server-fixture.test.mjs +++ b/tests/vendor/browser-server-fixture.test.mjs @@ -2,12 +2,12 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { createServer } from 'node:net'; import { fileURLToPath } from 'node:url'; -import { freePort } from '../../tools/vendor/free-port.mjs'; import { startBrowserServer, createServerSupervisor } from '../../tools/vendor/start-browser-server.mjs'; import { workflowWorkers } from '../../tools/vendor/test-workers.mjs'; const fakeServer = fileURLToPath(new URL('./fake-browser-server.mjs', import.meta.url)); const fake = (...extra) => ({ command: process.execPath, args: port => [fakeServer, '--port', String(port), ...extra] }); +const REAL_SERVER_TIMEOUT_MS = 90_000; function listen(port = 0) { return new Promise((resolve, reject) => { @@ -17,24 +17,42 @@ function listen(port = 0) { }); } +async function releasedPort() { + const probe = await listen(); + const { port } = probe.address(); + await new Promise(resolve => probe.close(resolve)); + return port; +} + test('free port allocation never reuses a bound port', async t => { const held = await listen(); t.after(() => held.close()); const heldPort = held.address().port; - const seen = []; - for (let index = 0; index < 20; index += 1) { - const port = await freePort(); - assert.ok(Number.isInteger(port) && port > 0 && port < 65536, `port ${port} is a TCP port`); - assert.notEqual(port, heldPort, 'a port another process holds is never handed out'); - const probe = await listen(port); - seen.push(port); - await new Promise(resolve => probe.close(resolve)); + const servers = await Promise.all(Array.from({ length: 6 }, () => startBrowserServer(fake()))); + t.after(() => Promise.all(servers.map(server => server.stop()))); + const ports = servers.map(server => server.port); + for (const server of servers) { + assert.ok(Number.isInteger(server.port) && server.port > 0 && server.port < 65536, `port ${server.port} is a TCP port`); + assert.equal(server.url, `http://127.0.0.1:${server.port}`); + assert.equal(await (await fetch(`${server.url}/prompt_manager/test`)).text(), 'ready'); } - assert.ok(new Set(seen).size > 1, 'the OS hands out more than one port'); + assert.ok(!ports.includes(heldPort), 'a port another process holds is never handed out'); + assert.equal(new Set(ports).size, ports.length, 'concurrent servers never share a port'); }); -test('startBrowserServer serves the readiness page on the given port and stops cleanly', async () => { - const port = await freePort(); +test('startBrowserServer asks the server to bind port 0 and reads the port it announces', async () => { + const asked = []; + const server = await startBrowserServer({ command: process.execPath, args: port => { asked.push(port); return fake().args(port); } }); + try { + assert.deepEqual(asked, [0], 'no port is reserved and released before the server binds'); + assert.ok(server.port > 0); + } finally { + await server.stop(); + } +}); + +test('startBrowserServer serves the readiness page on a requested port and stops cleanly', async () => { + const port = await releasedPort(); const server = await startBrowserServer({ port, ...fake() }); assert.equal(server.port, port); assert.equal(server.url, `http://127.0.0.1:${port}`); @@ -44,22 +62,20 @@ test('startBrowserServer serves the readiness page on the given port and stops c }); test('startBrowserServer reports a startup failure with the server\'s stderr tail', async () => { - const port = await freePort(); - await assert.rejects(startBrowserServer({ port, ...fake('--fail') }), error => { + await assert.rejects(startBrowserServer(fake('--fail')), error => { assert.match(error.message, /exited with code 4 before it was ready/); assert.match(error.message, /last fake stderr line/); return true; }); }); -test('startBrowserServer gives up on a server that never becomes ready', async () => { - const port = await freePort(); +test('startBrowserServer gives up on a server that never announces a port or becomes ready', async () => { const silent = { command: process.execPath, args: () => ['-e', 'setInterval(() => {}, 1000)'] }; - await assert.rejects(startBrowserServer({ port, ...silent, timeoutMs: 500 }), /not ready .* within 500 ms/); + await assert.rejects(startBrowserServer({ ...silent, timeoutMs: 500 }), /not ready .* within 500 ms/); }); test('worker server restarts after an unexpected exit', async t => { - const supervisor = createServerSupervisor({ start: async () => startBrowserServer({ port: await freePort(), ...fake() }) }); + const supervisor = createServerSupervisor({ start: () => startBrowserServer(fake()) }); t.after(() => supervisor.stop()); const first = await supervisor.ensure(); assert.equal(supervisor.crash(first), null, 'a running server reports no crash'); @@ -74,29 +90,41 @@ test('worker server restarts after an unexpected exit', async t => { assert.equal(await supervisor.ensure(), second, 'a healthy server is reused'); }); -test('supervisor retries a port that was taken between reservation and bind', async t => { - const ports = []; +test('supervisor reports a startup failure once and tries again on the next ensure', async t => { let attempts = 0; const supervisor = createServerSupervisor({ start: async () => { attempts += 1; - if (attempts === 1) throw new Error('OSError: [Errno 98] error while attempting to bind: address already in use'); - const port = await freePort(); - ports.push(port); - return startBrowserServer({ port, ...fake() }); + if (attempts === 1) throw new Error('ImportError: no module'); + return startBrowserServer(fake()); }, }); t.after(() => supervisor.stop()); + await assert.rejects(supervisor.ensure(), /ImportError/); + assert.equal(attempts, 1, 'a failed startup is not retried behind the caller\'s back'); const server = await supervisor.ensure(); assert.equal(attempts, 2); - assert.equal(server.port, ports[0]); + assert.equal(await (await fetch(`${server.url}/prompt_manager/test`)).text(), 'ready'); }); -test('supervisor does not retry a startup failure that is not a port collision', async () => { - let attempts = 0; - const supervisor = createServerSupervisor({ start: async () => { attempts += 1; throw new Error('ImportError: no module'); } }); - await assert.rejects(supervisor.ensure(), /ImportError/); - assert.equal(attempts, 1); +test('tests/browser/server.py binds an OS-chosen port for --port 0 and still honours --port N', { timeout: 2 * REAL_SERVER_TIMEOUT_MS }, async t => { + const held = await listen(); + t.after(() => held.close()); + const chosen = await startBrowserServer({ timeoutMs: REAL_SERVER_TIMEOUT_MS }); + try { + assert.notEqual(chosen.port, held.address().port); + assert.equal((await fetch(`${chosen.url}/prompt_manager/test`)).status, 200); + } finally { + await chosen.stop(); + } + const port = await releasedPort(); + const fixed = await startBrowserServer({ port, timeoutMs: REAL_SERVER_TIMEOUT_MS }); + try { + assert.equal(fixed.port, port); + assert.equal((await fetch(`http://127.0.0.1:${port}/prompt_manager/test`)).status, 200); + } finally { + await fixed.stop(); + } }); test('worker count follows PM_TEST_WORKERS, then one in CI, then a quarter of the CPUs up to eight', () => { diff --git a/tests/vendor/fake-browser-server.mjs b/tests/vendor/fake-browser-server.mjs index 1a4f700..0f14322 100644 --- a/tests/vendor/fake-browser-server.mjs +++ b/tests/vendor/fake-browser-server.mjs @@ -1,6 +1,8 @@ -// A stand-in for tests/browser/server.py: `--port N` serves the readiness page, -// `--fail` writes to stderr and exits before listening, and GET /__exit makes -// the process die unexpectedly after writing a last stderr line. +// A stand-in for tests/browser/server.py: `--port N` (0 = an OS-chosen port) +// serves the readiness page and announces the bound port on stdout as +// `PM_TEST_SERVER_PORT=`, `--fail` writes to stderr and exits before +// listening, and GET /__exit makes the process die unexpectedly after writing a +// last stderr line. import { createServer } from 'node:http'; const args = process.argv.slice(2); @@ -17,5 +19,7 @@ const server = createServer((request, response) => { } response.end(request.url === '/prompt_manager/test' ? 'ready' : `pid ${process.pid}`); }); -server.listen(port, '127.0.0.1'); +server.listen(port, '127.0.0.1', () => { + process.stdout.write(`fake server: listening\nPM_TEST_SERVER_PORT=${server.address().port}\n`); +}); for (const signal of ['SIGTERM', 'SIGINT']) process.once(signal, () => server.close(() => process.exit(0))); diff --git a/tools/vendor/free-port.mjs b/tools/vendor/free-port.mjs deleted file mode 100644 index 88a2d34..0000000 --- a/tools/vendor/free-port.mjs +++ /dev/null @@ -1,17 +0,0 @@ -import { createServer } from 'node:net'; - -// Ask the OS for a port nobody holds: bind port 0 on loopback, read it back and -// release it. A port another process has bound is never returned. The caller -// binds it next; the short window in between is covered by retrying a startup -// that fails with "address already in use" (see createServerSupervisor). -export function freePort(host = '127.0.0.1') { - return new Promise((resolve, reject) => { - const server = createServer(); - server.unref(); - server.once('error', reject); - server.listen(0, host, () => { - const { port } = server.address(); - server.close(error => (error ? reject(error) : resolve(port))); - }); - }); -} diff --git a/tools/vendor/start-browser-server.mjs b/tools/vendor/start-browser-server.mjs index 358fd57..b0c7f6b 100644 --- a/tools/vendor/start-browser-server.mjs +++ b/tools/vendor/start-browser-server.mjs @@ -10,8 +10,9 @@ const STOP_TIMEOUT_MS = 5_000; const POLL_INTERVAL_MS = 100; const PROBE_TIMEOUT_MS = 1_000; const STDERR_TAIL_BYTES = 8_192; -const START_ATTEMPTS = 3; -const PORT_COLLISION = /address already in use|errno 98\b|errno 48\b|10048|EADDRINUSE/i; +const HOST = '127.0.0.1'; +// tests/browser/server.py prints this once it holds its listening socket. +const PORT_ANNOUNCEMENT = /^PM_TEST_SERVER_PORT=(\d+)\s*$/m; const pythonServer = port => ['-m', 'tests.browser.server', '--port', String(port)]; @@ -19,28 +20,41 @@ function describeExit({ code, signal }) { return signal ? `signal ${signal}` : `code ${code}`; } -// Start one disposable tests/browser/server.py on `port` and wait until it -// serves the readiness page. Resolves to {port, url, exited, exitStatus, +// Start one disposable tests/browser/server.py and wait until it serves the +// readiness page. `port` 0 (the default) lets the server bind an OS-chosen port +// itself and announce it on stdout, so nothing can take the port between +// choosing and binding. Resolves to {port, url, exited, exitStatus, // stderrTail(), stop()}; rejects with the server's stderr tail when it exits // or stays unready. export async function startBrowserServer({ - port, + port = 0, command = process.env.PM_TEST_PYTHON || 'python', args = pythonServer, cwd = repositoryRoot, timeoutMs = STARTUP_TIMEOUT_MS, } = {}) { - if (!Number.isInteger(port) || port <= 0) throw new Error(`startBrowserServer needs a port, got ${port}`); - const url = `http://127.0.0.1:${port}`; - const child = spawn(command, args(port), { cwd, stdio: ['ignore', 'ignore', 'pipe'] }); + if (!Number.isInteger(port) || port < 0 || port > 65535) { + throw new Error(`startBrowserServer needs a port from 0 to 65535, got ${port}`); + } + const child = spawn(command, args(port), { cwd, stdio: ['ignore', 'pipe', 'pipe'] }); let stderr = ''; child.stderr.setEncoding('utf8'); child.stderr.on('data', chunk => { stderr = (stderr + chunk).slice(-STDERR_TAIL_BYTES); }); + let stdout = ''; + let announced = null; + child.stdout.setEncoding('utf8'); + // Keep draining stdout for the server's whole life so it never blocks on a full pipe. + child.stdout.on('data', chunk => { + if (announced !== null) return; + stdout = (stdout + chunk).slice(-STDERR_TAIL_BYTES); + const match = PORT_ANNOUNCEMENT.exec(stdout); + if (match) announced = Number(match[1]); + }); const killOnExit = () => child.kill('SIGKILL'); process.once('exit', killOnExit); const server = { - port, - url, + port: port || null, + url: port ? `http://${HOST}:${port}` : null, exitStatus: null, stopping: false, stderrTail: () => stderr.trimEnd(), @@ -72,15 +86,27 @@ export async function startBrowserServer({ const failure = async reason => { await server.stop(); const tail = server.stderrTail(); - return new Error(`test server on port ${port} ${reason}${tail ? `\n--- server stderr tail ---\n${tail}` : ''}`); + return new Error(`test server on port ${server.port ?? '(not yet announced)'} ${reason}` + + (tail ? `\n--- server stderr tail ---\n${tail}` : '')); }; const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { if (server.exitStatus !== null) { throw await failure(`exited with ${describeExit(server.exitStatus)} before it was ready`); } + if (announced !== null && server.url === null) { + server.port = announced; + server.url = `http://${HOST}:${announced}`; + } + if (announced !== null && announced !== server.port) { + throw await failure(`announced port ${announced} instead`); + } + if (server.url === null) { + await delay(POLL_INTERVAL_MS); + continue; + } try { - const response = await fetch(`${url}${READY_PATH}`, { signal: AbortSignal.timeout(PROBE_TIMEOUT_MS) }); + const response = await fetch(`${server.url}${READY_PATH}`, { signal: AbortSignal.timeout(PROBE_TIMEOUT_MS) }); await response.arrayBuffer(); if (response.ok) return server; } catch { @@ -92,23 +118,17 @@ export async function startBrowserServer({ } // Keep one live server for a Playwright worker. ensure() returns the running -// server, or starts a fresh one when the last one died; crash(server) turns an -// unexpected exit into an error carrying the stderr tail; a startup that lost -// its port to another process between reservation and bind is retried. -export function createServerSupervisor({ start, attempts = START_ATTEMPTS }) { +// server, or starts a fresh one when the last one died (a failed start is +// reported, not retried); crash(server) turns an unexpected exit into an error +// carrying the stderr tail. +export function createServerSupervisor({ start }) { let current = null; return { async ensure() { if (current && current.exitStatus === null) return current; current = null; - for (let attempt = 1; ; attempt += 1) { - try { - current = await start(); - return current; - } catch (error) { - if (attempt >= attempts || !PORT_COLLISION.test(error.message)) throw error; - } - } + current = await start(); + return current; }, crash(server) { if (!server || server.stopping || server.exitStatus === null) return null;