Publish in-process server-client pair test harness (12.3.2)#515
Merged
Publish in-process server-client pair test harness (12.3.2)#515
Conversation
Contributor
In-process server–client pair test harnessThis PR adds a reusable in‑process Wireframe server–client pair test harness to the wireframe_testing crate, completes roadmap item 12.3.2, and publishes a living execution‑plan at docs/execplans/12-3-2-in-process-server-and-client-pair-test-harness.md. Core functionality
Lifecycle guarantees and behaviour fixes
Code‑quality note & reviewer discussion
Supporting test helpers
Tests & BDD coverage
Documentation & execplan
Cargo / dependency change
Public API exports
Notable implementation details for reviewers
Review guidancePlease focus review on:
WalkthroughSummarise a new in-process test harness that spawns a loopback-TCP Changes
Sequence DiagramsequenceDiagram
participant Test as Test Code
participant Harness as spawn_wireframe_pair()
participant Listener as unused_listener()
participant Server as WireframeServer
participant TCP as Loopback TCP
participant Client as WireframeClient
Test->>Harness: Call spawn_wireframe_pair(app_factory, config)
Harness->>Listener: Reserve loopback address
Harness->>Server: bind_existing_listener(listener)
Server->>Server: Spawn server task
Server-->>Harness: Signal readiness via oneshot (rgba(0,128,0,0.5))
Harness->>Harness: Build client with config
Harness->>Client: Connect to server addr
Client->>TCP: Open loopback connection
TCP->>Server: Deliver connection
Harness->>Test: Return WireframePair
Test->>Client: client_mut().call(request)
Client->>TCP: Send request
TCP->>Server: Receive request
Server->>Server: Invoke app handler (echo)
Server->>TCP: Send response
TCP->>Client: Deliver response
Client->>Test: Return response
Test->>Harness: Call pair.shutdown()
Harness->>Server: Send shutdown signal
Server->>Server: Drain and stop task
Harness->>Test: Shutdown complete
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideImplements a new in-process Wireframe server/client pair harness in the wireframe_testing crate, exposes a minimal public API (WireframePair and spawn helpers), and validates it via rstest integration tests, rstest-bdd scenarios, and documentation updates tied to roadmap item 12.3.2. Sequence diagram for spawn_wireframe_pair server-client startup lifecyclesequenceDiagram
actor TestAuthor
participant TestCode
participant client_pair_module
participant unused_listener
participant WireframeServer
participant TokioRuntime
participant WireframeClientBuilder
participant WireframeClient
TestAuthor->>TestCode: write async test
TestCode->>client_pair_module: spawn_wireframe_pair(app_factory, configure_client)
client_pair_module->>unused_listener: unused_listener()
unused_listener-->>client_pair_module: TcpListener
client_pair_module->>WireframeServer: new(app_factory)
WireframeServer-->>client_pair_module: WireframeServer
client_pair_module->>WireframeServer: workers(1)
WireframeServer-->>client_pair_module: WireframeServer
client_pair_module->>WireframeServer: bind_existing_listener(TcpListener)
WireframeServer-->>client_pair_module: WireframeServer
client_pair_module->>WireframeServer: local_addr()
WireframeServer-->>client_pair_module: SocketAddr
client_pair_module->>TokioRuntime: spawn(server.run_with_shutdown(...))
TokioRuntime-->>client_pair_module: JoinHandle
client_pair_module->>WireframeServer: ready_signal(ready_tx)
WireframeServer-->>TokioRuntime: signal ready when bound
client_pair_module->>TokioRuntime: await ready_rx
TokioRuntime-->>client_pair_module: ready
client_pair_module->>WireframeClientBuilder: new()
WireframeClientBuilder-->>client_pair_module: WireframeClientBuilder
client_pair_module->>WireframeClientBuilder: configure_client(builder)
WireframeClientBuilder-->>client_pair_module: WireframeClientBuilder
client_pair_module->>WireframeClientBuilder: connect(SocketAddr)
WireframeClientBuilder-->>client_pair_module: WireframeClient
client_pair_module-->>TestCode: WireframePair
TestCode->>WireframePair: client_mut()
WireframePair-->>TestCode: &mut WireframeClient
TestCode->>WireframePair: shutdown().await
WireframePair->>WireframeClient: drop
WireframePair->>TokioRuntime: send shutdown signal
TokioRuntime-->>WireframePair: server task completes
WireframePair-->>TestCode: Ok
Class diagram for the new WireframePair harness APIclassDiagram
class WireframePair {
- WireframeClient client
- SocketAddr addr
- Sender shutdown_tx
- JoinHandle handle
+ client_mut() WireframeClient
+ local_addr() SocketAddr
+ shutdown() TestResult
}
class WireframeClient {
}
class WireframeClientBuilder {
+ new() WireframeClientBuilder
+ connect(addr SocketAddr) WireframeClient
}
class WireframeServer {
+ new(app_factory AppFactory) WireframeServer
+ workers(count int) WireframeServer
+ bind_existing_listener(listener TcpListener) WireframeServer
+ local_addr() SocketAddr
+ ready_signal(sender Sender) WireframeServer
+ run_with_shutdown(shutdown_future Future) Result
}
class AppFactory {
}
class TestResult {
}
class TestError {
}
class Sender {
+ send(value void) void
}
class JoinHandle {
+ await() Result
+ abort_handle() AbortHandle
}
class AbortHandle {
+ abort() void
}
class TcpListener {
}
class TcpStream {
}
class BincodeSerializer {
}
class RewindStream {
}
class FrameCodec {
}
WireframePair "1" o-- "1" WireframeClient : holds
WireframePair "1" o-- "1" Sender : shutdown_tx
WireframePair "1" o-- "1" JoinHandle : handle
WireframeClientBuilder ..> WireframeClient : builds
WireframeServer ..> AppFactory : uses
WireframeServer ..> TcpListener : binds
WireframeClient ..> TcpStream : uses
WireframeClient ..> BincodeSerializer : serializes
WireframeClient ..> RewindStream : wraps
RewindStream ..> TcpStream : wraps
WireframeServer ..> FrameCodec : uses
TestResult ..> TestError : wraps
class client_pair_module {
+ spawn_wireframe_pair(app_factory AppFactory, configure_client ConfigureClientFn) TestResult
+ spawn_wireframe_pair_default(app_factory AppFactory) TestResult
}
client_pair_module ..> WireframePair : returns
client_pair_module ..> WireframeServer : starts
client_pair_module ..> WireframeClientBuilder : configures
client_pair_module ..> Sender : creates_channels
client_pair_module ..> JoinHandle : spawns_task
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Introduce a reusable public test harness in wireframe_testing that starts a real Wireframe server and a connected client within the same test process using real loopback TCP sockets. This harness aims to reduce duplication, lifecycle bugs, and complexity in client-facing tests by providing a clean, generic API for bounded server/client orchestration. It includes support for client builder customization, explicit shutdown handling, and integrates with rstest and rstest-bdd for coverage. This change also updates design docs, user guides, and roadmap to document this new validation milestone (12.3.2), ensuring downstream crates can leverage the harness for protocol integration and compatibility tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
devboxerhub
Bot
force-pushed
the
feat-inprocess-server-client-test-ag7dz7
branch
from
a35f755 to
20c4f5f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add WireframePair, spawn_wireframe_pair, and spawn_wireframe_pair_default to wireframe_testing, providing a reusable loopback TCP harness that starts a real server and connects a client within the same test process. This eliminates lifecycle boilerplate and address-race bugs in client-facing integration tests. - Stage A+B: client_pair module in wireframe_testing with WireframePair struct, spawn functions, graceful shutdown, and Drop safety net - Stage C: four rstest integration tests covering round-trip, custom max frame length, explicit shutdown, and drop-without-shutdown - Stage D: two rstest-bdd scenarios with full fixture/steps/scenario wiring for default and custom-configuration round trips - Stage E: updated wireframe-client-design, wireframe-testing-crate, users-guide, and roadmap documentation; exec plan marked COMPLETE - Stage F: all quality gates pass (check-fmt, lint, test, markdownlint, nixie) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
WireframePair::client_mutAPI panics after shutdown, which is surprising for a test harness; consider returning aResultorOptionhere so misuse is surfaced at compile time or via explicit error handling instead of a runtime panic. - The
Dropimplementation forWireframePaircreates a new OS thread that sleeps for 5 seconds before aborting, which is quite heavy for a safety net; you might want to use an async timeout on the existing Tokio runtime (or a shorter delay) to avoid unbounded thread accumulation in large test suites. - The echo-app setup (Echo type, handler, and factory) is duplicated between
tests/client_pair_harness.rsandtests/fixtures/client_pair_harness.rs; consider extracting this into a shared helper to keep the harness behaviour and routing logic in sync across tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `WireframePair::client_mut` API panics after shutdown, which is surprising for a test harness; consider returning a `Result` or `Option` here so misuse is surfaced at compile time or via explicit error handling instead of a runtime panic.
- The `Drop` implementation for `WireframePair` creates a new OS thread that sleeps for 5 seconds before aborting, which is quite heavy for a safety net; you might want to use an async timeout on the existing Tokio runtime (or a shorter delay) to avoid unbounded thread accumulation in large test suites.
- The echo-app setup (Echo type, handler, and factory) is duplicated between `tests/client_pair_harness.rs` and `tests/fixtures/client_pair_harness.rs`; consider extracting this into a shared helper to keep the harness behaviour and routing logic in sync across tests.
## Individual Comments
### Comment 1
<location path="wireframe_testing/src/client_pair.rs" line_range="66" />
<code_context>
+///
+/// [`WireframeServer`]: wireframe::server::WireframeServer
+/// [`WireframeClient`]: wireframe::client::WireframeClient
+pub struct WireframePair {
+ client: Option<WireframeClient<BincodeSerializer, RewindStream<tokio::net::TcpStream>, ()>>,
+ addr: SocketAddr,
</code_context>
<issue_to_address>
**issue (complexity):** Consider collapsing the per-field Options into a single Running state and simplifying shutdown/Drop so the lifecycle reads linearly and avoids extra threads or implicit invariants.
You can simplify the lifecycle quite a bit by collapsing the per-field `Option`s into a single “running” state, and by making `Drop` abort immediately after signalling shutdown instead of spawning a second thread.
### 1. Replace per-field `Option`s with a single `Running` state
This removes the need to juggle multiple `Option`s and the implicit invariants between them:
```rust
struct Running {
client: WireframeClient<BincodeSerializer, RewindStream<tokio::net::TcpStream>, ()>,
shutdown_tx: oneshot::Sender<()>,
handle: JoinHandle<Result<(), wireframe::server::ServerError>>,
}
pub struct WireframePair {
addr: SocketAddr,
running: Option<Running>,
}
```
Construction becomes simpler:
```rust
Ok(WireframePair {
addr,
running: Some(Running {
client,
shutdown_tx,
handle,
}),
})
```
`client_mut` then enforces the invariant at the type level:
```rust
#[expect(
clippy::expect_used,
reason = "intentional panic for post-shutdown misuse"
)]
pub fn client_mut(
&mut self,
) -> &mut WireframeClient<BincodeSerializer, RewindStream<tokio::net::TcpStream>, ()> {
self.running
.as_mut()
.map(|r| &mut r.client)
.expect("client_mut called after shutdown")
}
```
### 2. Simplify `shutdown` and error plumbing
With `Running` encapsulating state, `shutdown` reads linearly and separates join errors from server errors:
```rust
pub async fn shutdown(&mut self) -> TestResult<()> {
if let Some(Running { client, shutdown_tx, handle }) = self.running.take() {
drop(client); // ensure server sees disconnect
let _ = shutdown_tx.send(());
match handle.await {
Err(join_err) => {
return Err(TestError::Msg(format!("server task join error: {join_err}")));
}
Ok(Err(server_err)) => {
return Err(TestError::Msg(format!("server error: {server_err}")));
}
Ok(Ok(())) => {}
}
}
Ok(())
}
```
This avoids `??` and the stringly-typed error path is now explicit and easier to scan.
### 3. Make `Drop` minimal and avoid extra threads
You can still send a shutdown signal in `Drop` and ensure the task is not leaked, without spawning a dedicated OS thread per pair. For a test harness, an immediate abort after signalling is typically sufficient:
```rust
impl Drop for WireframePair {
fn drop(&mut self) {
if let Some(Running { shutdown_tx, handle, .. }) = self.running.take() {
let _ = shutdown_tx.send(());
handle.abort(); // immediate safety net to avoid leaked tasks
}
}
}
```
This keeps the “safety net” behavior (no orphaned tasks) but removes:
- the extra `std::thread::spawn` per pair,
- the 5s timing wrinkle in `Drop`,
- cross-runtime interaction between `std::thread` and `tokio::JoinHandle`.
If you still want a timeout-based graceful path, you could document that callers should use `shutdown()` for that, and keep `Drop` as the minimal safety net above.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e032ce5880
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/fixtures/client_pair_harness.rs`:
- Around line 20-21: Duplicate definition of the test payload type `Echo(u8)`
exists; extract the `Echo` struct into a shared test utilities module (e.g.,
tests::util or a new tests/common.rs) and replace the local `struct Echo(u8);`
in both `tests/fixtures/client_pair_harness.rs` and
`tests/client_pair_harness.rs` with a use/import of that shared definition;
ensure the shared module exposes the same traits/derives (bincode::Encode,
bincode::BorrowDecode, PartialEq, Debug) so existing tests compile unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9578c7db-7ddb-4c7e-a464-b6ba14d2a13f
📒 Files selected for processing (16)
docs/execplans/12-3-2-in-process-server-and-client-pair-test-harness.mddocs/roadmap.mddocs/users-guide.mddocs/wireframe-client-design.mddocs/wireframe-testing-crate.mdtests/client_pair_harness.rstests/features/client_pair_harness.featuretests/fixtures/client_pair_harness.rstests/fixtures/mod.rstests/scenarios/client_pair_harness_scenarios.rstests/scenarios/mod.rstests/steps/client_pair_harness_steps.rstests/steps/mod.rswireframe_testing/Cargo.tomlwireframe_testing/src/client_pair.rswireframe_testing/src/lib.rs
Address three review concerns in the in-process server/client pair test harness: 1. Collapse per-field Options into a single Running struct, simplifying shutdown logic and reducing implicit invariants between fields. 2. Replace the OS-thread-based Drop safety net (5-second sleep before abort) with an immediate handle.abort() after signalling shutdown. Callers wanting graceful teardown should call shutdown() explicitly. 3. Clean up the spawned server task when the client connection fails in spawn_wireframe_pair, preventing leaked tasks and bound listeners. Additionally, extract the duplicated echo app setup into a shared echo_app_factory and echo_handler in wireframe_testing, keeping the routing logic in sync across the rstest integration tests and the BDD fixture. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move the duplicated `Echo(u8)` struct from `tests/client_pair_harness.rs` and `tests/fixtures/client_pair_harness.rs` into `wireframe_testing::integration_helpers` alongside `CommonTestEnvelope`. Both test files now import the shared definition, keeping the payload type and its derives in sync across the rstest and BDD suites. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/wireframe-client-design.md`:
- Around line 132-134: The doc currently implies spawn_wireframe_pair accepts an
optional client-builder parameter; update the wording to match the actual
signature by stating spawn_wireframe_pair takes a configuration closure (for
customizing the client) and add a note directing callers who want the
no-customization path to use spawn_wireframe_pair_default; reference
spawn_wireframe_pair and spawn_wireframe_pair_default and keep the surrounding
mention of unused_listener unchanged.
In `@wireframe_testing/src/client_pair.rs`:
- Around line 96-107: Change client_mut to return a TestResult<&mut
WireframeClient<BincodeSerializer, RewindStream<tokio::net::TcpStream>, ()>>
instead of panicking: remove the #[expect(...)] suppression and the
.expect("client_mut called after shutdown") call, and replace it with an ok_or /
match that returns an Err(TestResult::Err(...) or the crate's TestError via
TestResult) when self.running is None. Keep the rest of the method body
returning the mutable client reference on Ok, and update any callers to
propagate the TestResult instead of assuming a panic path (follow the pattern
used in WireframePair::shutdown()).
In `@wireframe_testing/src/integration_helpers.rs`:
- Around line 249-261: The helper echo_app_factory currently panics via
.expect("failed to build echo app"); change it to return a fallible result
instead: remove the clippy::expect_used suppression, make echo_app_factory
produce an impl Fn() -> Result<CommonTestApp, TestError> (or the existing
TestResult type) and propagate errors using ? on CommonTestApp::new() and
app.route(...) so the closure returns Err on failure rather than calling
.expect; update callers to handle the Result accordingly so panics are
eliminated from this shared fixture.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b39c032-6036-4799-85c6-8fab7dfa8599
📒 Files selected for processing (9)
docs/execplans/12-3-2-in-process-server-and-client-pair-test-harness.mddocs/users-guide.mddocs/wireframe-client-design.mddocs/wireframe-testing-crate.mdtests/client_pair_harness.rstests/fixtures/client_pair_harness.rswireframe_testing/src/client_pair.rswireframe_testing/src/integration_helpers.rswireframe_testing/src/lib.rs
Replace the two remaining panic paths in the pair harness with fallible returns: - client_mut() now returns TestResult<&mut WireframeClient<...>> instead of panicking when called after shutdown. Callers propagate the error with ?. - echo_app_factory() returns Fn() -> TestResult<CommonTestApp> instead of a closure that calls .expect(). The AppFactory blanket impl already accepts Fn() -> Result<App, Err> so callers need no changes. Also fix the wireframe-client-design.md wording that incorrectly described the configure_client parameter as optional; callers wanting no customization should use spawn_wireframe_pair_default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/client_pair_harness.rs`:
- Around line 56-74: The test custom_max_frame_length_via_pair_harness is
ineffective because it uses Echo(99) which fits the default frame limit; change
it to send a payload that exceeds the default client frame limit so the
builder.max_frame_length(2048) override is required to pass: in the
custom_max_frame_length_via_pair_harness function (and where Echo is
constructed/sized) create a larger payload whose serialized length is greater
than the default max frame length (or otherwise exercise a condition that would
be rejected by the default configuration), send that payload via
pair.client_mut()?.call(&request).await? and assert the response succeeds (or
assert the expected failure only when the override is missing) so the test
actually verifies spawn_wireframe_pair + builder.max_frame_length(2048) took
effect.
- Around line 81-97: The test explicit_shutdown_completes_cleanly should verify
the post-shutdown client_mut() contract: after calling pair.shutdown().await
assert that pair.client_mut() returns an Err (i.e. non-panicking error result)
to guard the new non-panicking path; update the test to stop after
pair.shutdown().await and add a short assertion checking pair.client_mut()
yields Err, referencing the explicit_shutdown_completes_cleanly test and the
pair.shutdown().await / pair.client_mut() calls.
In `@wireframe_testing/src/client_pair.rs`:
- Around line 121-131: In shutdown(), instead of immediately dropping the client
field from the Running struct, call the client's async close path
(WireframeClient::close().await) so any teardown hooks configured via
configure_client run, then drop the client and send shutdown_tx, and finally
await the server task handle; ensure you await and handle/propagate any error
returned by WireframeClient::close() before proceeding with shutdown of the
server task (running.take(), client, shutdown_tx, handle).
- Around line 150-160: In both the Drop implementation for WireframePair and the
abort_server function, stop immediately calling handle.abort() after sending
shutdown; instead send the shutdown signal via shutdown_tx.send(()), then await
the task's completion (join on the task handle returned from Running.handle)
with a bounded timeout, and only call handle.abort() if the join times out or
the task panics; update the teardown flow so run_with_shutdown() can run
tracker.close() and await tracker.wait().await (which tracker.spawn()’d
accept/connection tasks rely on) before force-aborting the handle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65e3c3ac-4b25-48be-ab5c-99d793312fb3
📒 Files selected for processing (8)
docs/execplans/12-3-2-in-process-server-and-client-pair-test-harness.mddocs/users-guide.mddocs/wireframe-client-design.mddocs/wireframe-testing-crate.mdtests/client_pair_harness.rstests/fixtures/client_pair_harness.rswireframe_testing/src/client_pair.rswireframe_testing/src/integration_helpers.rs
Improve the WireframePair harness shutdown behaviour and strengthen test coverage in response to inline code review feedback. Changes to WireframePair lifecycle: - Call client.close().await in shutdown() before dropping the client so configured teardown hooks (on_disconnect) run during explicit shutdown - Update Drop and abort_server to await task completion with a bounded timeout (100ms) before force-aborting; spawn timeout logic on a separate task to avoid blocking from within a runtime - Use tokio::select! to race task join against timeout, aborting only if the task does not complete in time, allowing tracker.close() and tracker.wait() to finish Test improvements: - Refactor custom_max_frame_length_via_pair_harness to send a 5000-byte payload that actually exceeds the default 4096-byte frame limit, verifying that both server (via buffer_capacity) and client (via max_frame_length) configuration take effect - Add assertion in explicit_shutdown_completes_cleanly that client_mut() returns Err after shutdown, guarding the new non-panicking contract All tests, lint, and format checks pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wireframe_testing/src/client_pair.rs`:
- Around line 150-186: The current Drop for WireframePair calls
tokio::runtime::Handle::try_current() inside a newly spawned OS thread which
always returns Err; instead capture the runtime handle on the current thread
before spawning and move that Option<Handle> into the closure so the spawned
thread can use it. Concretely, call tokio::runtime::Handle::try_current() (or
Handle::current()/clone when appropriate) in the Drop block just after taking
self.running, store the result (Option<Handle>) and move it into the
std::thread::spawn closure; inside the closure check that Option and call
runtime.spawn(...) when Some(handle) and fallback to handle.abort() when None.
This preserves the 100ms grace period while keeping references to shutdown_tx
and handle consistent with the Running struct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2e88831-e27a-40b6-a5cd-0d301e050c17
📒 Files selected for processing (2)
tests/client_pair_harness.rswireframe_testing/src/client_pair.rs
Capture the tokio runtime handle on the current thread before spawning OS threads in WireframePair::Drop and abort_server, since Handle::try_current() always returns Err when called from within a newly spawned OS thread that has no tokio context. Changes: - In Drop, call Handle::try_current().ok() before std::thread::spawn and move the Option<Handle> into the closure - In abort_server, apply the same pattern for consistency even though it is called from async context where try_current() would work - Update comments to clarify why we capture before spawning This preserves the 100ms grace period for tracker.close() and tracker.wait() while ensuring the runtime handle is available inside the spawned thread. All tests, lint, and format checks pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The previous Drop implementation wrapped cleanup in std::thread::spawn, which always failed to find a tokio runtime context (try_current() returned Err) and immediately aborted the task, defeating the 100ms grace period design. Fix by calling tokio::runtime::Handle::try_current() directly on the dropping thread (where the tokio context is available). When successful, spawn the timeout task on that runtime; when Err, fall through to immediate abort. This mirrors the abort_server implementation exactly. Also update the WireframePair doc comment to reflect the bounded-wait behaviour instead of stating it "immediately aborts". All tests, lint, and format checks pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wireframe_testing/src/client_pair.rs`:
- Around line 183-215: Both the Drop implementation for the server pair and the
abort_server function duplicate the same bounded-timeout shutdown logic; extract
that logic into a single helper (e.g., spawn_bounded_shutdown or similar) that
accepts the JoinHandle<Result<(), wireframe::server::ServerError>> and a
Duration (or timeout in ms) and performs the current behavior of trying to get
the current tokio runtime handle, spawning an async task that awaits the handle
with a tokio::select! against sleep, and aborts the JoinHandle on timeout (or
immediately aborts if no runtime). Replace the duplicated blocks in abort_server
and the Drop impl to call this new helper so the timeout value and abort
behavior are centralized and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a8806b5-a018-4db2-97a7-b620ecbc58a0
📒 Files selected for processing (1)
wireframe_testing/src/client_pair.rs
Both Drop for WireframePair and abort_server duplicated the same bounded-timeout shutdown logic. Extract into spawn_bounded_shutdown helper that centralizes the timeout value and abort behavior. Changes: - Add spawn_bounded_shutdown(handle, timeout) helper that: - Tries to get the current tokio runtime handle - Spawns an async task racing join against timeout via tokio::select! - Aborts only on timeout or immediately if no runtime - Update Drop to call spawn_bounded_shutdown with 100ms timeout - Update abort_server to call spawn_bounded_shutdown with 100ms timeout - Remove duplicated timeout logic from both call sites This ensures consistent shutdown behavior and makes the timeout value easy to adjust in one place if needed. All tests, lint, and format checks pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wireframe_testing/src/client_pair.rs`:
- Around line 121-147: The shutdown() currently does self.running.take() before
any await which drops Running if the future is cancelled; instead keep Running
in self until cleanup finishes: obtain a mutable reference to the Running inside
self (e.g., via self.running.as_mut() or similar) so you can call
running.client.close().await, send on running.shutdown_tx, await running.handle,
and only after all awaits succeed or errors are handled set self.running = None
(or call Option::take() at the final step); ensure you still return appropriate
TestError variants for join/ server errors and preserve the existing
shutdown_tx.send() and handle.await logic using the Running symbols (client,
shutdown_tx, handle).
- Around line 270-303: The spawn leaves shutdown_tx and handle unguarded so
panics or readiness failures leak the server task; wrap them in a RAII guard
(e.g., struct PendingServer(Option<(oneshot::Sender<()>, JoinHandle<Result<(),
wireframe::server::ServerError>>)>)) whose Drop calls abort_server(shutdown_tx,
handle), assign PendingServer(Some((shutdown_tx, handle))) after tokio::spawn
where server.ready_signal(ready_tx).run_with_shutdown(...) is spawned, replace
ready_rx.await.map_err(...) with a match that on Err takes the pending handle
and .awaits it to surface join or ServerError (returning a descriptive
TestError), on Ok take the pending pair out before calling
configure_client(WireframeClientBuilder::new()) so client.connect(addr).await
Err no longer calls abort_server directly (the Drop guard will handle abort on
unwinding).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0540786a-ec10-4e29-abc2-0fcce9ea43c0
📒 Files selected for processing (1)
wireframe_testing/src/client_pair.rs
Address two correctness issues in WireframePair lifecycle management:
1. shutdown() now keeps Running in self until all awaits complete
- Changed Running fields to Option so they can be taken individually
- Use running.as_mut() to borrow, take() fields one by one
- Only set self.running = None after all awaits succeed/fail
- If the shutdown future is cancelled mid-flight, Drop can still
clean up the remaining Running state
2. spawn_wireframe_pair uses PendingServer RAII guard
- Wrap (shutdown_tx, handle) in PendingServer guard immediately
after tokio::spawn
- Guard's Drop calls spawn_bounded_shutdown if not taken
- Eliminates manual abort_server call in error path
- Panics or early returns now automatically clean up via Drop
- Take the pending pair out before client.connect so Drop doesn't
fire on normal success path
Changes to Running struct:
- All fields now Option<T> instead of T
- Updated client_mut to use .and_then(|r| r.client.as_mut())
- Updated Drop to unwrap Options before cleanup
- Updated spawn_wireframe_pair to construct with Some(...) wrappers
All tests, lint, and format checks pass.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. wireframe_testing/src/client_pair.rs Comment on file pub async fn shutdown(&mut self) -> TestResult<()> {
if let Some(running) = self.running.as_mut() {
// Take the client and close it.
if let Some(client) = running.client.take() {
client.close().await;
}
// Send the shutdown signal.
if let Some(shutdown_tx) = running.shutdown_tx.take() {
let _ = shutdown_tx.send(());
}
// Await the server task.
if let Some(handle) = running.handle.take() {
match handle.await {
Err(join_err) => {
// Clean up Running before returning error.
self.running = None;
return Err(TestError::Msg(format!(
"server task join error: {join_err}"
)));
}
Ok(Err(server_err)) => {
// Clean up Running before returning error.
self.running = None;
return Err(TestError::Msg(format!("server error: {server_err}")));
}
Ok(Ok(())) => {
// Success - clean up Running.
self.running = None;
}
}
}
}
Ok(())
}❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wireframe_testing/src/client_pair.rs (1)
254-256:⚠️ Potential issue | 🟠 MajorKeep
PendingServerarmed until client construction succeeds.Lines 254-256 promise that connect failures tear the server down before returning, but Line 326 disarms the RAII guard before
configure_clientandconnect. Any error or panic after that point drops plain locals instead of going through the bounded cleanup path, andready_rx.await.map_err(...)also hides the real start-up failure. Hold the guard until afterbuilder.connect(addr).awaitsucceeds, join the task on readiness failure to surface the actual server error, and replace the.expect(...)handoff with a propagatedTestError. As per coding guidelines: ".expect()and.unwrap()are forbidden outside of tests. Errors must be propagated."🔧 Suggested fix
- ready_rx - .await - .map_err(|_| TestError::Msg("server did not signal ready".into()))?; - - // Take the pending server out of the guard before connecting the client. - let (shutdown_tx, handle) = pending.take().expect("pending server already taken"); - - let builder = configure_client(WireframeClientBuilder::new()); - let client = builder.connect(addr).await?; + match ready_rx.await { + Ok(()) => {} + Err(_) => { + let Some((_, handle)) = pending.take() else { + return Err(TestError::Msg("pending server state missing".into())); + }; + return match handle.await { + Err(join_err) => Err(TestError::Msg(format!( + "server task failed before readiness: {join_err}" + ))), + Ok(Err(server_err)) => Err(TestError::Msg(format!( + "server failed before readiness: {server_err}" + ))), + Ok(Ok(())) => Err(TestError::Msg( + "server exited before signalling readiness".into(), + )), + }; + } + } + + let builder = configure_client(WireframeClientBuilder::new()); + let client = builder.connect(addr).await?; + let Some((shutdown_tx, handle)) = pending.take() else { + return Err(TestError::Msg("pending server state missing".into())); + };Also applies to: 320-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wireframe_testing/src/client_pair.rs` around lines 254 - 256, The RAII guard PendingServer is being disarmed too early: keep the PendingServer guard alive until client construction completes successfully by moving the guard drop to after builder.connect(addr).await succeeds; call configure_client and await builder.connect while the guard is still held, and on ready_rx.await failure join the server task to surface the real startup error instead of hiding it—replace the .expect(...) handoff with returning/propagating a TestError so errors are propagated rather than unwrapped. Ensure references to PendingServer, configure_client, builder.connect, ready_rx, and TestError are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wireframe_testing/src/client_pair.rs`:
- Around line 116-119: The shutdown() implementation currently takes
running.handle out of self before awaiting the join, which breaks the
cancellation-safe guarantee for Running/Drop; instead, keep the server handle in
self while awaiting by calling join via running.handle.as_mut().await (or create
a temporary guard that borrows the handle) and only clear/set running.handle =
None after the join completes; update the code paths in shutdown() (the block
around where running.handle is taken and the join is awaited, covering the
region corresponding to the existing 137-155 logic) to await through a mutable
borrow and clear the field afterward so Drop still observes the handle if
shutdown is cancelled.
---
Duplicate comments:
In `@wireframe_testing/src/client_pair.rs`:
- Around line 254-256: The RAII guard PendingServer is being disarmed too early:
keep the PendingServer guard alive until client construction completes
successfully by moving the guard drop to after builder.connect(addr).await
succeeds; call configure_client and await builder.connect while the guard is
still held, and on ready_rx.await failure join the server task to surface the
real startup error instead of hiding it—replace the .expect(...) handoff with
returning/propagating a TestError so errors are propagated rather than
unwrapped. Ensure references to PendingServer, configure_client,
builder.connect, ready_rx, and TestError are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ff28157-bef4-4600-aac5-9c058aba6f88
📒 Files selected for processing (1)
wireframe_testing/src/client_pair.rs
Restore cancellation safety to shutdown() and fix PendingServer guard lifetime to prevent server task leaks. Changes: - shutdown(): use self.running.as_mut() instead of take() and await handle via as_mut() so Running stays in self until all awaits complete; only clear self.running after handle.await finishes - spawn_wireframe_pair: keep PendingServer guard alive until after builder.connect succeeds so guard cleans up if connection fails Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wireframe_testing/src/client_pair.rs (1)
317-319:⚠️ Potential issue | 🟠 MajorPropagate the server’s startup failure instead of collapsing it into a generic readiness error.
Join the spawned server task when
ready_rx.awaitreturnsErr. The current branch dropspending, which tears the task down but discards the actualJoinErrororServerError, so a pre-readiness crash is reported only as “server did not signal ready”.🩹 Proposed fix
- ready_rx - .await - .map_err(|_| TestError::Msg("server did not signal ready".into()))?; + match ready_rx.await { + Ok(()) => {} + Err(_) => { + let Some((_, handle)) = pending.take() else { + return Err(TestError::Msg("pending server state missing".into())); + }; + return match handle.await { + Err(join_err) => Err(TestError::Msg(format!( + "server task failed before readiness: {join_err}" + ))), + Ok(Err(server_err)) => Err(TestError::Msg(format!( + "server failed before readiness: {server_err}" + ))), + Ok(Ok(())) => Err(TestError::Msg( + "server exited before signalling readiness".into(), + )), + }; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wireframe_testing/src/client_pair.rs` around lines 317 - 319, When ready_rx.await returns Err, don't collapse the failure into TestError::Msg; instead await the spawned server task (the join handle named pending) to retrieve its JoinError or the inner ServerError and propagate that as the TestError (or wrap with a descriptive message). Replace the current map_err branch that drops pending with logic that .awaits pending (or .join().await) to capture the actual failure and convert it into a TestError variant (preserving error details) rather than always returning "server did not signal ready".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wireframe_testing/src/client_pair.rs`:
- Around line 325-326: The code currently calls pending.take().expect("pending
server already taken") which panics; replace this with proper error propagation
by making the surrounding function return a Result (or use the function's
existing Result) and map the None case to an appropriate error type/value
instead of panicking; locate the hand-off where pending.take() is used (the let
(shutdown_tx, handle) = pending.take().expect(...) line) and change it to
destructure via match or .ok_or_else(...) so you can return Err(...) when
pending is None, preserving the shutdown_tx and handle variable names for
downstream logic.
---
Duplicate comments:
In `@wireframe_testing/src/client_pair.rs`:
- Around line 317-319: When ready_rx.await returns Err, don't collapse the
failure into TestError::Msg; instead await the spawned server task (the join
handle named pending) to retrieve its JoinError or the inner ServerError and
propagate that as the TestError (or wrap with a descriptive message). Replace
the current map_err branch that drops pending with logic that .awaits pending
(or .join().await) to capture the actual failure and convert it into a TestError
variant (preserving error details) rather than always returning "server did not
signal ready".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31d25bed-fd61-4a8b-a68f-9c03a3ac8f6d
📒 Files selected for processing (1)
wireframe_testing/src/client_pair.rs
Improve error handling in spawn_wireframe_pair by eliminating panic paths and surfacing actual server startup errors to callers. Changes: - Replace pending.take().expect(...) with .ok_or_else(...) for proper error propagation when pending server is already taken - When ready_rx.await fails, await the server task handle to capture the actual startup error (JoinError or ServerError) instead of returning generic "server did not signal ready" message - Distinguish three failure modes: task panic/cancel, server error, and clean exit before ready signal Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wireframe_testing/src/client_pair.rs`:
- Around line 317-333: Update the American spelling "signaling" to British
"signalling" in this block: change the inline comment that reads "Server dropped
ready_tx without signaling - join the task to get the real error." to use
"signalling", update the error message returned when Ok(Ok(())) which currently
says "server exited before signaling ready" to "server exited before signalling
ready", and update any other string literal in this snippet (e.g., any logging
or TestError::Msg text) that uses "signaling" so all occurrences match
en-GB-oxendict spelling; keep the same punctuation and message semantics, only
adjust spelling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba6c73b0-1610-47b0-93fe-d830b2def76b
📒 Files selected for processing (1)
wireframe_testing/src/client_pair.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
What this change covers
Implementation stages (high level)
Risks and considerations
Progress status
Documentation impact
+- New: docs/execplans/12-3-2-in-process-server-and-client-pair-test-harness.md describing the ExecPlan for this milestone.
Review guidance
📎 Task: https://www.devboxer.com/task/1ee16d32-1205-4834-b001-ca47396977ba
Summary by Sourcery
Introduce a reusable in-process Wireframe server/client pair harness in the wireframe_testing crate and validate it with integration and BDD tests while updating documentation and roadmap for roadmap item 12.3.2.
New Features:
Enhancements:
Tests: