--- id: implementation/fix-test-helper-consolidation name: "Tests: consolidate the duplicated harness shapes (review-002 N-5)" status: completed depends_on: [implementation/fix-coverage-residue] scope: narrow risk: low impact: component level: implementation --- ## Description The producer's test module re-implements the harness's shapes four times (about ~250 structural-duplicate test lines; (`docs/reviews/002-protocol-core-implementation-review.md` §N-5). The cause is legitimate (unit tests in `src/` cannot import `tests/harness.rs` are separate crates), so the fix is the standard shared-module pattern, not a harness import: - `src/producer.rs:954-987` (`QueueEgress`), `1237-1274` (`HotNoneEgress`), `1321-1359` (`PureHotNoneEgress`), `1504-1553` (`Witness` egress): four `Egress` impls of the same queue shape - `src/producer.rs:792-814` (`greet`/`write_request`/`read_reply`) vs `tests/harness.rs:876-933` (the same trio) - `src/producer.rs:719-745` (echo-dial) vs `tests/harness.rs:131` Fix per the review's sketch: a `tests/common/` path or a `#[cfg(test)]` `pub(crate)` module in `src/` compiled into both the lib test and the integration suite via a `path`-included module (the standard `mod common` pattern). Consolidates ~200 lines and keeps the two harness shapes from diverging further (they already have: the harness's `ChanEgress` and the producer's `QueueEgress` differ only in the mutex wrapper). **Not collapsible** (deliberately different; leave them): `HotNoneEgress`/`PureHotNoneEgress` (R-1's two distinct violation shapes) and the `Witness` egress (carries the drop flag). The consolidation targets the *byte-near-identical* shapes only: `QueueEgress`/echo-dial and the `greet`/`reply` trio. This task rides *after* the coverage-residue task so the consolidation sees the final test population (new tests from N-8 may add more duplicates to fold). If the coverage task's new tests introduce fresh duplicates of harness shapes, fold them here too. ## Acceptance Criteria - [x] The shared module exists and both the lib tests and the integration suite compile against it (the `mod common` pattern; no `tests/harness.rs` import from `src/`) - [x] `QueueEgress`/echo-dial and the `greet`/`write_request`/ `read_reply` trio are single-sourced (the byte-near-identical shapes) - [x] `HotNoneEgress`/`PureHotNoneEgress`/`Witness` remain test-local (deliberately distinct shapes, not collapsed) - [x] Zero behavior change: the full suite green with the same test count (minus any folded duplicates, recorded in the Notes) - [x] `cargo test` green; clippy/fmt clean; both wasm checks clean ## References - docs/reviews/002-protocol-core-implementation-review.md §N-5 - tests/harness.rs (the shapes being single-sourced) - src/producer.rs (the four `Egress` impls + the trio) ## Notes > Filled by the implementation agent (2026-09-25): - **The shared module is `tests/common/mod.rs`** (314 lines), the standard `mod common` pattern with two wiring details worth recording: - **The lib-test include rides `lib.rs`, not the test module**: `src/lib.rs` declares `#[cfg(test)] #[path = "../tests/common/mod.rs"] pub(crate) mod test_common;` plus `#[cfg(test)] extern crate self as alksocks;`. The `#[path]` from a nested module (`src/producer.rs`'s tests) does not resolve: `#[path]` is relative to the containing module's *directory* (`src/producer/tests/` for an inline child), and `#[path]` rejects non-literal expressions (no `concat!(env!("CARGO_MANIFEST_DIR"), ...)`), so the only clean mount point is the crate root, where `../tests/common/mod.rs` resolves. The `extern crate self` alias is the standard companion: the module is written once against the extern-crate paths (`alksocks::…`) the integration suite resolves; inside the lib test crate the alias makes the same paths resolve. `src/producer.rs` consumes it as `use crate::test_common as common;` with no `tests/harness.rs` import from `src/`. - **The integration suite reaches the same copy through `harness`**: `tests/harness.rs` declares `#[path = "common/mod.rs"] pub mod common;` and re-exports the consumed subset under the names its callers already use (`echo_egress` for `echo_egress_factory`, `udp_frame`/`udp_unframe` for `datagram_frame`/`decode_datagram`, `witnessing_echo_dial` for `echo_dial_witnessed`), and the per-test `use harness::{…}` surface is unchanged, so the consumer-* suites needed no edits at all. - **Single-sourced shapes** (the task's named targets plus the byte-identical stragglers the consolidation surfaced): - The echo dial: harness `echo_dial` + `witnessing_echo_dial` + producer's `echo_dial(targets)` → common's `echo_dial_witnessed` + `echo_dial` (a throwaway-witness wrapper); the producer keeps a one-line `echo_dial(targets)` adapter since its witness arg is per-test. - The refusing dial (all five `SocksDialError` variants reconstructed per call): three copies → one. - The queue egress + echo factory: harness `ChanEgress` + producer's `QueueEgress` (the review's mutex-wrapper-only divergence) + `echo_egress`/`echo_egress_factory` → common's `QueueEgress` + `echo_egress_factory` (the mutex-wrapped shape both had converged on, with `next_reply` parking on `recv()`; the readiness contract preserved verbatim). - The refusing egress factory → common's `refusing_egress` (harness copy; the producer's inline `Err(EgressError::Unreachable(…))` closures stayed, since they are one-liners, not the shape). - The raw RFC 1928 client trio: harness `RawRfcClient`'s `greet`/`request`/`read_reply` bodies + producer's `greet`/`write_request`/`read_reply` → common's `raw_greet`/`raw_request`/`raw_read_reply` (generic over `S: AsyncRead + AsyncWrite + Unpin`, substrate-agnostic per convention 7) + the unwrapping producer-shape wrappers (`greet`/`write_request`/`read_reply_v4`, the v4-only reply shape the producer tests' BND.ADDR assertions index). The harness's `RawRfcClient` now delegates to the common trio; its `connect()` convenience and two-ATYP-unaware API stay (its callers use it). - The datagram frame helpers: harness `udp_frame`/`udp_unframe` (test module) + core-integration's `udp_frame`/`udp_unframe` + producer's `datagram_frame`/`decode_datagram` → common's `datagram_frame`/`decode_datagram` (re-exported under both names). - The parked accept factory: harness-era producer `parked_accept_factory` + core-integration's `parked_accept` → common's `parked_accept_factory` (bind addr `127.0.0.1:2121`, the ECHO_BIND_PORT carrier; core-integration imports it aliased as `parked_accept`). - **Deliberately not collapsed** (per the task; verified test-local): producer's `HotNoneEgress` (mutex-yielding hot `Ok(None)`) and `PureHotNoneEgress` (zero-yield, R-1's two distinct violation shapes), `Witness`/`FailingSendWitness` (the drop flags), core-integration's `HotNoneEgress` (same-named but the concurrency-load variant), and the refusing accept-factory/accept-future pair (typed per error class; used by the harness and consumer-bind only). Also left: harness's `echo_accept` + producer's `bind_accept_factory` (the echo-pipe accept shape; the producer copy carries the per-test requested- target witness, so the bodies differ beyond the mutex wrapper; noted for the next touch but out of this task's named scope. - **Zero behavior change, same test count**: 145 lib / 38 associate / 41 bind / 40 connect / 41 integration / 30 harness / 37 differential / 3 doc-tests, byte-identical to the coverage task's post-N-8 population (no folded duplicates; the coverage task's new tests were all consumer-side unit shapes with no harness duplicates to fold). Net −307 lines (81 insertions / 388 deletions across the four touched files + the new 314-line module). - **Verification**: `cargo test` + `--all-features` green (same counts), clippy `-D warnings` clean (native + wasm), fmt clean, `cargo doc --no-deps` clean, both wasm checks clean (`check` + `clippy` on `wasm32-unknown-unknown`), `cargo publish --dry-run --allow-dirty` completes. The `tests/common/` path rides the existing `exclude = ["tasks/", …]`-style packaging posture automatically (dev-only; `#[cfg(test)]` keeps it out of the published lib; the integration binaries are not packaged). ## Summary Consolidated review 002 N-5's duplicated harness shapes into `tests/common/mod.rs`, the standard `mod common` pattern, mounted from `src/lib.rs` (`#[cfg(test)] #[path] pub(crate) mod test_common` + `extern crate self as alksocks`) for the lib test crate and from `tests/harness.rs` (`#[path = "common/mod.rs"]` + name-preserving re-exports) for the integration suite, so the per-test `use harness::{…}` surface and the producer tests' local names are unchanged. Single-sourced: the echo dial (plain + witnessing), the refusing dial, the queue egress + its echo factory (the harness/ producer mutex-wrapper divergence collapsed to one shape), the refusing egress, the raw RFC 1928 client trio (generic over the stream type; the harness's `RawRfcClient` delegates to it), the datagram frame codec helpers (three copies → one), and the parked accept factory. Left deliberately distinct: the hot-none egress variants (R-1's two shapes), the drop-witness egresses, and the typed accept-refusal factories. Zero behavior change (full suite green with the identical test population (375 + 2 ignored doc), no tests folded. Verification: `cargo test` + `--all-features` green, clippy `-D warnings` clean (native + wasm), fmt clean, doc clean, both wasm checks clean, publish dry-run completes.