Make integration test assertions fallible

docs/developers-guide.md

@@ -1281,6 +1281,51 @@ client that finishes its work and drops its connection produces exactly one of
 those kinds. Every other I/O error is returned from the task instead, so it
 surfaces when the caller joins the handle.
 
+### Fallible assertions in integration tests
+
+Integration tests whose bodies return `Result` cannot use `assert_eq!` and
+friends without tripping `clippy::panic_in_result_fn`, which flags a panic
+inside a function that promises to report failure by returning `Err`. The
+`tests/common/fallible_assertions/` modules provide three small replacement
+checks. Each returns `Result<(), String>`: `Ok(())` when the check holds, and
+`Err` carrying the predicate-specific message when it does not. A test
+propagates a failure with `?` rather than panicking, so the diagnostic reaches
+the harness as an ordinary test failure.
+
+The three checks divide by what they compare:
+
+- `check` (in `check.rs`) takes a boolean predicate and the message to report
+  when it is false. Use it for conditions that are not plain equality, such as
+  a collection containing an expected element or a value lying in a range.
+- `check_eq` (in `check_eq.rs`) compares two `Copy` values and reports
+  `"{message}: expected {expected:?}, got {actual:?}"`. Use it for small scalar
+  values, such as a connection count or an expected byte.
+- `check_equal` (in `check_equal.rs`) compares two borrowed, possibly unsized
+  values (`A: PartialEq<E> + ?Sized`, `E: ?Sized`) and reports the same
+  expected/actual shape. Use it for string slices, structs compared by
+  reference, and other non-`Copy` values.
+
+Each helper lives in its own module because each integration-test crate pulls
+in only the checks it uses, with an explicit path attribute:
+
+```rust
+#[path = "common/fallible_assertions/check_equal.rs"]
+mod fallible_check_equal;
+
+use fallible_check_equal::check_equal;
+```
+
+The helpers are declared `pub(super)`, which resolves to crate visibility
+because the test file is the crate root. That is why the `#[path]` attribute
+must appear in the test file itself rather than inside a submodule: a
+declaration inside a submodule would be visible only to that module, not to the
+sibling test modules that need it.
+
+These helpers exist only for integration tests. Production code and
+non-`Result` unit tests continue to use the standard assertion macros, whose
+panic-on-failure behaviour is exactly right when the test has no result channel
+to report through.
+
 ### `LoggerHandle::Default` and `ObservabilityHandle::Default`
 
 Both `LoggerHandle` (in `wireframe_testing::logging`) and `ObservabilityHandle`

tests/client_lifecycle.rs

@@ -10,33 +10,42 @@
 
 use std::sync::{
     Arc,
-    atomic::{AtomicUsize, Ordering},
+    atomic::{AtomicU32, AtomicUsize, Ordering},
 };
 
+#[path = "common/fallible_assertions/check.rs"]
+mod fallible_check;
+#[path = "common/fallible_assertions/check_eq.rs"]
+mod fallible_check_eq;
+
+use fallible_check::check;
+use fallible_check_eq::check_eq;
 use tokio::net::TcpListener;
-use wireframe::client::WireframeClient;
+use wireframe::{
+    client::WireframeClient,
+    preamble::{read_preamble, write_preamble},
+};
 use wireframe_testing::TestResult;
 
 /// Test that setup and teardown callbacks are both invoked for a full
 /// connection lifecycle.
 #[tokio::test]
-#[expect(
-    clippy::panic_in_result_fn,
-    reason = "asserts provide clearer diagnostics in tests"
-)]
 async fn client_setup_and_teardown_callbacks_run() -> TestResult<()> {
     let setup_count = Arc::new(AtomicUsize::new(0));
     let teardown_count = Arc::new(AtomicUsize::new(0));
+    let teardown_state = Arc::new(AtomicU32::new(0));
     let setup = setup_count.clone();
     let teardown = teardown_count.clone();
+    let observed_teardown_state = teardown_state.clone();
 
     let listener = TcpListener::bind("127.0.0.1:0").await?;
     let addr = listener.local_addr()?;
 
     // Server accepts and then closes
     let server = tokio::spawn(async move {
-        let (stream, _) = listener.accept().await.expect("accept");
+        let (stream, _) = listener.accept().await?;
         drop(stream); // Close immediately
+        Ok::<(), wireframe::testkit::TestError>(())
     });
 
     let client = WireframeClient::builder()
@@ -49,38 +58,40 @@ async fn client_setup_and_teardown_callbacks_run() -> TestResult<()> {
         })
         .on_connection_teardown(move |state: u32| {
             let teardown = teardown.clone();
+            let observed_teardown_state = observed_teardown_state.clone();
             async move {
-                assert_eq!(state, 42, "teardown should receive state from setup");
+                observed_teardown_state.store(state, Ordering::SeqCst);
                 teardown.fetch_add(1, Ordering::SeqCst);
             }
         })
         .connect(addr)
         .await?;
 
-    assert_eq!(
+    check_eq(
         setup_count.load(Ordering::SeqCst),
         1,
-        "setup callback should run exactly once"
-    );
+        "setup callback should run exactly once",
+    )?;
 
     client.close().await;
 
-    assert_eq!(
+    check_eq(
+        teardown_state.load(Ordering::SeqCst),
+        42,
+        "teardown should receive state from setup",
+    )?;
+    check_eq(
         teardown_count.load(Ordering::SeqCst),
         1,
-        "teardown callback should run exactly once"
-    );
+        "teardown callback should run exactly once",
+    )?;
 
-    server.await?;
+    server.await??;
     Ok(())
 }
 
 /// Test that the error hook is invoked when the server disconnects unexpectedly.
 #[tokio::test]
-#[expect(
-    clippy::panic_in_result_fn,
-    reason = "asserts provide clearer diagnostics in tests"
-)]
 async fn client_error_hook_invoked_on_disconnect() -> TestResult<()> {
     let error_count = Arc::new(AtomicUsize::new(0));
     let count = error_count.clone();
@@ -90,8 +101,9 @@ async fn client_error_hook_invoked_on_disconnect() -> TestResult<()> {
 
     // Server accepts and then closes immediately
     let server = tokio::spawn(async move {
-        let (stream, _) = listener.accept().await.expect("accept");
+        let (stream, _) = listener.accept().await?;
         drop(stream);
+        Ok::<(), wireframe::testkit::TestError>(())
     });
 
     let mut client = WireframeClient::builder()
@@ -104,17 +116,20 @@ async fn client_error_hook_invoked_on_disconnect() -> TestResult<()> {
         .connect(addr)
         .await?;
 
-    server.await?;
+    server.await??;
 
     // Try to receive - should fail and invoke error hook
     let result: Result<Vec<u8>, wireframe::client::ClientError> = client.receive().await;
-    assert!(result.is_err(), "receive should fail after disconnect");
+    check(
+        result.is_err(),
+        format!("receive should fail after disconnect, got {result:?}"),
+    )?;
 
-    assert_eq!(
+    check_eq(
         error_count.load(Ordering::SeqCst),
         1,
-        "error callback should be invoked on disconnect"
-    );
+        "error callback should be invoked on disconnect",
+    )?;
 
     Ok(())
 }
@@ -129,15 +144,18 @@ struct ServerAck {
     accepted: bool,
 }
 
+async fn serve_preamble_ack(listener: TcpListener) -> TestResult<()> {
+    let (mut stream, _) = listener.accept().await?;
+    let (_hello, _) = read_preamble::<_, ClientHello>(&mut stream).await?;
+    write_preamble(&mut stream, &ServerAck { accepted: true }).await?;
+    drop(stream);
+    Ok(())
+}
+
 /// Test that lifecycle hooks can be combined with preamble callbacks.
 #[tokio::test]
-#[expect(
-    clippy::panic_in_result_fn,
-    reason = "asserts provide clearer diagnostics in tests"
-)]
 async fn client_lifecycle_hooks_work_with_preamble() -> TestResult<()> {
     use futures::FutureExt;
-    use wireframe::preamble::{read_preamble, write_preamble};
 
     let setup_count = Arc::new(AtomicUsize::new(0));
     let teardown_count = Arc::new(AtomicUsize::new(0));
@@ -149,17 +167,7 @@ async fn client_lifecycle_hooks_work_with_preamble() -> TestResult<()> {
     let listener = TcpListener::bind("127.0.0.1:0").await?;
     let addr = listener.local_addr()?;
 
-    // Server reads preamble and sends ack
-    let server = tokio::spawn(async move {
-        let (mut stream, _) = listener.accept().await.expect("accept");
-        let (_hello, _) = read_preamble::<_, ClientHello>(&mut stream)
-            .await
-            .expect("read preamble");
-        write_preamble(&mut stream, &ServerAck { accepted: true })
-            .await
-            .expect("write ack");
-        drop(stream);
-    });
+    let server = tokio::spawn(serve_preamble_ack(listener));
 
     let client = WireframeClient::builder()
         .with_preamble(ClientHello { version: 1 })
@@ -170,7 +178,12 @@ async fn client_lifecycle_hooks_work_with_preamble() -> TestResult<()> {
                 let (ack, leftover) = read_preamble::<_, ServerAck>(stream).await.map_err(|e| {
                     std::io::Error::new(std::io::ErrorKind::InvalidData, e.to_string())
                 })?;
-                assert!(ack.accepted, "server should accept preamble");
+                if !ack.accepted {
+                    return Err(std::io::Error::new(
+                        std::io::ErrorKind::InvalidData,
+                        "server should accept preamble",
+                    ));
+                }
                 Ok(leftover)
             }
             .boxed()
@@ -191,26 +204,26 @@ async fn client_lifecycle_hooks_work_with_preamble() -> TestResult<()> {
         .connect(addr)
         .await?;
 
-    server.await?;
+    server.await??;
 
-    assert_eq!(
+    check_eq(
         preamble_count.load(Ordering::SeqCst),
         1,
-        "preamble callback should run"
-    );
-    assert_eq!(
+        "preamble callback should run",
+    )?;
+    check_eq(
         setup_count.load(Ordering::SeqCst),
         1,
-        "setup callback should run"
-    );
+        "setup callback should run",
+    )?;
 
     client.close().await;
 
-    assert_eq!(
+    check_eq(
         teardown_count.load(Ordering::SeqCst),
         1,
-        "teardown callback should run"
-    );
+        "teardown callback should run",
+    )?;
 
     Ok(())
 }

tests/common/fallible_assertions/check.rs

@@ -0,0 +1,23 @@
+//! Fallible predicate checks shared by integration-test crates.
+
+/// Return an error containing `message` when a predicate is false.
+///
+/// # Examples
+///
+/// ```rust,ignore
+/// let successful = check(true, "the route is registered");
+/// let failed = check(false, "the route is missing");
+/// // `successful` is `Ok(())`; `failed` contains the supplied message.
+/// ```
+///
+/// # Errors
+///
+/// Returns `Err` with the supplied message when `condition` is false.
+pub(super) fn check(condition: bool, message: impl Into<String>) -> Result<(), String> {
+    if condition {
+        Ok(())
+    } else {
+        let message = message.into();
+        Err(message)
+    }
+}

tests/common/fallible_assertions/check_eq.rs

@@ -0,0 +1,26 @@
+//! Fallible equality checks for copyable values in integration-test crates.
+
+/// Compare two values and report their context and debug representations.
+///
+/// # Examples
+///
+/// ```rust,ignore
+/// let matched = check_eq(3, 3, "item count");
+/// let mismatched = check_eq(2, 3, "item count");
+/// // `matched` is `Ok(())`; the error says `item count: expected 3, got 2`.
+/// ```
+///
+/// # Errors
+///
+/// Returns `Err` when the values differ, formatted as
+/// `"{message}: expected {expected:?}, got {actual:?}"`.
+pub(super) fn check_eq<T>(actual: T, expected: T, message: &str) -> Result<(), String>
+where
+    T: std::fmt::Debug + PartialEq + Copy,
+{
+    if actual == expected {
+        Ok(())
+    } else {
+        Err(format!("{message}: expected {expected:?}, got {actual:?}"))
+    }
+}

tests/common/fallible_assertions/check_equal.rs

@@ -0,0 +1,27 @@
+//! Fallible equality checks for borrowed values in integration-test crates.
+
+/// Compare possibly unsized values and report their context on mismatch.
+///
+/// # Examples
+///
+/// ```rust,ignore
+/// let matched = check_equal(&"ready", &"ready", "state");
+/// let mismatched = check_equal(&"waiting", &"ready", "state");
+/// // `matched` is `Ok(())`; the error says `state: expected \"ready\", got \"waiting\"`.
+/// ```
+///
+/// # Errors
+///
+/// Returns `Err` when `actual` does not equal `expected`, formatted with their
+/// debug representations and `context`.
+pub(super) fn check_equal<A, E>(actual: &A, expected: &E, context: &str) -> Result<(), String>
+where
+    A: std::fmt::Debug + PartialEq<E> + ?Sized,
+    E: std::fmt::Debug + ?Sized,
+{
+    if actual == expected {
+        Ok(())
+    } else {
+        Err(format!("{context}: expected {expected:?}, got {actual:?}"))
+    }
+}