Centralize integration test helpers in wireframe_testing (integration_helpers)

[leynos]

Summary

• Centralize integration test helpers in the wireframe_testing crate via a new integration_helpers module, exposing CommonTestEnvelope, TestApp, TestResult, factory, and unused_listener for shared integration tests.
• Introduce core runtime changes to support AppFactory-based app creation and adapt the server to use a generic AppFactory instead of a Fn() -> WireframeApp factory.
• Update tests to import from wireframe_testing and to use the new integration helpers and AppFactory-based factories. Propagate IO results from helpers (e.g., unused_listener) as appropriate.
• Update documentation to reference the new location and usage.

Changes

Core

• Introduced AppFactory<Ser, Ctx, E, Codec> trait and its impls, enabling a uniform factory interface for creating WireframeApp instances.
• Updated WireframeServer and related runtime components to bound on AppFactory<Ser, Ctx, E, Codec> instead of a Fn() -> WireframeApp, enabling shared integration helpers across crates.
• Updated internal wiring (spawn_connection_task, accept_loop, etc.) to build apps via the AppFactory trait and to work with the FactoryResult abstraction where needed.

Integrations helpers and exports

• New: wireframe_testing/src/integration_helpers.rs
  • CommonTestEnvelope: simple envelope with correlation support implementing CorrelatableFrame and Packet.
  • TestApp: alias for wireframe::app::WireframeApp with BincodeSerializer and unit envelope.
  • TestResult: shared result type for integration tests.
  • unused_listener(): helper to bind to a free local port (StdTcpListener) with proper error propagation.
  • factory: a default WireframeApp factory fixture for tests.
• Exports
  • wireframe_testing/src/lib.rs: expose integration_helpers via pub mod integration_helpers and re-export CommonTestEnvelope, TestApp, TestResult, factory, unused_listener for ergonomic imports.

Documentation

• docs/rust-testing-with-rstest-fixtures.md: note added about using integration helpers from wireframe_testing to compile once and avoid per-test-crate lint suppressions.
• docs/wireframe-testing-crate.md: mention integration_helpers module and its purpose.
• wireframe_testing/README.md: document the integration_helpers usage and example (as part of integration helpers section).

Tests

• Many tests updated to import from wireframe_testing (e.g., TestResult, TestApp) and to use the new integration_helpers infrastructure.
• Some test code adjusted to handle Result-based helpers (e.g., unused_listener calls now propagated with ? in appropriate spots).

Migration notes

• Tests previously importing from crate::common should now import from wireframe_testing, e.g.:
  • use wireframe_testing::{TestResult, TestApp, factory};
  • use wireframe_testing::integration_helpers::CommonTestEnvelope;
• If you relied on unused_listener returning a plain StdTcpListener, handle IO errors (e.g., ? operator) where appropriate since the helper now returns a Result.
• Some test crates may need to adjust to AppFactory-based factories and updated error propagation.

Test plan

• Run cargo test -p wireframe_testing to ensure compilation and tests pass.
• Run full workspace test suite to validate cross-crate integration tests continue to compile and behave as expected.

◳ Generated by DevBoxer ◰

---

ℹ️ Tag @devboxerhub to ask questions and address PR feedback

📎 Task: https://www.devboxer.com/task/daed61f9-98b1-47c5-a303-59dbf443fb3f

📝 Closes #433

Summary by Sourcery

Centralize shared integration test helpers in the wireframe_testing crate and update tests and docs to use the new integration_helpers module.

New Features:

• Introduce the integration_helpers module in wireframe_testing providing shared integration test utilities like CommonTestEnvelope, TestApp, TestResult, a default app factory, and an unused TCP listener helper.

Enhancements:

• Re-export core integration testing helpers from wireframe_testing::lib for ergonomic imports across test crates.
• Adjust integration tests to depend on wireframe_testing for common helpers instead of local tests/common modules, including propagating IO errors from the unused_listener helper.

Documentation:

• Document the new integration_helpers module and its usage in the wireframe_testing README and crate documentation.
• Extend rstest fixture guidance to recommend hosting shared integration helpers in the wireframe_testing crate for reuse across test crates.

[sourcery-ai]

Reviewer's Guide

Centralizes shared integration test helpers into a new wireframe_testing::integration_helpers module, re-exports its main types from the crate root, updates integration tests to consume them, and refreshes documentation to describe the new workflow and migration notes.

Class diagram for wireframe_testing integration_helpers module

classDiagram
    class CommonTestEnvelope {
        +u32 id
        +Option~u64~ correlation_id
        +Vec~u8~ payload
        +correlation_id() Option~u64~
        +set_correlation_id(correlation_id Option~u64~) void
        +id() u32
        +into_parts() PacketParts
        +from_parts(parts PacketParts) CommonTestEnvelope
    }

    class CorrelatableFrame {
        <<trait>>
        +correlation_id() Option~u64~
        +set_correlation_id(correlation_id Option~u64~) void
    }

    class Packet {
        <<trait>>
        +id() u32
        +into_parts() PacketParts
        +from_parts(parts PacketParts) Self
    }

    class PacketParts {
        +id() u32
        +correlation_id() Option~u64~
        +into_payload() Vec~u8~
        +new(id u32, correlation_id Option~u64~, payload Vec~u8~) PacketParts
    }

    class WireframeApp {
        <<generic>>
        -BincodeSerializer serializer
        -unit_envelope Envelope
    }

    class BincodeSerializer {
        <<type>>
    }

    class Envelope {
        <<type_parameter>>
    }

    class TestApp {
        <<type_alias>>
        +alias_of WireframeApp~BincodeSerializer, (), Envelope~
    }

    class TestResult {
        <<type_alias>>
        +alias_of Result~T, Box~dyn Error + Send + Sync~~
    }

    class factory {
        <<fixture_function>>
        +factory() Fn() -> TestApp + Send + Sync + Clone + 'static
    }

    class unused_listener_fn {
        <<function>>
        +unused_listener() Result~StdTcpListener, std::io::Error~
    }

    CommonTestEnvelope ..|> CorrelatableFrame
    CommonTestEnvelope ..|> Packet
    PacketParts o-- CommonTestEnvelope

    WireframeApp o-- BincodeSerializer
    WireframeApp o-- Envelope

    TestApp ..|> WireframeApp
    TestResult ..|> Result

    factory ..> TestApp
    unused_listener_fn ..> StdTcpListener

Loading

File-Level Changes

Change	Details	Files
Introduce a centralized integration_helpers module in wireframe_testing for shared integration test utilities.	Add CommonTestEnvelope implementing CorrelatableFrame and Packet with correlation-aware payload handling. Define TestApp as a WireframeApp alias using BincodeSerializer and unit envelope for integration tests. Define TestResult as a boxed error Result alias for shared use across test suites. Implement unused_listener returning Result bound to an ephemeral localhost port. Provide a rstest factory() fixture returning a default TestApp builder closure.	wireframe_testing/src/integration_helpers.rs
Expose integration helpers from the wireframe_testing crate root for ergonomic imports.	Declare pub mod integration_helpers in the crate root. Re-export CommonTestEnvelope, TestApp, TestResult, factory, and unused_listener from the crate root for direct use in test crates.	wireframe_testing/src/lib.rs
Refactor tests to consume shared helpers from wireframe_testing instead of local tests/common modules and adjust for Result-returning helpers.	Replace imports from crate::common and tests/common/mod.rs with wireframe_testing::{TestResult, TestApp, factory, unused_listener, CommonTestEnvelope} as appropriate. Remove tests/common/mod.rs and its re-exports now that helpers live in wireframe_testing. Update call sites of unused_listener to propagate IO errors with the ? operator where necessary. Simplify test modules that re-exported TestResult for BDD fixtures to re-export wireframe_testing::TestResult instead. Consolidate various helper imports (decode_frames, push_expect, logger, etc.) together with TestResult/TestApp imports from wireframe_testing where relevant.	tests/preamble/timeouts.rs tests/response.rs tests/fixtures/panic.rs tests/connection.rs tests/connection_actor_errors.rs tests/connection_actor_fairness.rs tests/connection_actor_shutdown.rs tests/lifecycle.rs tests/metadata.rs tests/middleware_order.rs tests/multi_packet_streaming.rs tests/push.rs tests/push_policies.rs tests/server.rs tests/bdd/mod.rs tests/fixtures/correlation.rs tests/fixtures/message_assembler.rs tests/fixtures/multi_packet.rs tests/multi_packet.rs tests/connection_fragmentation.rs tests/fixtures/client_lifecycle.rs tests/fixtures/client_preamble.rs tests/fixtures/client_runtime.rs tests/fixtures/codec_error/mod.rs tests/fixtures/codec_stateful.rs tests/fixtures/fragment/mod.rs tests/fixtures/request_parts.rs tests/session_registry.rs tests/preamble.rs tests/preamble/support.rs tests/stream_end.rs tests/wireframe_protocol.rs tests/common/mod.rs
Update documentation to describe the new integration_helpers module and recommend using it for shared integration helpers.	Document integration_helpers usage and example in wireframe_testing/README.md. Mention src/integration_helpers.rs as the place for shared integration helpers in docs/wireframe-testing-crate.md. Add guidance in docs/rust-testing-with-rstest-fixtures.md to favor exporting cross-crate integration helpers from wireframe_testing to avoid per-crate lint suppressions.	wireframe_testing/README.md docs/wireframe-testing-crate.md docs/rust-testing-with-rstest-fixtures.md

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#433	Extract integration test helpers from tests/common/mod.rs into the wireframe_testing crate as public API (e.g., CommonTestEnvelope, TestApp, TestResult, factory, unused_listener).	✅
#433	Update integration tests to import and use the helpers from wireframe_testing instead of tests/common, and remove the tests/common module (including its file-level #![allow(...)] attributes).	✅
#433	Update documentation/guidance to reflect the new location and usage of integration helpers in wireframe_testing and the rationale (single compilation, no per-test-crate lint suppressions).	✅

Possibly linked issues

• #0: PR moves helpers to wireframe_testing::integration_helpers, updates all tests and docs, and deletes tests/common module

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Move shared integration test helpers into a new public integration_helpers module in the wireframe_testing crate; update many integration tests to import helpers from wireframe_testing; adjust some listener call sites to propagate errors; introduce an AppFactory trait and accept-loop refactor to create per‑connection apps via that trait.

Changes

Cohort / File(s)	Summary
Docs & README docs/rust-testing-with-rstest-fixtures.md, docs/wireframe-testing-crate.md, wireframe_testing/README.md	Document new integration_helpers and guidance to export shared integration helpers from wireframe_testing.
New testing module & exports wireframe_testing/src/integration_helpers.rs, wireframe_testing/src/lib.rs, wireframe_testing/Cargo.toml	Add integration_helpers with CommonTestEnvelope, unused_listener(), TestApp, TestResult, TestError, factory() fixture; re-export these from crate root; add thiserror dependency.
wireframe_testing tests wireframe_testing/tests/integration_helpers.rs	Add integration test exercising new helpers (server start, client send/verify, shutdown).
Removed legacy common tests/common/mod.rs, tests/bdd/mod.rs, tests/preamble.rs	Remove legacy tests/common module and its pub mod common re-exports.
Tests — import migration many tests/*.rs, tests/fixtures/*, tests/preamble/*, tests/fragment_transport/* (≈45 files)	Replace imports and public re-exports to use wireframe_testing::{TestResult, TestApp, CommonTestEnvelope, factory, unused_listener, TestError}; minor call-site changes such as unused_listener()?.
Fragment helpers relocation tests/common/fragment_helpers.rs, tests/fragment_transport/*, tests/fragment_transport/...	Expose fragment helpers via explicit path (#[path = "common/fragment_helpers.rs"] mod fragment_helpers;) and import from crate::fragment_helpers; simplify some error construction to return TestError variants directly; add pub type TestResult alias in fragment_helpers.
Tests — error consolidation tests/multi_packet.rs, related tests	Change local error handling to use TestError instead of Box<dyn Error + Send + Sync> and import TestError/TestResult from wireframe_testing.
Server factory refactor src/server/mod.rs, src/server/config/*, src/server/connection.rs, src/server/runtime.rs, src/server/config/binding.rs, src/server/config/mod.rs, src/server/config/preamble.rs	Introduce FactoryResult and AppFactory traits; replace closure-based Fn() -> WireframeApp factory bounds with AppFactory; add handle_app_connection; tighten generic bounds (require Codec: FrameCodec, E: Packet, etc.).
New accept loop src/server/runtime/accept.rs	Add generic, cancellable accept loop and iteration logic with AcceptListener, AcceptLoopOptions, PreambleHooks, backoff behaviour and spawning of connection tasks via AppFactory.
Test tweaks & helpers wireframe_testing/src/helpers.rs, various test files	Tighten explicit generics in tests, use BoxFuture<'static, ()> handlers, refine payload assertions and small test adjustments.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Test as Test harness
participant WT as wireframe_testing::integration_helpers
participant Listener as OS Listener
participant Server as WireframeServer (AppFactory)
participant Client as WireframeClient

Test->>WT: call factory() and unused_listener()
WT-->>Test: return App factory and StdTcpListener
Test->>Server: start server with factory() bound to Listener
Server->>Listener: accept connection
Client->>Server: send CommonTestEnvelope
Server->>Server: AppFactory.build() -> instantiate App
Server->>Server: run preamble and route handler
Server-->>Client: send response with correlation_id
Client-->>Test: receive and verify response
Test->>Server: signal shutdown
Server-->>Test: shutdown completes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

"
Move the helpers, bind the port,
Factories spin up apps on court,
Tests send envelopes, responses come,
Accept loop keeps the traffic hum,
Shared helpers now live as one."

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR introduces AppFactory trait changes to the runtime module which, whilst related to the factory-based approach, extend beyond the original scope of moving test helpers from tests/common to wireframe_testing.	Separate the AppFactory and runtime refactoring into a distinct PR, keeping this one focused solely on extracting and centralising integration test helpers from tests/common to wireframe_testing.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title clearly summarises the main change: centralising integration test helpers in wireframe_testing via an integration_helpers module.
Description check	✅ Passed	The pull request description comprehensively explains the changes: centralising helpers, introducing AppFactory, updating tests, and modifying documentation.
Linked Issues check	✅ Passed	The PR fully addresses issue #433 by moving integration test helpers from tests/common to wireframe_testing, eliminating lint suppressions, and providing a proper public API for test utilities.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor-move-test-helpers-to-wireframe-testing-ddh3wt

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[leynos]

@coderabbitai Have the following now been resolved?

Please address the comments from this code review:

## Overall Comments
- The `unused_listener` helper currently binds specifically to `Ipv4Addr::LOCALHOST`; if you expect tests to run in environments that prefer IPv6 or need configurability, consider parameterizing the address or using `TcpListener::bind("127.0.0.1:0")`/`("localhost:0")` to be more flexible.
- To make `CommonTestEnvelope` easier to use across tests, consider adding small helper constructors (e.g., `with_id`, `with_payload`) or encode/decode helpers that hide the raw `Vec<u8>` handling and bincode usage, reducing repetition in test code.

## Individual Comments

### Comment 1
<location> `wireframe_testing/src/integration_helpers.rs:37` </location>
<code_context>
+/// }
+/// ```
+#[must_use]
+pub fn unused_listener() -> std::io::Result<StdTcpListener> {
+    let addr = SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 0);
+    StdTcpListener::bind(addr)
</code_context>

<issue_to_address>
**issue (review_instructions):** Add both unit and behavioural tests covering the new integration_helpers functionality (unused_listener, CommonTestEnvelope, TestApp factory).

Add unit tests in the wireframe_testing crate that exercise `unused_listener` (e.g., ensures it binds to localhost and returns a non-zero port), `CommonTestEnvelope`’s `CorrelatableFrame` and `Packet` implementations (round-tripping via `from_parts`/`into_parts`, correlation ID propagation), and the `factory` fixture (returns a default `TestApp` that can be constructed and used). In addition, add at least one behavioural test that uses these helpers in an integration-style scenario (e.g., spin up a `WireframeServer` via `factory` and `unused_listener` and perform a minimal request/response using `CommonTestEnvelope`) to satisfy the requirement for both behavioural and unit tests for new features.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*`

**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.

</details>
</issue_to_address>

### Comment 2
<location> `wireframe_testing/src/integration_helpers.rs:52` </location>
<code_context>
+    pub id: u32,
+    /// Optional correlation ID for request/response matching.
+    pub correlation_id: Option<u64>,
+    /// Serialised message payload.
+    pub payload: Vec<u8>,
+}
</code_context>

<issue_to_address>
**suggestion (review_instructions):** Field doc comment uses 'Serialised' instead of en-GB-oxendict 'Serialized' (-ize spelling).

The guidance specifies en-GB-oxendict spelling, which prefers -ize forms. Please change this to `Serialized message payload.` to conform.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.rs`

**Instructions:**
Comments must use en-GB-oxendict (-ize / -yse / -our) spelling and grammar.

</details>
</issue_to_address>

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the integration_helpers.rs file to check for helper constructors, tests, and spelling
cat wireframe_testing/src/integration_helpers.rs

Length of output: 5518

---

🏁 Script executed:

#!/bin/bash
# Read the integration test file
cat wireframe_testing/tests/integration_helpers.rs

Length of output: 1909

---

@leynos Yes, all the previously raised comments have been resolved:

Overall Comments

✅ IPv6/flexibility: unused_listener() now binds to "localhost:0" instead of Ipv4Addr::LOCALHOST, providing the flexibility you requested.

✅ Helper constructors: Added both new(id, correlation_id, payload) and with_payload(id, payload) constructors for CommonTestEnvelope, making it easier to use across tests.

Individual Comments

✅ Comment 1 (Tests): All required tests are present:

• Unit tests in wireframe_testing/src/integration_helpers.rs:
  • unused_listener_binds_loopback() validates loopback binding and non-zero port
  • common_test_envelope_round_trips_parts() validates CorrelatableFrame and Packet implementations with round-tripping
  • factory_builds_default_app() validates the factory fixture
• Behavioural test in wireframe_testing/tests/integration_helpers.rs:
  • integration_helpers_round_trip() performs a full integration test spinning up a WireframeServer using factory() and unused_listener(), then executes a request/response cycle with CommonTestEnvelope

✅ Comment 2 (Spelling): Changed from "Serialised" to "Serialized" on line 52, conforming to en-GB-oxendict (-ize) spelling.

All issues are now addressed.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/server/runtime.rs (1)

274-336: ⚠️ Potential issue | 🟡 Minor

Update the accept_loop type-parameter docs to reference AppFactory, not a “factory function”.
Align the Rustdoc with the new trait-based factory abstraction.

🤖 Fix all issues with AI agents

In `@wireframe_testing/tests/integration_helpers.rs`:
- Around line 42-50: The test currently compares response.payload to an
allocated Vec via vec![1, 2, 3], causing an unnecessary allocation; update the
comparison in the function containing response checks (the block that verifies
response.id, response.payload, and response.correlation_id in
wireframe_testing::tests::integration_helpers) to compare against a slice
literal instead (e.g., use & [1, 2, 3] or &[1,2,3] depending on payload type) so
the check uses a borrowable slice and avoids allocating a new Vec.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/server/runtime.rs (2)

388-396: 🧹 Nitpick | 🔵 Trivial

Simplify the clone invocation.

The explicit dereference in (*factory).clone() is unnecessary. When F: Clone, calling .clone() on &F automatically produces an owned F.

♻️ Proposed simplification

         res = listener.accept() => Some(match res {
             Ok((stream, _)) => {
                 spawn_connection_task(
                     stream,
-                    (*factory).clone(),
+                    factory.clone(),
                     handles.preamble.clone(),
                     handles.tracker,
                 );

---

1-6: 🧹 Nitpick | 🔵 Trivial

File length exceeds 400-line limit.

At 406 lines, this file marginally exceeds the 400-line guideline. Decompose in a future refactor—extracting AcceptLoopOptions, AcceptHandles, and PreambleHooks into a separate submodule would reduce complexity.