test: downstream wire-compat (vectors + smoke)

[JohnMcLear]

PR Summary by Qodo

Add wire-compat vector tests and optional live smoke test (node:test via tsx)
🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Walkthroughs

User Description

Phase 2 of ether/etherpad#7923.

Core PR #7923 (Phase 1) added a canonical wire-format fixture that every Etherpad client must decode identically. This PR is the downstream half for etherpad-cli-client: it vendors that fixture and verifies this repo's own Changeset/AttributePool decoders produce identical results.

This repo previously had no test runner at all — this adds the first one, using the built-in node:test driver run through tsx (the repo is TS, ESM, .js-extension imports).

What's added

• test/fixtures/wire-vectors.json — the canonical fixture, vendored verbatim. 5 vectors: plain insert/delete, formatted insert, multiline insert, attribute reuse. Each says: applying changeset to initialText (with pool) yields resultText. Path overridable via ETHERPAD_WIRE_VECTORS.
• test/vectors.test.ts (pnpm run test:vectors) — server-free, must pass. Reconstructs each AttributePool via AttributePool.fromJsonable, applies the changeset with the repo's Changeset.applyToText (text equality) and Changeset.applyToAText (drives the pool decode through mustGetAttrib), asserting byte-for-byte against resultText.
• test/smoke.test.ts (pnpm run test:smoke) — live, skips cleanly with no server. Reads ETHERPAD_SMOKE_URL (default http://localhost:9003) and ETHERPAD_SMOKE_APIKEY. If no API key, or /api/ isn't reachable (1.5s probe), it t.skip()s instead of failing. With a server it: creates a pad via HTTP API → connects with the repo's own connect() socket.io client → appends via the USER_CHANGES write path (pad.append) → polls getText (bounded 10s) to confirm round-trip → deletes the pad.

Scripts

"test:vectors": "tsx --test test/vectors.test.ts",
"test:smoke":   "tsx --test test/smoke.test.ts",
"test":         "pnpm run test:vectors && pnpm run test:smoke"

These exact names are what the core CI manifest invokes.

Env contract

var	default	effect
ETHERPAD_WIRE_VECTORS	vendored test/fixtures/wire-vectors.json	override fixture path
ETHERPAD_SMOKE_URL	http://localhost:9003	smoke target
ETHERPAD_SMOKE_APIKEY	(unset → skip)	HTTP API key

Results

pnpm run test:vectors → 5 pass / 0 fail, no decoder changes needed (this repo's decoders are already wire-compatible). pnpm run test:smoke with no server → 1 skipped, 0 fail (green). Lint clean.

Only test files, the fixture, the tsx devDep, and the three scripts are added; no source refactors.

🤖 Generated with Claude Code

AI Description

• Vendor canonical Etherpad wire-format vectors and validate Changeset/AttributePool decoding.
• Add a server-free vectors suite plus a live smoke suite that skips cleanly without credentials.
• Introduce tsx-driven node:test runner and CI-compatible test:* scripts.

Diagram

graph TD
  TR(["pnpm test (tsx + node:test)"]) --> TV["vectors.test.ts"] --> CL["Changeset + AttributePool"]
  TV --> FX["wire-vectors.json"]
  TR --> TS["smoke.test.ts"] --> ES{{"Etherpad server"}}
  TS --> CL
  subgraph Legend
    direction LR
    _t([Test runner/test]) ~~~ _f["File/fixture"] ~~~ _ext{{External service}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Adopt Vitest/Jest as the test runner

• ➕ Richer ecosystem (watch mode, snapshots, coverage integrations)
• ➕ Common conventions for TS projects
• ➖ Heavier dependency footprint than node:test
• ➖ More configuration required for ESM + TS + .js-extension imports

2. Compile tests with `tsc` and run `node --test` on emitted JS

• ➕ Avoids runtime TS transpilation dependency
• ➕ Maximizes parity with published JS artifacts
• ➖ Adds build step and output management for tests
• ➖ Slower iteration; more CI plumbing for temporary build artifacts

3. Containerize Etherpad for smoke test in CI

• ➕ Makes the live smoke test deterministic and always exercised
• ➕ Avoids reliance on external environment variables/availability
• ➖ Increases CI complexity and runtime
• ➖ Requires maintaining a compatible Etherpad image/config

Recommendation: Keep the current tsx + node:test approach: it’s minimal, matches Node 18+ ESM constraints, and cleanly separates deterministic (vectors) from opportunistic (smoke) coverage via skip logic. Consider containerizing Etherpad later if always-on smoke coverage becomes a CI requirement.

File Changes

Tests (3)

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

Vendor canonical wire-format vectors fixture

• Adds a JSON fixture containing five canonical changeset vectors, including inserts, deletes, multiline changes, formatting, and attribute reuse cases.

test/fixtures/wire-vectors.json

---

smoke.test.ts Add live HTTP+socket.io round-trip smoke test with skip behavior +119/-0

Add live HTTP+socket.io round-trip smoke test with skip behavior

• Introduces an integration test that creates a pad via HTTP API, connects using the repo's socket.io client, appends via the USER_CHANGES path, and polls getText for round-trip verification. Skips cleanly when the API key is missing or the server is unreachable; performs best-effort cleanup by deleting the pad.

test/smoke.test.ts

---

vectors.test.ts Add offline wire-compat vector replay tests for decoders +56/-0

Add offline wire-compat vector replay tests for decoders

• Loads the wire vector fixture (path overridable via 'ETHERPAD_WIRE_VECTORS') and replays each changeset against the initial text. Verifies both 'Changeset.applyToText' and 'Changeset.applyToAText' outputs match the expected result text, exercising 'AttributePool.fromJsonable' decoding.

test/vectors.test.ts

---

Other (2)

package.json Add tsx and node:test scripts for vectors and smoke suites +5/-1

Add tsx and node:test scripts for vectors and smoke suites

• Adds 'tsx' as a devDependency and introduces 'test', 'test:vectors', and 'test:smoke' scripts. Updates lint script formatting to allow additional scripts.

package.json

---

pnpm-lock.yaml Lockfile updates for tsx (and transitive esbuild/fsevents) +290/-0

Lockfile updates for tsx (and transitive esbuild/fsevents)

• Records the newly added 'tsx@4.22.4' dependency and its transitive dependencies (notably 'esbuild' platform packages and optional 'fsevents').

pnpm-lock.yaml

---

[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. close() may be undefined ✓ Resolved 🐞 Bug ☼ Reliability

Description

smoke.test.ts calls client?.close(), but connect() returns an EventEmitter immediately and only
assigns close() later inside an async superagent.get(...).then(...) callback; if that callback never
runs, the finally block calls an undefined value and throws TypeError. This can mask the real
failure and make the smoke test fail during cleanup.

Code

test/smoke.test.ts[R110-112]

+  } finally {
+    client?.close();
+    // Best-effort cleanup.

Evidence

The smoke test always sets client = connect(...) before entering the finally, and then calls
client?.close(). In connect(), ee.close is not defined until later inside the .then(...)
callback, so close can be unset when cleanup runs.

test/smoke.test.ts[76-112]
src/index.ts[110-143]
src/index.ts[241-243]

Agent prompt

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

## Issue description
The smoke test calls `client?.close()` during cleanup. `connect()` returns before `close` is assigned, so `client` can be non-null while `client.close` is still `undefined`, causing a TypeError in the `finally` block.
### Issue Context
`connect()` assigns `ee.close = () => socket.close()` only after an async HTTP GET resolves.
### Fix
Preferred:
1) In `src/index.ts`, initialize `ee.close` immediately to a no-op and add a `.catch(...)` on the initial `superagent.get(...)` to emit `connect_error` and/or set `close` to a safe cleanup implementation.
2) In `test/smoke.test.ts`, defensively call `client?.close?.()` so cleanup never throws.
### Fix Focus Areas
- test/smoke.test.ts[76-112]
- src/index.ts[110-143]
- src/index.ts[241-243]

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

---

2. Dangling timeout keeps test alive ✓ Resolved 🐞 Bug ➹ Performance

Description

withTimeout() creates a setTimeout that is never cleared/unref'ed, so even if the wrapped promise
resolves quickly the pending timer can keep the Node test process alive until it fires (10s in the
socket-connect wait). This makes the smoke test unnecessarily slow and can delay CI completion.

Code

test/smoke.test.ts[R52-57]

+const withTimeout = <T>(p: Promise<T>, ms: number, label: string): Promise<T> =>
+  Promise.race([
+    p,
+    new Promise<T>((_, reject) =>
+      setTimeout(() => reject(new Error(`timeout: ${label}`)), ms)),
+  ]);

Evidence

The smoke test's withTimeout helper creates a setTimeout inside Promise.race without any
clearTimeout/unref, and it is used with a 10s timeout for socket connect—leaving a live timer behind
even on fast success.

test/smoke.test.ts[52-57]
test/smoke.test.ts[81-89]

Agent prompt

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

## Issue description
`withTimeout()` schedules a timer but never cancels it when the wrapped promise settles. In Node.js, pending timers keep the event loop alive, so the test process can remain running until the timeout elapses.
### Issue Context
This impacts `test:smoke`, especially the `withTimeout(..., 10000, 'socket connect')` call.
### Fix
Change `withTimeout()` to store the timeout handle and `clearTimeout()` it in a `finally` after `Promise.race()` settles, and consider calling `timeoutId.unref()` so the timer does not keep the process alive.
### Fix Focus Areas
- test/smoke.test.ts[52-57]
- test/smoke.test.ts[81-89]

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

---

[JohnMcLear]

Thanks Qodo — both bugs were valid and are fixed in e4b4643 (test-only, no source changes):

1. close() may be undefined — connect() assigns ee.close only inside its async bootstrap .then(), so the method can be undefined when the finally cleanup runs. Changed client?.close() → client?.close?.() to guard the call itself, not just the (always-defined) reference. Kept the fix in the test rather than hardening src/index.ts, to honour this PR's no-source-refactor scope.
2. Dangling timeout — withTimeout now clears its setTimeout in a finally, so a fast-resolving promise no longer leaves a pending timer keeping the test process alive.