Add a Playwright end-to-end harness with a containerised gateway - #91
Add a Playwright end-to-end harness with a containerised gateway#91bburda wants to merge 7 commits into
Conversation
Runs the suite against a real gateway in a container: manifest-defined scripts, uploads enabled, and a named volume for uploaded files so the checkout stays clean. Seven scenarios drive the live gateway, six more use route mocking for states a healthy gateway will not produce on demand, and one smoke test proves the stack itself. The suite runs on a single worker because the mocked project still reaches the gateway for entity discovery.
Adds the end-to-end job alongside the existing checks, dumping the gateway's container logs when a run fails so a container that dies during startup can be diagnosed from the run alone. Documents the Scripts tab and how to run the suite locally.
There was a problem hiding this comment.
Pull request overview
Adds first end-to-end (Playwright) coverage for the ROS2 Medkit Web UI by running the SPA in a real browser against a containerised gateway, and wiring it into CI. This complements the existing Vitest/jsdom suite by validating real network + DOM behaviour, especially for the Scripts feature introduced in #90.
Changes:
- Add a Playwright config, global setup, and a new
e2e/suite (live gateway scenarios + mocked error-state scenarios + smoke test). - Add a pinned Docker Compose gateway stack (manifest-only discovery, scripts enabled, uploads isolated in a named volume).
- Add CI job + docs updates for running E2E locally; ignore Playwright artifacts in git.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds project reference for the new e2e/ TypeScript project. |
| README.md | Documents Scripts tab behaviour and how to run the E2E suite locally. |
| playwright.config.ts | Defines Playwright projects, serialisation strategy (1 worker), web server config, and storage state. |
| e2e/tsconfig.json | Adds TS config for Playwright specs/global setup. |
| e2e/smoke.spec.ts | Smoke test ensuring the stack boots and the entity tree renders. |
| e2e/scripts.spec.ts | Live-gateway Scripts scenarios (list/run/fail/stop/upload/write/delete/remove execution). |
| e2e/scripts-errors.spec.ts | Mocked gateway responses for error/edge UI states that are hard to induce live. |
| e2e/global-setup.ts | Waits for gateway health and seeds persisted server URL into storage state. |
| e2e/gateway/scripts/sleep.sh | Gateway fixture script for stop/terminate scenario. |
| e2e/gateway/scripts/hello.sh | Gateway fixture script for stdin-parameter echo scenario. |
| e2e/gateway/scripts/fail.sh | Gateway fixture script for non-zero exit / stderr behaviour. |
| e2e/gateway/params.yaml | Gateway params enabling bind-all + CORS + manifest-only discovery + scripts uploads. |
| e2e/gateway/manifest.yaml | Minimal manifest defining entities and managed scripts for E2E. |
| e2e/fixtures/uploaded-script.sh | Fixture uploaded script used by E2E to validate upload/run/delete path. |
| e2e/docker-compose.yml | Compose stack for pinned gateway image + init service + named volume for uploads. |
| CONTRIBUTING.md | Adds instructions and warnings for running the E2E suite locally. |
| .gitignore | Ignores Playwright output directories and E2E auth storage state. |
| .github/workflows/ci.yml | Adds an e2e job running Compose + Playwright and uploading artifacts on failure. |
| async function waitForGateway(): Promise<void> { | ||
| const deadline = Date.now() + 120_000; | ||
| let lastError = 'no attempt made'; | ||
| while (Date.now() < deadline) { | ||
| try { | ||
| const res = await fetch(`${GATEWAY_URL}/health`); | ||
| if (res.ok) return; | ||
| lastError = `HTTP ${res.status}`; | ||
| } catch (err) { | ||
| lastError = err instanceof Error ? err.message : String(err); | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
| } | ||
| throw new Error(`Gateway did not become healthy at ${GATEWAY_URL}: ${lastError}`); | ||
| } |
| const BASE_URL = 'http://localhost:5173'; | ||
|
|
||
| export default defineConfig({ | ||
| testDir: './e2e', | ||
| globalSetup: './e2e/global-setup.ts', | ||
| timeout: 60_000, | ||
| expect: { timeout: 30_000 }, | ||
| reporter: [['html', { open: 'never' }], ['list']], | ||
| // Both projects below hit the same single containerised gateway for | ||
| // everything they do not explicitly mock (entity discovery, health, | ||
| // faults). Left to Playwright's default per-project parallelism, the | ||
| // 'mocked' project's own worker runs concurrently with 'scripts-serial' | ||
| // and the resulting burst of simultaneous full-app connections can push | ||
| // the gateway's response time past the client's health-check timeout, | ||
| // aborting an unrelated connect() and failing an unrelated test. A single | ||
| // global worker keeps every test's gateway traffic strictly sequential. | ||
| workers: 1, | ||
| use: { baseURL: BASE_URL, storageState: 'e2e/.auth/state.json', trace: 'retain-on-failure' }, | ||
| webServer: { | ||
| command: 'npm run dev -- --port 5173 --strictPort', | ||
| url: BASE_URL, | ||
| reuseExistingServer: !process.env.CI, | ||
| timeout: 120_000, | ||
| }, |
| const card = page.locator('[data-slot="card"]', { has: status }).last(); | ||
| await expect(page.getByText('The gateway no longer tracks this execution')).toBeVisible({ timeout: 30_000 }); | ||
| await expect(card.getByRole('button', { name: 'Remove' })).toBeVisible({ timeout: 30_000 }); |
| test.afterEach(async ({ page }, testInfo) => { | ||
| // Uploads persist in the gateway's volume between runs, so any script left | ||
| // over from this test (including a previous, unfinished run of it) must be | ||
| // removed - otherwise the next run would find a duplicate row and a | ||
| // getByRole('button', { name: uploadedName }) lookup would no longer be unique. | ||
| await openScripts(page, 'Test ECU'); | ||
| const names = [uploadedNameFor(testInfo), writtenNameFor(testInfo, 'bash'), writtenNameFor(testInfo, 'python')]; | ||
| for (const name of names) { | ||
| const row = page.getByRole('button', { name }); | ||
| if (await row.isVisible().catch(() => false)) { | ||
| await row.click(); | ||
| await page.getByRole('button', { name: 'Delete' }).click(); | ||
| await expect(row).toBeHidden({ timeout: 30_000 }); | ||
| } | ||
| } | ||
| }); |
| image: ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 | ||
| user: root | ||
| volumes: | ||
| - e2e-uploads:/e2e-uploads | ||
| entrypoint: ['chown', '-R', '999:999', '/e2e-uploads'] | ||
| gateway: | ||
| # Pinned on purpose: :latest is overwritten on every push to the gateway | ||
| # main branch, which would let unrelated changes turn this repo CI red. | ||
| image: ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 |
…by digest - Give each gateway health-check attempt in global setup its own timeout via AbortSignal.timeout, since a stalled connect (as opposed to a refused one) would otherwise hang past the overall deadline with no informative error. - Read E2E_APP_URL in playwright.config.ts and derive the dev server's port from it, matching global setup, so the two cannot end up pointed at different addresses. - Scope the "no longer tracks this execution" assertion in the polling-404 scenario to the card that owns the status badge, matching every other assertion in that test. - Make the scripts spec's afterEach cleanup best-effort: a failure opening the panel or removing one leftover script no longer masks the test's own failure or stops the rest of the cleanup from being attempted. - Pin the e2e gateway image by immutable digest instead of a mutable tag, so a later re-publish of the same tag cannot change what CI pulls, keeping the human-readable tag in a comment for reference.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
e2e/scripts.spec.ts:171
- This scenario is described as writing a bash script, but the uploaded file name ends in
.sh. The gateway’s interpreter selection (documented in the UI) uses.bashfor bash and treats other extensions (including.sh) as/bin/sh, so this test won’t actually exercise the bash interpreter path.
await page.getByRole('button', { name: 'Upload' }).click();
const dialog = page.getByRole('dialog');
await dialog.getByRole('button', { name: 'Write script' }).click();
await dialog.getByLabel('File name').fill(`${scriptName}.sh`);
// Reachable by role and accessible name against the real CodeMirror
README.md:91
- Local E2E instructions omit installing Playwright browsers. On a fresh machine/checkout,
npm run test:e2ewill fail unless the Chromium browser bundle has been installed (CI does this vianpx playwright install ...).
# Run the end-to-end suite against a containerised gateway
docker compose -f e2e/docker-compose.yml up -d
npm run test:e2e
CONTRIBUTING.md:106
- The local E2E runbook doesn’t mention installing Playwright browsers. On a fresh environment,
npm run test:e2ewill fail unless Chromium has been installed at least once (CI handles this explicitly).
2. Run the suite:
```bash
npm run test:e2e
```
| test('writes a bash script in the UI, runs it and shows its output', async ({ page }, testInfo) => { | ||
| const scriptName = writtenNameFor(testInfo, 'bash'); | ||
| await openScripts(page, 'Test ECU'); | ||
| await page.getByRole('button', { name: 'Upload' }).click(); |
There was a problem hiding this comment.
major - getByRole('button', { name: 'Upload' }) also matches uploaded script rows.
getByRole name matching is substring and case-insensitive by default. Script rows carry aria-label={script.name}, and this file names its own fixtures uploaded_<worker>_<repeat>, which contains "upload". So this toolbar lookup collides with any uploaded_* row in the panel. Same at line 205.
Reproduced with leftover uploads in the volume:
strict mode violation: getByRole('button', { name: 'Upload' }) resolved to 4 elements:
1) <button ...> aka getByRole('button', { name: 'Upload', exact: true })
2) <button aria-label="uploaded_0_0" ...>
3) <button aria-label="uploaded_0_0" ...>
4) <button aria-label="uploaded_0_0" ...>
Fix: { name: 'Upload', exact: true }, or scope the lookup to the panel toolbar.
There was a problem hiding this comment.
Fixed in 2a6be37. Both lookups are now anchored to the toolbar button exactly, so a leftover uploaded_* row cannot satisfy them.
| const names = [uploadedNameFor(testInfo), writtenNameFor(testInfo, 'bash'), writtenNameFor(testInfo, 'python')]; | ||
| for (const name of names) { | ||
| try { | ||
| const row = page.getByRole('button', { name }); |
There was a problem hiding this comment.
major - this cleanup cannot recover once two leftovers share a name.
uploadedNameFor/writtenNameFor derive from workerIndex and repeatEachIndex, both 0 on every run, so the name is identical across runs. The gateway accepts duplicate names and returns two separate ids (verified: two POSTs of uploaded_0_0 produced script_..._3 and script_..._4, both listed). One leftover self-heals here. Two do not: row.isVisible() on the next line throws a strict mode violation, the catch turns it into a console.warn, and nothing gets deleted. Reachable by interrupting a run mid-upload and re-running that single test with --grep, or by two overlapping local runs.
Reproduced: with two leftovers the run gave 3 failed / 7 passed, and this hook left a third leftover behind. The gateway then stays wedged until someone runs docker compose down -v.
Fix: loop on row.first() while await row.count() > 0, so N leftovers are removed rather than only a unique one.
There was a problem hiding this comment.
Fixed in 2a6be37. The cleanup now loops while the locator matches anything and deletes .first() each time, so N leftovers are removed rather than only a unique one. Verified by running the suite twice back to back against the same volume without destroying it in between.
| # same commit would move one. ghcr.io/selfpatch/ros2_medkit-jazzy:sha-7939c94 | ||
| image: ghcr.io/selfpatch/ros2_medkit-jazzy@sha256:565db07e1e972b31684bf864fbaad7e8a70aacabf2ef0cd4510fdbd8e3281831 | ||
| ports: | ||
| - '${E2E_GATEWAY_PORT:-8080}:8080' |
There was a problem hiding this comment.
major - this publishes the gateway on 0.0.0.0 (verified: 0.0.0.0:8080 and [::]:8080).
The gateway runs with allow_uploads: true and executes uploaded shell scripts, unauthenticated, and CONTRIBUTING tells developers to up -d and leave it running. On any shared or untrusted network that is remote code execution on the developer's machine for as long as the container is up.
Fix: '127.0.0.1:${E2E_GATEWAY_PORT:-8080}:8080'.
There was a problem hiding this comment.
Fixed in 2a6be37, bound to 127.0.0.1 with the port override preserved. Thank you for this one - the combination you describe is exactly right, and the comment above the mapping now spells out why the prefix must not be dropped, since the reason is not visible from the line itself.
| port: 8080 | ||
| cors: | ||
| allowed_origins: | ||
| - 'http://localhost:5173' |
There was a problem hiding this comment.
minor - hardcoded origin defeats the E2E_APP_URL override.
playwright.config.ts derives the dev server port from E2E_APP_URL, but the container only allows http://localhost:5173. Verified: Origin: http://localhost:5174 gets no Access-Control-Allow-Origin back. Overriding E2E_APP_URL to dodge a busy 5173 makes every gateway call fail in the browser, surfacing as an unexplained connect timeout rather than a CORS error anyone will read.
Fix: template the origin from the same variable, or allow * in this fixture.
There was a problem hiding this comment.
Fixed in 2a6be37. The fixture no longer hardcodes the origin, so overriding the app URL no longer produces a silent CORS failure that reads as a connection timeout.
| import { chromium } from '@playwright/test'; | ||
| import { mkdirSync } from 'node:fs'; | ||
|
|
||
| const GATEWAY_URL = process.env.E2E_GATEWAY_URL ?? 'http://localhost:8080/api/v1'; |
There was a problem hiding this comment.
minor - E2E_GATEWAY_PORT and E2E_GATEWAY_URL are independent knobs.
The compose file takes E2E_GATEWAY_PORT, this takes E2E_GATEWAY_URL. Setting only the first, which is the natural move when 8080 is already taken, leaves setup polling 8080 for the full 120 s before failing. Derive the URL from the port, or state that both have to be set together.
There was a problem hiding this comment.
Fixed in 2a6be37. The gateway URL is now derived from the port variable, so setting the one knob that is natural to reach for is enough.
| const BASE_URL = process.env.E2E_APP_URL ?? 'http://localhost:5173'; | ||
| const APP_PORT = new URL(BASE_URL).port || '5173'; | ||
|
|
||
| export default defineConfig({ |
There was a problem hiding this comment.
minor - no forbidOnly in CI.
Verified with CI=1 and a committed test.only: Running 1 test using 1 worker, 1 passed, exit 0. One stray .only shrinks the suite to a single test and CI stays green.
Fix: forbidOnly: !!process.env.CI.
There was a problem hiding this comment.
Fixed in 2a6be37: forbidOnly is set from the CI environment variable.
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| sleep 300 |
There was a problem hiding this comment.
minor - 300 s sleeper against a global concurrency cap of 5.
The cap is per gateway, not per script, and it is 5. Verified: the 6th concurrent sleeper returned x-medkit-concurrency-limit, and a hello run right after was rejected too. An interrupted run leaves this sleeper alive for the whole timeout_sec: 300 in manifest.yaml, and the harness has no reset path (DELETE on a running execution returns 409 "Stop it first"). Five interruptions inside that window block every execution the suite tries to start.
Fix: drop the sleep and timeout_sec to about 30 s. Still ample for the stop test, and leaks clear quickly.
There was a problem hiding this comment.
Fixed in 2a6be37. The sleeper and the manifest timeout are both down to thirty seconds, which still leaves ample room for the scenario that stops it mid-run while making an interrupted run clear on its own well before the concurrency cap becomes a problem.
| - name: Build project | ||
| run: npm run build | ||
|
|
||
| e2e: |
There was a problem hiding this comment.
minor - this job cannot run on this PR.
The workflow triggers on pull_request: branches: [main] only, and this branch targets feature/scripts-tab. Neither #91 nor #90 gets any signal from it. First real execution is after the stack lands on main, which is also the first chance to learn whether the 4 GB image pull plus a ~2 min run fits the 20 minute job timeout on a GitHub runner.
There was a problem hiding this comment.
Fixed in 942c22d, kept as its own commit so it can be reverted independently: the trigger no longer filters by target branch, so a stacked pull request gets a signal. Flagging it explicitly since it changes behaviour for every future pull request in the repository and is your call to keep. This run is the first real execution of the job, which also answers the open question about the image pull fitting the timeout.
The e2e gateway container ran with uploads enabled and executes uploaded shell scripts without authentication, yet published its port on every interface - on a shared or untrusted network that is remote code execution for as long as the container is left running. Bind it to 127.0.0.1 instead, keeping the port override working. Several other rough edges in the harness made it unreliable or misleading: - The toolbar "Upload" button lookup matched by substring, so it also resolved to leftover rows named uploaded_<worker>_<repeat> (which contains "upload"). Match it by exact name instead. - afterEach cleanup checked isVisible() once per leftover name; once two runs left duplicates behind, that check itself threw and nothing got deleted, compounding the problem. Loop on the locator's count and delete .first() until none remain. - The gateway's CORS config only allowed the default dev server origin, so overriding E2E_APP_URL to dodge a busy port failed every request with no CORS error to explain why. Allow any origin - this is a throwaway local/CI fixture with allow_credentials left at its default false, so a wildcard carries none of the risk it would in production. - global-setup only read E2E_GATEWAY_URL, while docker-compose reads E2E_GATEWAY_PORT, so overriding just the port (the natural move) left setup polling the wrong address for its full deadline. Derive the URL from the port when the URL itself is not set. - playwright.config.ts had no forbidOnly, so a committed test.only would pass CI quietly instead of failing it. - The sleeper script and its manifest timeout ran for 300s against a global concurrency cap of 5, so a handful of interrupted runs could exhaust every execution slot with no reset short of destroying the stack. Both are now 30s, still ample for the scenario that stops it mid-run. - The "writes a bash script" scenario uploaded a .sh file, but the gateway only runs .bash under bash - .sh runs under sh like everything else - so it never covered the branch its name claims to. Give it a .bash extension. Documented the E2E_GATEWAY_PORT/E2E_APP_URL override in CONTRIBUTING.
pull_request previously triggered only for PRs targeting main, so a PR targeting any other branch got no CI signal at all. Drop the branch filter on pull_request (push stays scoped to main) so every pull request runs the check, e2e and docker-build jobs.
ScriptRow's handleDelete now calls window.confirm before deleting a script. Playwright auto-dismisses a native dialog with no handler, which returns false and turns every delete into a silent no-op, so every spec that clicks Delete needs to accept the dialog explicitly. Add clickDeleteAndConfirm, a shared helper that registers the dialog listener before the click (accepting after the click would deadlock, since window.confirm blocks the page's JS until the dialog is resolved) and asserts the dialog actually appeared with the expected message, so a removed confirmation guard would fail the test rather than pass silently. Use it everywhere a spec deletes a script: the upload/run/delete scenario, both write-script scenarios, the managed-script-rejection scenario, and the shared afterEach cleanup that removes leftovers between runs.
Pull Request
Summary
Adds the repository's first end-to-end coverage: Playwright driving a real browser against a gateway running in a container, plus the CI job that runs it.
Builds on #90 and is based on its branch, so the diff here is only the harness and the specs. It stays red until #90 merges, because the scenarios exercise the Scripts tab that pull request introduces.
What the harness sets up:
loadrather than for the network to fall idle, because the app holds a fault stream open and the network never falls idle.What the specs cover: seven scenarios against the live gateway, six against mocked responses for states a healthy gateway will not produce on demand, and the smoke test. The live ones assert gateway behaviour rather than only rendering: that a script receives its parameters on stdin, that a failing one surfaces its exit code and stderr with stdout discarded, that a stopped one is reported as stopped rather than failed, and that a script written in the browser in bash and in python runs and returns its output.
Two things worth knowing:
latestis overwritten on every push to the gateway's main branch, which would let unrelated changes turn this repository's CI red. Bumping it is meant to be a deliberate act.Issue
Type
Testing
16 Playwright tests pass against the containerised gateway. The live specs were also run twice in a row against the same container without restarting it, to prove that uploads are cleaned up and the suite is idempotent.
The CI job dumps the gateway's container logs when a run fails, before tearing the stack down, so a container that dies during startup can be diagnosed from the run alone. That failure mode is otherwise invisible on an ephemeral runner.
Checklist
npm run lint)npm run build)