From 086afbb5dca98a7b8e82a3a2a8f1e6e1b44c65a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C5=ABdolfs=20O=C5=A1i=C5=86=C5=A1?= Date: Wed, 30 Sep 2026 16:38:01 +0200 Subject: [PATCH] Give each e2e test its own reliable peer The page always talked to the shared palm backend, so tests couldn't set up peers of their own. Peers now have no passphrase by default, so tests don't need an ssh-agent and test keys stay out of the developer's agent. Fixtures wait for replication instead of relying on gossip timing, and a request cancelled by a reload no longer counts as a page error. --- .github/workflows/e2e.yaml | 3 +- tests/e2e/auth.spec.ts | 39 +++++++++++++++++++ tests/e2e/navigation.spec.ts | 4 +- tests/e2e/nodeStatus.spec.ts | 9 +++++ tests/e2e/theme.spec.ts | 4 +- tests/support/fixtures.ts | 75 +++++++++++++++++++++++++++++++++++- tests/support/globalSetup.ts | 4 ++ tests/support/peerManager.ts | 45 +++++++++++++++------- tests/support/router.ts | 4 +- 9 files changed, 165 insertions(+), 22 deletions(-) create mode 100644 tests/e2e/auth.spec.ts create mode 100644 tests/e2e/nodeStatus.spec.ts diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 876f12c76..b4e015a3c 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -28,7 +28,8 @@ jobs: libayatana-appindicator3-dev \ librsvg2-dev \ libwebkit2gtk-4.1-dev \ - libjavascriptcoregtk-4.1-dev + libjavascriptcoregtk-4.1-dev \ + openssh-client - run: npm ci - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 diff --git a/tests/e2e/auth.spec.ts b/tests/e2e/auth.spec.ts new file mode 100644 index 000000000..0a38e6f1f --- /dev/null +++ b/tests/e2e/auth.spec.ts @@ -0,0 +1,39 @@ +import { + expect, + gitOptions, + test, + useBackend, +} from "@tests/support/fixtures.js"; + +test("unlocks encrypted keys through the ssh-agent", async ({ + page, + peerManager, + sshAuthSock, +}) => { + const peer = await peerManager.createPeer({ + name: "locked", + gitOptions: gitOptions["bob"], + passphrase: "asdf", + sshAuthSock, + }); + await peer.logOut(); + await peer.startHttpd(); + await useBackend(page, peer); + + await page.goto("/"); + await expect(page.getByText("Unlock keys")).toBeVisible(); + + await page + .getByPlaceholder("Enter passphrase to unlock your keys") + .fill("wrong"); + await page.getByRole("button", { name: "Unlock" }).click(); + await expect( + page.getByText("Not able to decrypt keys with provided passphrase."), + ).toBeVisible(); + + await page + .getByPlaceholder("Enter passphrase to unlock your keys") + .fill("asdf"); + await page.getByRole("button", { name: "Unlock" }).click(); + await expect(page.getByText("Unlock keys")).toBeHidden(); +}); diff --git a/tests/e2e/navigation.spec.ts b/tests/e2e/navigation.spec.ts index 51e9ca30d..79ad37508 100644 --- a/tests/e2e/navigation.spec.ts +++ b/tests/e2e/navigation.spec.ts @@ -1,6 +1,6 @@ import type { Page } from "@playwright/test"; -import { expect, test } from "@tests/support/fixtures.js"; +import { expect, reload, test } from "@tests/support/fixtures.js"; function backButton(page: Page) { return page.getByRole("button", { name: "Back", exact: true }); @@ -88,7 +88,7 @@ test("history position survives a reload", async ({ page }) => { await openRepo(page); await page.waitForLoadState("networkidle"); - await page.reload(); + await reload(page); await expectRepo(page); await expect(backButton(page)).toBeEnabled(); diff --git a/tests/e2e/nodeStatus.spec.ts b/tests/e2e/nodeStatus.spec.ts new file mode 100644 index 000000000..21e207f78 --- /dev/null +++ b/tests/e2e/nodeStatus.spec.ts @@ -0,0 +1,9 @@ +import { expect, test } from "@tests/support/fixtures.js"; + +test("shows whether the node is running", async ({ page, peer }) => { + await page.goto("/"); + await expect(page.getByRole("button", { name: "Online" })).toBeVisible(); + + await peer.stopNode(); + await expect(page.getByRole("button", { name: "Offline" })).toBeVisible(); +}); diff --git a/tests/e2e/theme.spec.ts b/tests/e2e/theme.spec.ts index d50a37dcc..af14e3404 100644 --- a/tests/e2e/theme.spec.ts +++ b/tests/e2e/theme.spec.ts @@ -1,4 +1,4 @@ -import { expect, test } from "@tests/support/fixtures.js"; +import { expect, reload, test } from "@tests/support/fixtures.js"; test("default theme", async ({ page }) => { await page.goto("/repos"); @@ -14,7 +14,7 @@ test("theme persistence", async ({ page }) => { await page.getByRole("button", { name: "Light", exact: true }).click(); await expect(page.locator("html")).toHaveAttribute("data-theme", "light"); - await page.reload(); + await reload(page); await expect(page.locator("html")).toHaveAttribute("data-theme", "light"); }); diff --git a/tests/support/fixtures.ts b/tests/support/fixtures.ts index 9e31ebab4..a94c7011f 100644 --- a/tests/support/fixtures.ts +++ b/tests/support/fixtures.ts @@ -1,7 +1,9 @@ /* eslint-disable @typescript-eslint/naming-convention */ import type { PeerManager, RadiclePeer } from "./peerManager.js"; +import type { Page } from "@playwright/test"; import * as Fs from "node:fs/promises"; +import * as Os from "node:os"; import * as Path from "node:path"; import type * as Stream from "node:stream"; @@ -13,6 +15,7 @@ import { createPeerManager } from "@tests/support/peerManager.js"; import { createRepo } from "@tests/support/repo.js"; import { createOptions, supportDir, tmpDir } from "@tests/support/support.js"; import { execa } from "execa"; +import waitOn from "wait-on"; import { formatOid } from "@app/lib/utils.js"; @@ -26,6 +29,7 @@ export const test = base.extend<{ stateDir: string; peerManager: PeerManager; peer: RadiclePeer; + sshAuthSock: string; outputLog: Stream.Writable; }>({ forAllTests: [ @@ -48,6 +52,12 @@ export const test = base.extend<{ if (!process.env.CONTINUE_ON_ERRORS) { page.on("pageerror", msg => { + if (tearingDown.has(page)) { + return; + } + if (reloading.has(page) && isCancelledFetch(msg)) { + return; + } expect( false, `Test failed because there was a console error in the app: ${msg}`, @@ -96,16 +106,19 @@ export const test = base.extend<{ await logFile.close(); }, - peerManager: async ({ stateDir, outputLog }, use) => { + peerManager: async ({ stateDir, outputLog, page }, use) => { const peerManager = await createPeerManager({ dataDir: Path.resolve(Path.join(stateDir, "peers")), outputLog, }); await use(peerManager); + // The page outlives the peers and keeps polling them, so its requests + // fail once they shut down. The test is already over by then. + tearingDown.add(page); await peerManager.shutdown(); }, - peer: async ({ peerManager }, use) => { + peer: async ({ page, peerManager }, use) => { const peer = await peerManager.createPeer({ name: "httpd", gitOptions: gitOptions["bob"], @@ -113,10 +126,25 @@ export const test = base.extend<{ await peer.startNode(); await peer.startHttpd(); + await useBackend(page, peer); await use(peer); }, + // eslint-disable-next-line no-empty-pattern + sshAuthSock: async ({}, use) => { + const dir = await Fs.mkdtemp(Path.join(Os.tmpdir(), "radicle-ssh-agent-")); + const socket = Path.join(dir, "agent.sock"); + const agent = execa("ssh-agent", ["-D", "-a", socket]); + agent.catch(() => undefined); + await waitOn({ resources: [`socket:${socket}`], timeout: 5000 }); + + await use(socket); + + agent.kill(); + await Fs.rm(dir, { recursive: true, force: true }); + }, + // eslint-disable-next-line no-empty-pattern stateDir: async ({}, use, testInfo) => { const stateDir = testInfo.outputDir; @@ -133,6 +161,36 @@ export const test = base.extend<{ }, }); +const reloading = new WeakSet(); +const tearingDown = new WeakSet(); + +// WebKit reports a fetch that was cancelled because the page went away as an +// access control failure. +function isCancelledFetch(error: Error) { + return /Fetch API cannot load .* due to access control checks/.test( + error.message + error.stack, + ); +} + +export async function reload(page: Page) { + reloading.add(page); + try { + await page.reload(); + // WebKit reports fetches the old page had in flight after the new one + // has loaded, so keep ignoring them until the network settles. + await page.waitForLoadState("networkidle"); + } finally { + reloading.delete(page); + } +} + +// Call before the first `page.goto`. +export async function useBackend(page: Page, peer: RadiclePeer) { + await page.addInitScript(port => { + window.__TEST_HTTP_API_PORT__ = port; + }, peer.httpdBaseUrl.port); +} + function log(text: string, label: string, outputLog: Stream.Writable) { const output = text .split("\n") @@ -390,6 +448,19 @@ export async function createCobsFixture( ["patch", "review", patchThree, "-m", "This looks better", "--accept"], createOptions(repoFolder, 2), ); + // Don't let palm act on the patch before it has eve's review. + await peer.waitForEvent( + event => + event.type === "refsFetched" && + event.rid === rid && + event.remote === eve.nodeId && + event.updated.some(update => + Object.values(update).some(ref => + ref.name.endsWith(`/xyz.radicle.patch/${patchThree}`), + ), + ), + 10_000, + ); await Fs.appendFile( Path.join(repoFolder, "README.md"), "\n\nHad to push a new revision", diff --git a/tests/support/globalSetup.ts b/tests/support/globalSetup.ts index 620a3474c..b7595d74d 100644 --- a/tests/support/globalSetup.ts +++ b/tests/support/globalSetup.ts @@ -51,6 +51,10 @@ export default async function globalSetup(): Promise<() => void> { "./crates/test-http-api/Cargo.toml", ]); + // Keeps test keys out of the developer's ssh-agent. + delete process.env.SSH_AUTH_SOCK; + delete process.env.SSH_AGENT_PID; + const peerManager = await createPeerManager({ dataDir: Path.resolve(tmpDir, "peers"), outputLog: Fs.createWriteStream( diff --git a/tests/support/peerManager.ts b/tests/support/peerManager.ts index 0223cf3ae..043f810f0 100644 --- a/tests/support/peerManager.ts +++ b/tests/support/peerManager.ts @@ -60,20 +60,23 @@ export interface RoutingEntry { rid: string; } -interface PeerManagerParams { - dataPath: string; - radSeed: string; +interface PeerOptions { // Name for easy identification. Used on file system and in logs. name: string; gitOptions?: Record; + // Without one the keys are unencrypted and need no ssh-agent. + passphrase?: string; + sshAuthSock?: string; +} + +interface PeerManagerParams extends PeerOptions { + dataPath: string; + radSeed: string; outputLog: Stream.Writable; } export interface PeerManager { - createPeer(params: { - name: string; - gitOptions?: Record; - }): Promise; + createPeer(params: PeerOptions): Promise; /** * Kill all processes spawned by any of the peers */ @@ -101,8 +104,7 @@ export async function createPeerManager(createParams: { async createPeer(params) { const peer = await RadiclePeer.create({ dataPath: createParams.dataDir, - name: params.name, - gitOptions: params.gitOptions, + ...params, radSeed: Array(64) .fill((peers.length + 1).toString()) .join(""), @@ -152,6 +154,8 @@ export class RadiclePeer { #eventRecords: NodeEvent[] = []; #outputLog: Stream.Writable; #gitOptions?: Record; + #passphrase: string; + #sshAuthSock?: string; #listenSocketAddr?: string; #httpdBaseUrl?: BaseUrl; #nodeProcess?: SpawnResult; @@ -168,6 +172,8 @@ export class RadiclePeer { radHome: string; logFile: Stream.Writable; name: string; + passphrase: string; + sshAuthSock?: string; }) { this.checkoutPath = props.checkoutPath; this.nodeId = props.nodeId; @@ -177,13 +183,20 @@ export class RadiclePeer { this.#radHome = props.radHome; this.#outputLog = props.logFile; this.#name = props.name; + this.#passphrase = props.passphrase; + this.#sshAuthSock = props.sshAuthSock; } - public async waitForEvent(searchEvent: NodeEvent, timeoutInMs: number) { + public async waitForEvent( + searchEvent: NodeEvent | ((event: NodeEvent) => boolean), + timeoutInMs: number, + ) { const start = new Date().getTime(); + const predicate = + typeof searchEvent === "function" ? searchEvent : matches(searchEvent); while (true) { - if (this.#eventRecords.find(matches(searchEvent))) { + if (this.#eventRecords.find(predicate)) { return; } if (new Date().getTime() - start > timeoutInMs) { @@ -204,6 +217,8 @@ export class RadiclePeer { gitOptions, radSeed: node, outputLog: logFile, + passphrase = "", + sshAuthSock, }: PeerManagerParams): Promise { const checkoutPath = Path.join(dataPath, name, "copy"); await Fs.mkdir(checkoutPath, { recursive: true }); @@ -219,9 +234,10 @@ export class RadiclePeer { const env = { ...gitOptions, RAD_HOME: radHome, - RAD_PASSPHRASE: "asdf", + RAD_PASSPHRASE: passphrase, RAD_KEYGEN_SEED: node, RAD_SOCKET: socket, + SSH_AUTH_SOCK: sshAuthSock, }; /* eslint-enable @typescript-eslint/naming-convention */ @@ -237,6 +253,8 @@ export class RadiclePeer { radHome, logFile, name, + passphrase, + sshAuthSock, }); } @@ -399,7 +417,8 @@ export class RadiclePeer { GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_NOSYSTEM: "1", RAD_HOME: this.#radHome, - RAD_PASSPHRASE: "asdf", + RAD_PASSPHRASE: this.#passphrase, + SSH_AUTH_SOCK: this.#sshAuthSock, RAD_LOCAL_TIME: "1671125284", RAD_KEYGEN_SEED: this.#radSeed, RAD_SOCKET: this.#socket, diff --git a/tests/support/router.ts b/tests/support/router.ts index c608a8612..4c1cd7ca4 100644 --- a/tests/support/router.ts +++ b/tests/support/router.ts @@ -1,11 +1,11 @@ import type { Page } from "@playwright/test"; -import { expect } from "@tests/support/fixtures.js"; +import { expect, reload } from "@tests/support/fixtures.js"; // Reloads the current page and verifies that the URL stays correct export const expectUrlPersistsReload = async (page: Page) => { const url = page.url(); - await page.reload(); + await reload(page); await expect(page).toHaveURL(url); };