test: downstream wire-compat (vectors + smoke)

[JohnMcLear]

PR Summary by Qodo

Add wire-compat vectors fixture and live smoke test for etherpad-client
🧪 Tests 🕐 20-40 Minutes

Walkthroughs

User Description

Phase 2 of ether/etherpad#7923

Phase 1 (ether/etherpad#7923) added a canonical wire-format fixture that every Etherpad client must decode identically. This PR is the downstream half for the Rust etherpad-client crate: it vendors that fixture and proves the crate's changeset decoder + OT apply path stays in lockstep with core's serialization.

No production code changed — test files and a vendored JSON fixture only.

Files added

• crates/etherpad-client/tests/fixtures/wire-vectors.json — the canonical vectors, vendored verbatim. Path overridable via ETHERPAD_WIRE_VECTORS so core CI can inject a fresh copy.
• crates/etherpad-client/tests/vectors.rs — server-free, runs in normal CI. Loads the fixture (serde_json) and, for each vector, applies changeset to initialText using the crate's own changeset::parser::parse + ot::apply (the same code changeset_roundtrip.rs / ot_apply.rs exercise), then asserts the result equals resultText. Covers plain insert/delete, formatted insert (*0), multiline insert (|2), and attribute reuse.
• crates/etherpad-client/tests/smoke.rs — live, #[ignore]'d. Connects a PadSession to a fresh pad, sends a changeset inserting a known marker, and verifies the round-trip via the HTTP getText API (when an apikey is supplied) or a fresh PadSession's initial_text. Modeled on the integration_etherpad.rs skip pattern: if no server is reachable it prints a skip message and returns Ok rather than failing.

Manifest commands core runs

• cargo test --test vectors — passes (all 5 vectors).
• cargo test --test smoke -- --ignored — runs the live smoke test; compiles and lists with no server via cargo test --test smoke -- --ignored --list.

Env contract

• ETHERPAD_WIRE_VECTORS — override fixture path (default: vendored tests/fixtures/wire-vectors.json).
• ETHERPAD_SMOKE_URL — smoke base URL (fallback PAD_ETHERPAD_BASE, then http://localhost:9003).
• ETHERPAD_SMOKE_APIKEY — apikey for the HTTP getText verification path.

Notes

Applying a changeset produces only TEXT; attribute pools annotate spans but don't change the resulting characters, so the pool field is loaded for schema fidelity with core's fixture but isn't needed to verify text output. No decoder bug was found — the existing parser + apply handle all five vectors as-is.

🤖 Generated with Claude Code

AI Description

• Vendor canonical wire-format vectors to ensure changeset decode/apply stays compatible with
  Etherpad core.
• Add a server-free test that parses each changeset and asserts resulting text matches fixture.
• Add an ignored live smoke test that round-trips a marker via Etherpad WS + optional HTTP getText.

Diagram

graph TD
  V["wire-vectors.json"] --> TV["vectors.rs test"] --> CP["changeset::parser"] --> OA["ot::apply"] --> AS["assert resultText"]
  TS["smoke.rs test (ignored)"] --> PS["PadSession"] --> EP["Etherpad server"]
  PS --> HTTP["HTTP getText (optional)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Fetch vectors from upstream at test time (URL/artifact)

• ➕ Always tests against the newest canonical fixture without manual vendor updates
• ➕ Reduces repository churn from fixture updates
• ➖ Introduces network/artifact-availability flakiness in CI
• ➖ Harder to reproduce failures locally without the fetched artifact

2. Embed vectors directly in Rust test source

• ➕ No file-path/env handling; simplest runtime IO
• ➕ Easier to evolve alongside test expectations
• ➖ Harder to keep byte-for-byte identical to upstream canonical fixture
• ➖ More painful diffs when upstream fixture changes (Rust string escaping, formatting)

3. Submodule/subtree sync from Etherpad core testdata directory

• ➕ Keeps provenance and update flow explicit; can be pinned to core SHA
• ➕ Still avoids network calls during tests
• ➖ Adds workflow/tooling overhead for contributors and CI
• ➖ More complex repository setup (submodules)

Recommendation: The current approach (vendored JSON fixture with an env override, plus a server-free vectors test and an ignored live smoke test) is the best balance of determinism and real-world coverage: CI stays stable/offline by default while core CI can inject a fresh fixture or run the smoke test against a real server when desired.

File Changes

Tests (3)

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

Vendor canonical wire-format vectors fixture

• Adds the canonical upstream wire-format vectors as a JSON fixture. Provides test cases for inserts/deletes, formatting attributes, multiline inserts, and attribute reuse.

crates/etherpad-client/tests/fixtures/wire-vectors.json

---

smoke.rs Add ignored live smoke test for wire round-trip via Etherpad +194/-0

Add ignored live smoke test for wire round-trip via Etherpad

• Introduces an ignored tokio-based smoke test that connects to a real Etherpad, sends a marker via PadSession, and verifies persistence via HTTP getText (apikey) or a fresh session. Implements reachability checks and a skip-on-unreachable pattern to avoid failing in environments without a server.

crates/etherpad-client/tests/smoke.rs

---

vectors.rs Add server-free vectors test using parser + OT apply +96/-0

Add server-free vectors test using parser + OT apply

• Loads the vendored (or env-overridden) vectors JSON, parses each changeset using the crate's changeset parser, applies it via ot::apply, and asserts the resulting text matches resultText. Aggregates failures to provide a single actionable report across all vectors.

crates/etherpad-client/tests/vectors.rs

---

[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. Smoke skip too shallow ✓ Resolved 🐞 Bug ☼ Reliability

Description

smoke_wire_roundtrip only skips when a plain HTTP GET to the base URL fails, but it uses expect()
for Etherpad-specific setup (cookie fetch, WS connect, handshake), so partially-available or wrong
endpoints will fail the test instead of skipping.

Code

crates/etherpad-client/tests/smoke.rs[R103-131]

+    if !reachable(&base).await {
+        eprintln!("Etherpad not reachable at {base}, skipping smoke test");
+        return;
+    }
+    let pad_id = format!(
+        "pad-rust-smoke-{}",
+        std::time::SystemTime::now()
+            .duration_since(std::time::UNIX_EPOCH)
+            .unwrap()
+            .as_secs()
+    );
+    eprintln!("target: {base}/p/{pad_id}");
+
+    // Connect a session and push a unique marker.
+    let cookie = TungsteniteSocket::fetch_pad_cookie(&base, &pad_id)
+        .await
+        .expect("fetch_pad_cookie");
+    let mut socket = TungsteniteSocket::new(&base, Some(cookie));
+    socket.connect().await.expect("ws connect");
+    let mut session = PadSession::new(
+        Box::new(socket),
+        SessionConfig {
+            pad_id: pad_id.clone(),
+            token: "t.smoke".into(),
+            protocol_version: 2,
+        },
+    );
+    session.handshake().await.expect("handshake");
+

Evidence

The file-level comment claims the test is safe to run in environments with no Etherpad, but the
implementation only skips on a successful/failed GET to base and then hard-fails via expect()
during Etherpad-specific setup steps.

crates/etherpad-client/tests/smoke.rs[8-12]
crates/etherpad-client/tests/smoke.rs[31-39]
crates/etherpad-client/tests/smoke.rs[103-131]

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 ignored smoke test intends to be safe to run when Etherpad isn’t available, but it only checks reachability via `GET base` and then uses `expect()` for subsequent Etherpad-specific steps. If the base URL responds to HTTP but Etherpad is not actually ready/compatible (or websocket/cookie fetch fails), the test will fail instead of skipping.
### Issue Context
This is test-only code, but it is intended to run in CI via `cargo test --test smoke -- --ignored`.
### Fix Focus Areas
- crates/etherpad-client/tests/smoke.rs[31-39]
- crates/etherpad-client/tests/smoke.rs[103-131]
### Suggested change
- Treat `fetch_pad_cookie`, `connect`, and `handshake` failures as a skip condition:
- replace `expect(...)` with `match`/`if let Err(e)` blocks that `eprintln!("...skipping: {e}")` and `return;`
- Optionally strengthen `reachable()` to check an Etherpad-specific endpoint (or just attempt `fetch_pad_cookie` as the “is Etherpad reachable” probe).

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

---

2. Pad ID seconds collision ✓ Resolved 🐞 Bug ☼ Reliability

Description

pad_id is derived only from UNIX epoch seconds, so multiple smoke runs started in the same second
can target the same pad and interfere with each other (flaky results / cross-test contamination).

Code

crates/etherpad-client/tests/smoke.rs[R107-113]

+    let pad_id = format!(
+        "pad-rust-smoke-{}",
+        std::time::SystemTime::now()
+            .duration_since(std::time::UNIX_EPOCH)
+            .unwrap()
+            .as_secs()
+    );

Evidence

The pad id uses only as_secs() (second resolution), while the test already uses a UUID for the
inserted marker, indicating a readily-available uniqueness source that is not applied to pad naming.

crates/etherpad-client/tests/smoke.rs[107-113]
crates/etherpad-client/tests/smoke.rs[134-136]

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’s `pad_id` uses only second-resolution time, which can collide across parallel invocations or quick retries.
### Issue Context
The test already depends on `uuid` (used for the marker), so making the pad id unique is straightforward.
### Fix Focus Areas
- crates/etherpad-client/tests/smoke.rs[107-113]
- crates/etherpad-client/tests/smoke.rs[132-136]
### Suggested change
Replace the time-based pad id with a UUID-based (or time+UUID) id, e.g.:
- `let pad_id = format!("pad-rust-smoke-{}", uuid::Uuid::now_v7());`
This avoids collisions and reduces flakiness when multiple ignored tests run concurrently.

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

---

[JohnMcLear]

Thanks @qodo-free-for-open-source-projects — both bugs fixed in ada8caf:

1. Smoke skip too shallow — fetch_pad_cookie, connect, and handshake now skip (eprintln + return) on failure instead of expect(), so a reachable-but-not-Etherpad endpoint skips cleanly rather than hard-failing. The file-level doc comment now states this broadened skip behavior explicitly.
2. Pad ID seconds collision — pad id now uses uuid::Uuid::now_v7() (the crate is already a dev-dep, used for the marker) instead of second-resolution epoch time, eliminating cross-run collisions.