alksocks/tasks/implementation/fix-test-helper-consolidation.md

id, name, status, depends_on, scope, risk, impact, level

id	name	status	depends_on	scope	risk	impact	level
implementation/fix-test-helper-consolidation	Tests: consolidate the duplicated harness shapes (review-002 N-5)	completed	implementation/fix-coverage-residue	narrow	low	component	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

• 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/)
• QueueEgress/echo-dial and the greet/write_request/ read_reply trio are single-sourced (the byte-near-identical shapes)
• HotNoneEgress/PureHotNoneEgress/Witness remain test-local (deliberately distinct shapes, not collapsed)
• Zero behavior change: the full suite green with the same test count (minus any folded duplicates, recorded in the Notes)
• 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.