test: downstream wire-compat (vectors + smoke)

[JohnMcLear]

PR Summary by Qodo

Add downstream wire-compat vectors guard and optional live smoke test
🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Walkthroughs

User Description

Phase 2 of ether/etherpad#7923

Phase 1 (core PR ether/etherpad#7923) added a canonical wire-format fixture (wire-vectors.json) that every Etherpad client must decode identically. This PR adds the downstream guard for the desktop/mobile shells.

Why the vectors test is a fixture-integrity guard here

The Rust/CLI clients re-implement changeset decoding, so they run these vectors through their own decoders. The desktop and mobile apps are thin shells — they load an Etherpad server URL and embed core's Ace editor in a webview. There is no local changeset decoder to exercise. So instead of a decode test, vectors.spec.ts asserts the shape/contract of the vendored fixture:

• every record has the 5 fields (name, initialText, changeset, pool, resultText) with the right types,
• pool.numToAttrib is a plain object and pool.nextNum a non-negative integer,
• initialText / resultText are non-empty and newline-terminated,
• changesets use the canonical Z: header, names are unique.

This guards against a malformed/empty fixture being injected into the repo and documents the contract the embedded editor relies on.

Smoke test (headless-light, by design)

The full Electron e2e stays in this repo's own CI. smoke.spec.ts is a deliberately headless-light HTTP roundtrip proving the shell could talk to a real server without booting Electron:

1. create a pad via the HTTP API,
2. fetch /p/<pad> (the exact URL the shell loads in its webview) → assert HTTP 200,
3. getText via the API to confirm content.

It skips cleanly (never fails CI) unless both a reachable server and an API key are present.

Env contract

Var	Default	Purpose
ETHERPAD_WIRE_VECTORS	vendored packages/shell/tests/fixtures/wire-vectors.json	override fixture path
ETHERPAD_SMOKE_URL	http://localhost:9003	server base URL for the smoke roundtrip
ETHERPAD_SMOKE_APIKEY	(unset)	HTTP API key; without it (or a reachable server) the smoke skips

Files

• packages/shell/tests/fixtures/wire-vectors.json — vendored canonical fixture
• packages/shell/tests/wire/vectors.spec.ts — fixture-integrity guard (22 cases)
• packages/shell/tests/wire/smoke.spec.ts — live headless-light roundtrip / clean skip
• package.json — test:vectors and test:smoke root scripts

Verification

• pnpm run test:vectors → 22 passed
• pnpm run test:smoke (no server / no key) → skips cleanly, suite green
• full pnpm --filter @etherpad/shell test → 232 passed; lint + typecheck clean

🤖 Generated with Claude Code

AI Description

• Vendor canonical wire-vectors fixture and assert its contract/shape stays valid.
• Add optional live HTTP smoke roundtrip that cleanly skips without server+API key.
• Expose focused root scripts to run vectors and smoke tests from CI/dev.

Diagram

graph TD
  CI["CI / Developer"] --> PKG["package.json scripts"] --> V["Vitest runner"] --> VEC["vectors.spec.ts"] --> FIX[/"wire-vectors.json"/]
  V --> SMK["smoke.spec.ts"] --> SRV{{"Etherpad server"}} --> EP[/"HTTP API + /p/<pad>"/]

  subgraph Legend
    direction LR
    _proc["Test/process"] ~~~ _io[/"Fixture/IO"/] ~~~ _ext{{"External system"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Provision an ephemeral Etherpad (Docker) for smoke in CI

• ➕ Smoke test runs deterministically in CI (no skips)
• ➕ Catches server-compat regressions earlier and more consistently
• ➖ Adds CI complexity, runtime, and maintenance burden
• ➖ Conflicts with goal of never failing without extra infrastructure

2. Mock the server contract (fake /api + /p/)

• ➕ Fully offline and deterministic
• ➕ Fast, no credentials needed
• ➖ Doesn't validate real Etherpad behavior
• ➖ Easy for mocks to drift from actual server contract

3. Run a minimal Electron/webview e2e in this repo

• ➕ Closest to real desktop/mobile shell behavior
• ➕ Validates embedded editor boot path end-to-end
• ➖ Slow/flaky compared to pure HTTP tests
• ➖ Requires heavier CI setup; overlaps with downstream CI by design

Recommendation: Keep the current approach: a strict fixture-integrity guard plus an opt-in, clean-skipping live HTTP smoke test. It delivers downstream protection without introducing CI infrastructure requirements or duplicating Electron e2e coverage that belongs in downstream pipelines.

File Changes

Tests (3)

wire-vectors.json Vendor canonical wire-format vectors fixture +7/-0

Vendor canonical wire-format vectors fixture

• Adds a downstream copy of the canonical 'wire-vectors.json' fixture. The vectors cover basic insert/delete, formatted insert, multiline insert, and attribute reuse scenarios.

packages/shell/tests/fixtures/wire-vectors.json

---

smoke.spec.ts Add optional live Etherpad HTTP roundtrip smoke test +86/-0

Add optional live Etherpad HTTP roundtrip smoke test

• Adds a headless-light smoke test that, when configured, creates a pad via the HTTP API, fetches '/p/<pad>' (webview URL) and verifies 'getText'. The test skips cleanly unless a reachable server and 'ETHERPAD_SMOKE_APIKEY' are present.

packages/shell/tests/wire/smoke.spec.ts

---

vectors.spec.ts Add fixture-integrity guard for wire vectors contract +88/-0

Add fixture-integrity guard for wire vectors contract

• Adds a Vitest suite that validates the vendored vectors are non-empty, uniquely named, and conform to the expected schema. It asserts canonical 'Z:' changeset headers, pool shape ('numToAttrib' object, non-negative integer 'nextNum'), and newline-terminated non-empty texts; fixture path is overrideable via 'ETHERPAD_WIRE_VECTORS'.

packages/shell/tests/wire/vectors.spec.ts

---

Other (1)

package.json Add root scripts for vectors and smoke test entrypoints +2/-0

Add root scripts for vectors and smoke test entrypoints

• Introduces 'test:vectors' and 'test:smoke' scripts that run targeted Vitest specs in '@etherpad/shell'. This makes the downstream wire-compat checks easy to invoke from CI and locally.

package.json

---

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. Wasted reachability probe ✓ Resolved 🐞 Bug ➹ Performance

Description

beforeAll() always calls reachable() even when ETHERPAD_SMOKE_APIKEY is unset (a case that
always exits early), adding an avoidable network timeout delay to every run of this test file. This
reduces test reliability in constrained environments and unnecessarily slows @etherpad/shell’s
vitest run.

Code

packages/shell/tests/wire/smoke.spec.ts[R51-66]

+let serverUp = false;
+
+beforeAll(async () => {
+  serverUp = await reachable();
+});
+
+describe('live server smoke (shell HTTP contract)', () => {
+  it('completes a create -> fetch /p/<pad> -> getText roundtrip', async () => {
+    if (!serverUp || !APIKEY) {
+      const why = !serverUp ? `no Etherpad reachable at ${BASE}` : `ETHERPAD_SMOKE_APIKEY not set`;
+      console.warn(
+        `[smoke] ${why} — skipping live smoke test. ` +
+          `Set ETHERPAD_SMOKE_URL + ETHERPAD_SMOKE_APIKEY to run it.`,
+      );
+      return; // skip cleanly: never fail the gate without a reachable server + key
+    }

Evidence

The file computes APIKEY from env, probes reachability unconditionally in beforeAll(), and then
exits early when APIKEY is empty—so the probe cannot change the outcome in the no-key case.

packages/shell/tests/wire/smoke.spec.ts[23-25]
packages/shell/tests/wire/smoke.spec.ts[53-66]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`reachable()` is executed unconditionally in `beforeAll()`, even though the test will immediately exit when `ETHERPAD_SMOKE_APIKEY` is not set. This adds a needless network call + timeout to runs that cannot possibly execute the smoke.
### Issue Context
The skip condition is `(!serverUp || !APIKEY)`, but `serverUp` is computed regardless of whether `APIKEY` exists.
### Fix Focus Areas
- packages/shell/tests/wire/smoke.spec.ts[51-66]
### Suggested change
- Only probe reachability when `APIKEY` is present (e.g., `if (APIKEY) serverUp = await reachable();`).
- Optionally, restructure the test so the reachability check happens only when needed (inside the test after confirming `APIKEY` exists), which also lets you remove the mutable `serverUp` global.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Pad cleanup not guaranteed ✓ Resolved 🐞 Bug ☼ Reliability

Description

The smoke test creates a pad but only calls deletePad at the end of the happy path, so any failure
before that (HTTP fetch/expect/API assertion) leaves the pad behind on the target server. This can
pollute persistent/shared smoke servers over time.

Code

packages/shell/tests/wire/smoke.spec.ts[R68-85]

+    const padID = `phase2-smoke-${Date.now()}`;
+    const text = 'phase2 wire-compat smoke\n';
+
+    const created = await api<null>('createPad', { padID, text });
+    expect(created.code, created.message).toBe(0);
+
+    // The exact URL the shell loads in its webview.
+    const padRes = await fetch(`${BASE}/p/${encodeURIComponent(padID)}`, {
+      signal: AbortSignal.timeout(5000),
+    });
+    expect(padRes.status).toBe(200);
+
+    const got = await api<{ text: string }>('getText', { padID });
+    expect(got.code, got.message).toBe(0);
+    expect(got.data.text).toBe(text);
+
+    await api<null>('deletePad', { padID });
+  });

Evidence

The code issues createPad and performs subsequent assertions before deletePad; there is no
try/finally or other guaranteed cleanup path.

packages/shell/tests/wire/smoke.spec.ts[68-85]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`deletePad` is not guaranteed to run if any step after `createPad` fails, leaving test pads behind.
### Issue Context
The test creates a pad, does multiple network calls/assertions, then deletes it only at the end.
### Fix Focus Areas
- packages/shell/tests/wire/smoke.spec.ts[68-85]
### Suggested change
Wrap the smoke roundtrip in `try/finally`:
- After successful `createPad`, run the fetch + `getText` assertions in a `try` block.
- In `finally`, call `deletePad` (and consider swallowing delete errors so cleanup doesn’t mask the real failure):
- `await api('deletePad', { padID }).catch(() => {})`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[JohnMcLear]

Thanks @qodo — both findings addressed in 1e77b74:

1. Wasted reachability probe — beforeAll now only probes when ETHERPAD_SMOKE_APIKEY is set (if (APIKEY) serverUp = await reachable();), so no-key runs skip without any network call/timeout.
2. Pad cleanup not guaranteed — the create → fetch → getText roundtrip is now wrapped in try/finally, with deletePad in finally and its errors swallowed (.catch(() => {})) so cleanup never masks the real failure.