From c4d1995fb082029b6c25a6998e19f2e65e431066 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 23 Sep 2026 22:01:49 +0200 Subject: [PATCH 1/5] Improve test fixtures and fallible assertions Keep fixed test inputs non-zero at compile time, propagate fallible helper errors, and preserve callback timing without nested closures. Replace redundant assertions and numeric lint expectations with equivalent source-level test logic. --- src/app/inbound_handler/tests.rs | 7 +- src/client/tests/error_handling.rs | 55 ++++----------- src/client/tests/lifecycle.rs | 69 ++++++++----------- src/client/tests/send_streaming_infra.rs | 10 +-- src/client/tests/streaming.rs | 17 +++-- src/fragment/tests/adapter_tests.rs | 29 +++++--- .../budget_enforcement_tests.rs | 17 +++-- src/message_assembler/budget_tests.rs | 30 ++++---- src/message_assembler/tests.rs | 12 ++-- src/server/runtime/tests.rs | 23 ++++--- tests/response.rs | 63 ++++++++++++++--- 11 files changed, 175 insertions(+), 157 deletions(-) diff --git a/src/app/inbound_handler/tests.rs b/src/app/inbound_handler/tests.rs index dfde80905..305864f31 100644 --- a/src/app/inbound_handler/tests.rs +++ b/src/app/inbound_handler/tests.rs @@ -82,8 +82,11 @@ fn decode_envelope_tracks_failures_and_logs_correlation_id() { &frame, &mut failure_tracker, ); - assert!(result.is_ok(), "expected recoverable decode failure"); - assert!(result.expect("decode result").is_none()); + let decoded = result.expect("expected recoverable decode failure"); + assert!( + decoded.is_none(), + "recoverable decode should not yield a message" + ); } let mut failure_tracker = diff --git a/src/client/tests/error_handling.rs b/src/client/tests/error_handling.rs index 449c7aa4c..b3d2c280c 100644 --- a/src/client/tests/error_handling.rs +++ b/src/client/tests/error_handling.rs @@ -1,6 +1,7 @@ //! Error handling tests for the wireframe client. use std::{ + future::Future, sync::{ Arc, atomic::{AtomicUsize, Ordering}, @@ -9,7 +10,7 @@ use std::{ }; use bytes::Bytes; -use futures::SinkExt; +use futures::{SinkExt, future::lazy}; use tokio::net::TcpStream; use tokio_util::codec::{Framed, LengthDelimitedCodec}; @@ -27,6 +28,12 @@ use crate::{ serializer::Serializer, }; +fn record_error_count(count: Arc) -> impl Future + Send { + lazy(move |_| { + count.fetch_add(1, Ordering::SeqCst); + }) +} + /// Connects a client and returns both the client and server stream for custom server behaviour. /// /// This helper is used by tests that need to manipulate the server stream after connection, @@ -48,18 +55,9 @@ where } #[tokio::test] -#[expect( - clippy::excessive_nesting, - reason = "async closures within builder patterns are inherently nested" -)] async fn error_callback_invoked_on_receive_error() { let error_count = test_error_hook_on_disconnect(|builder, count| { - builder.on_error(move |_err| { - let count = count.clone(); - async move { - count.fetch_add(1, Ordering::SeqCst); - } - }) + builder.on_error(move |_err| record_error_count(count.clone())) }) .await .expect("run error hook scenario"); @@ -87,21 +85,12 @@ async fn no_error_hook_does_not_panic() { } #[tokio::test] -#[expect( - clippy::excessive_nesting, - reason = "async closures within builder patterns are inherently nested" -)] async fn error_callback_invoked_on_deserialize_error() { let error_count = Arc::new(AtomicUsize::new(0)); let count = error_count.clone(); let (mut client, server_stream) = connect_with_server(|builder| { - builder.on_error(move |_err| { - let count = count.clone(); - async move { - count.fetch_add(1, Ordering::SeqCst); - } - }) + builder.on_error(move |_err| record_error_count(count.clone())) }) .await .expect("connect client and server"); @@ -136,10 +125,6 @@ async fn error_callback_invoked_on_deserialize_error() { } #[tokio::test] -#[expect( - clippy::excessive_nesting, - reason = "async closures within builder patterns are inherently nested" -)] async fn error_callback_invoked_on_send_io_error() { #[derive(bincode::Encode, bincode::BorrowDecode)] struct TestMessage(Vec); @@ -148,12 +133,7 @@ async fn error_callback_invoked_on_send_io_error() { let count = error_count.clone(); let (mut client, server_stream) = connect_with_server(|builder| { - builder.on_error(move |_err| { - let count = count.clone(); - async move { - count.fetch_add(1, Ordering::SeqCst); - } - }) + builder.on_error(move |_err| record_error_count(count.clone())) }) .await .expect("connect client and server"); @@ -187,10 +167,6 @@ async fn error_callback_invoked_on_send_io_error() { } #[tokio::test] -#[expect( - clippy::excessive_nesting, - reason = "async closures within builder patterns are inherently nested" -)] async fn error_callback_invoked_on_serialize_error() { #[derive(bincode::Encode, bincode::BorrowDecode)] struct TestMessage(u32); @@ -199,12 +175,9 @@ async fn error_callback_invoked_on_serialize_error() { let count = error_count.clone(); let (mut client, _server) = connect_with_server(|builder| { - builder.serializer(FailingSerializer).on_error(move |_err| { - let count = count.clone(); - async move { - count.fetch_add(1, Ordering::SeqCst); - } - }) + builder + .serializer(FailingSerializer) + .on_error(move |_err| record_error_count(count.clone())) }) .await .expect("connect client and server"); diff --git a/src/client/tests/lifecycle.rs b/src/client/tests/lifecycle.rs index e5559033c..b5763e2b8 100644 --- a/src/client/tests/lifecycle.rs +++ b/src/client/tests/lifecycle.rs @@ -1,12 +1,34 @@ //! Lifecycle hook tests for the wireframe client. -use std::sync::{ - Arc, - atomic::{AtomicUsize, Ordering}, +use std::{ + future::Future, + sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, + }, }; +use futures::future::lazy; + use super::helpers::{counting_hook, test_error_hook_on_disconnect, test_with_client}; +fn record_count(count: Arc) -> impl Future + Send { + lazy(move |_| { + count.fetch_add(1, Ordering::SeqCst); + }) +} + +async fn record_setup(count: Arc) -> &'static str { + record_count(count).await; + "state" +} + +fn record_teardown_state(value: Arc, state: usize) -> impl Future + Send { + lazy(move |_| { + value.store(state, Ordering::SeqCst); + }) +} + #[tokio::test] async fn setup_callback_invoked_on_connect() { let (setup_count, increment) = counting_hook(); @@ -23,10 +45,6 @@ async fn setup_callback_invoked_on_connect() { } #[tokio::test] -#[expect( - clippy::excessive_nesting, - reason = "async closures within builder patterns are inherently nested" -)] async fn teardown_callback_receives_setup_state() { let teardown_value = Arc::new(AtomicUsize::new(0)); let value = teardown_value.clone(); @@ -34,12 +52,7 @@ async fn teardown_callback_receives_setup_state() { let client = test_with_client(|builder| { builder .on_connection_setup(|| async { 42usize }) - .on_connection_teardown(move |state| { - let value = value.clone(); - async move { - value.store(state, Ordering::SeqCst); - } - }) + .on_connection_teardown(move |state| record_teardown_state(value.clone(), state)) }) .await .expect("connect client"); @@ -73,10 +86,6 @@ async fn teardown_without_setup_does_not_run() { } #[tokio::test] -#[expect( - clippy::excessive_nesting, - reason = "async closures within builder patterns are inherently nested" -)] async fn setup_and_teardown_callbacks_run() { let setup_count = Arc::new(AtomicUsize::new(0)); let teardown_count = Arc::new(AtomicUsize::new(0)); @@ -85,19 +94,8 @@ async fn setup_and_teardown_callbacks_run() { let client = test_with_client(|builder| { builder - .on_connection_setup(move || { - let setup = setup.clone(); - async move { - setup.fetch_add(1, Ordering::SeqCst); - "state" - } - }) - .on_connection_teardown(move |_: &str| { - let teardown = teardown.clone(); - async move { - teardown.fetch_add(1, Ordering::SeqCst); - } - }) + .on_connection_setup(move || record_setup(setup.clone())) + .on_connection_teardown(move |_: &str| record_count(teardown.clone())) }) .await .expect("connect client"); @@ -118,21 +116,12 @@ async fn setup_and_teardown_callbacks_run() { } #[tokio::test] -#[expect( - clippy::excessive_nesting, - reason = "async closures within builder patterns are inherently nested" -)] async fn on_connection_setup_preserves_error_hook() { // Configure on_error first, then on_connection_setup. // The error hook should be preserved. let error_count = test_error_hook_on_disconnect(|builder, count| { builder - .on_error(move |_err| { - let count = count.clone(); - async move { - count.fetch_add(1, Ordering::SeqCst); - } - }) + .on_error(move |_err| record_count(count.clone())) .on_connection_setup(|| async { 42u32 }) }) .await diff --git a/src/client/tests/send_streaming_infra.rs b/src/client/tests/send_streaming_infra.rs index a53c0dbad..d098f51ef 100644 --- a/src/client/tests/send_streaming_infra.rs +++ b/src/client/tests/send_streaming_infra.rs @@ -202,15 +202,7 @@ pub(super) async fn create_send_client_with_error_hook( } /// Generate a body of `n` bytes for testing: cycles through 0–255. -#[expect( - clippy::integer_division_remainder_used, - reason = "modulo generates a deterministic test byte pattern" -)] -#[expect( - clippy::cast_possible_truncation, - reason = "value is modulo 256, guaranteed to fit in u8" -)] -pub(super) fn test_body(n: usize) -> Vec { (0..n).map(|i| (i % 256) as u8).collect() } +pub(super) fn test_body(n: usize) -> Vec { (u8::MIN..=u8::MAX).cycle().take(n).collect() } type ByteResult = Result; type BlockingStream = tokio_stream::wrappers::ReceiverStream; diff --git a/src/client/tests/streaming.rs b/src/client/tests/streaming.rs index 53616a96f..030850871 100644 --- a/src/client/tests/streaming.rs +++ b/src/client/tests/streaming.rs @@ -38,10 +38,18 @@ where .next() .await .ok_or("stream ended before yielding a data frame")??; - assert_eq!(frame.payload, expected_payload); + if frame.payload != expected_payload { + return Err(format!( + "stream payload mismatch: got {:?}, expected {:?}", + frame.payload, expected_payload + ) + .into()); + } let end = stream.next().await; - assert!(end.is_none(), "stream should terminate"); + if end.is_some() { + return Err("stream should terminate after one data frame".into()); + } Ok(()) } @@ -98,9 +106,8 @@ async fn response_stream_terminates_on_terminator( .expect("call_streaming"); // First item is the data frame. - let first = stream.next().await; - assert!(first.is_some(), "should yield one data frame"); - assert!(first.expect("some").is_ok(), "data frame should be Ok"); + let first = stream.next().await.expect("should yield one data frame"); + assert!(first.is_ok(), "data frame should be Ok"); // Mid-stream, before the terminator is polled, the stream must report // that it has not terminated (guards the `is_terminated` false polarity). diff --git a/src/fragment/tests/adapter_tests.rs b/src/fragment/tests/adapter_tests.rs index 3d046cfc8..49787910b 100644 --- a/src/fragment/tests/adapter_tests.rs +++ b/src/fragment/tests/adapter_tests.rs @@ -37,13 +37,24 @@ impl Fragmentable for TestPacket { /// Result alias for fallible fragment-adapter test helpers. type TestResult = Result>; -fn adapter_config() -> TestResult { - Ok(FragmentationConfig { - fragment_payload_cap: NonZeroUsize::new(4) - .ok_or("fragment payload cap must be non-zero")?, - max_message_size: NonZeroUsize::new(64).ok_or("max message size must be non-zero")?, +/// Fixture payload cap; an accidental zero fails during compilation. +const FRAGMENT_PAYLOAD_CAP: NonZeroUsize = match NonZeroUsize::new(4) { + Some(value) => value, + None => panic!("fragment payload cap must be non-zero"), +}; + +/// Fixture maximum message size; an accidental zero fails during compilation. +const MAX_MESSAGE_SIZE: NonZeroUsize = match NonZeroUsize::new(64) { + Some(value) => value, + None => panic!("max message size must be non-zero"), +}; + +fn adapter_config() -> FragmentationConfig { + FragmentationConfig { + fragment_payload_cap: FRAGMENT_PAYLOAD_CAP, + max_message_size: MAX_MESSAGE_SIZE, reassembly_timeout: Duration::from_secs(30), - }) + } } fn build_test_packet() -> TestPacket { @@ -91,7 +102,7 @@ fn reassemble_fragment_sequence( #[test] fn default_fragment_adapter_fragments_and_reassembles_test_packets() { - let mut adapter = DefaultFragmentAdapter::new(adapter_config().expect("build adapter config")); + let mut adapter = DefaultFragmentAdapter::new(adapter_config()); let packet = build_test_packet(); let fragments = adapter @@ -111,7 +122,7 @@ fn default_fragment_adapter_fragments_and_reassembles_test_packets() { #[test] fn default_fragment_adapter_passes_through_non_fragment_payloads() { - let mut adapter = DefaultFragmentAdapter::new(adapter_config().expect("build adapter config")); + let mut adapter = DefaultFragmentAdapter::new(adapter_config()); let packet = TestPacket { id: 12, correlation_id: Some(9), @@ -127,7 +138,7 @@ fn default_fragment_adapter_passes_through_non_fragment_payloads() { #[test] fn default_fragment_adapter_exposes_purge_api() { - let mut config = adapter_config().expect("build adapter config"); + let mut config = adapter_config(); config.reassembly_timeout = Duration::ZERO; let mut adapter = DefaultFragmentAdapter::new(config); let header = FragmentHeader::new(MessageId::new(81), FragmentIndex::zero(), false); diff --git a/src/message_assembler/budget_enforcement_tests.rs b/src/message_assembler/budget_enforcement_tests.rs index 6deb9fccf..f64f2da2e 100644 --- a/src/message_assembler/budget_enforcement_tests.rs +++ b/src/message_assembler/budget_enforcement_tests.rs @@ -9,7 +9,6 @@ use super::{ connection_budgeted_state, dual_budgeted_state, in_flight_budgeted_state, - nz, submit_first, submit_first_at, unbounded_state, @@ -175,10 +174,10 @@ fn dual_budget_in_flight_triggers_before_connection( fn dual_budget_connection_triggers_when_in_flight_not_exceeded() { // connection=15 < in_flight=20, so connection triggers first. let mut state = MessageAssemblyState::with_budgets( - nz(1024), + nz!(1024), Duration::from_secs(30), - Some(nz(15)), - Some(nz(20)), + Some(nz!(15)), + Some(nz!(20)), ); let err = submit_first(&mut state, 1, &[0u8; 16], false).expect_err("should reject"); assert!( @@ -240,7 +239,7 @@ fn budget_violation_does_not_affect_other_assemblies( #[test] fn headroom_reclaimed_after_purge_allows_new_assembly() { let mut state = - MessageAssemblyState::with_budgets(nz(1024), Duration::from_secs(30), Some(nz(20)), None); + MessageAssemblyState::with_budgets(nz!(1024), Duration::from_secs(30), Some(nz!(20)), None); let now = Instant::now(); submit_first_at(&mut state, 1, &[0u8; 15], now).expect("first"); @@ -266,7 +265,7 @@ fn headroom_reclaimed_after_purge_allows_new_assembly() { #[test] fn headroom_reclaimed_after_completion_allows_new_frame() { let mut state = - MessageAssemblyState::with_budgets(nz(1024), Duration::from_secs(30), None, Some(nz(20))); + MessageAssemblyState::with_budgets(nz!(1024), Duration::from_secs(30), None, Some(nz!(20))); submit_first(&mut state, 1, &[0u8; 15], false).expect("first"); @@ -327,7 +326,7 @@ fn connection_budget_accepts_exact_fit( /// the exact-limit assembly. #[test] fn size_limit_accepts_exact_total() { - let mut state = MessageAssemblyState::new(nz(10), Duration::from_secs(30)); + let mut state = MessageAssemblyState::new(nz!(10), Duration::from_secs(30)); submit_first(&mut state, 1, &[0u8; 5], false).expect("first frame within limit"); let cont = continuation_header(1, 1, 5, true); @@ -343,7 +342,7 @@ fn size_limit_accepts_exact_total() { /// `total_message_size > max`, so a `>=` mutant would reject the exact case. #[test] fn declared_total_at_size_limit_is_accepted() { - let mut state = MessageAssemblyState::new(nz(10), Duration::from_secs(30)); + let mut state = MessageAssemblyState::new(nz!(10), Duration::from_secs(30)); let header = first_header_with_total(1, 0, 10); let input = FirstFrameInput::new(&header, EnvelopeRouting::default(), vec![], &[]) .expect("valid first-frame input"); @@ -361,7 +360,7 @@ fn declared_total_at_size_limit_is_accepted() { /// key and empty the state (guards the `-> vec![]` mutant). #[test] fn wall_clock_purge_evicts_expired_assembly() { - let mut state = MessageAssemblyState::new(nz(1024), Duration::ZERO); + let mut state = MessageAssemblyState::new(nz!(1024), Duration::ZERO); submit_first(&mut state, 7, &[0u8; 5], false).expect("buffered first frame"); assert_eq!(state.buffered_count(), 1); diff --git a/src/message_assembler/budget_tests.rs b/src/message_assembler/budget_tests.rs index 604f54ad4..8356655af 100644 --- a/src/message_assembler/budget_tests.rs +++ b/src/message_assembler/budget_tests.rs @@ -4,10 +4,7 @@ //! Enforcement tests (connection, in-flight, dual, isolation, headroom, //! single-frame bypass) are in the `enforcement` sub-module. -use std::{ - num::NonZeroUsize, - time::{Duration, Instant}, -}; +use std::time::{Duration, Instant}; use rstest::{fixture, rstest}; @@ -23,9 +20,16 @@ use crate::message_assembler::{ // Helpers // --------------------------------------------------------------------------- -/// Non-zero shorthand. -#[expect(clippy::unwrap_used, reason = "caller guarantees non-zero")] -fn nz(val: usize) -> NonZeroUsize { NonZeroUsize::new(val).unwrap() } +/// Reject a zero test budget during compilation, before fixture setup runs. +macro_rules! nz { + ($value:expr) => {{ + const NON_ZERO: ::std::num::NonZeroUsize = match ::std::num::NonZeroUsize::new($value) { + Some(value) => value, + None => panic!("test budget must be non-zero"), + }; + NON_ZERO + }}; +} /// Build a [`FirstFrameHeader`] and [`FirstFrameInput`] in the caller's /// scope from a key, body slice, and finality flag. @@ -70,29 +74,29 @@ fn submit_first_at( /// State with no budgets — backwards-compatibility baseline. #[fixture] fn unbounded_state() -> MessageAssemblyState { - MessageAssemblyState::new(nz(1024), Duration::from_secs(30)) + MessageAssemblyState::new(nz!(1024), Duration::from_secs(30)) } /// State with a 20-byte connection budget and 1024-byte per-message limit. #[fixture] fn connection_budgeted_state() -> MessageAssemblyState { - MessageAssemblyState::with_budgets(nz(1024), Duration::from_secs(30), Some(nz(20)), None) + MessageAssemblyState::with_budgets(nz!(1024), Duration::from_secs(30), Some(nz!(20)), None) } /// State with a 20-byte in-flight budget and 1024-byte per-message limit. #[fixture] fn in_flight_budgeted_state() -> MessageAssemblyState { - MessageAssemblyState::with_budgets(nz(1024), Duration::from_secs(30), None, Some(nz(20))) + MessageAssemblyState::with_budgets(nz!(1024), Duration::from_secs(30), None, Some(nz!(20))) } /// State with both connection (30) and in-flight (20) budgets. #[fixture] fn dual_budgeted_state() -> MessageAssemblyState { MessageAssemblyState::with_budgets( - nz(1024), + nz!(1024), Duration::from_secs(30), - Some(nz(30)), - Some(nz(20)), + Some(nz!(30)), + Some(nz!(20)), ) } diff --git a/src/message_assembler/tests.rs b/src/message_assembler/tests.rs index 97255de55..a045bc5c3 100644 --- a/src/message_assembler/tests.rs +++ b/src/message_assembler/tests.rs @@ -86,7 +86,7 @@ fn parse_frame_headers( #[case] payload: Vec, #[case] expected_header: FrameHeader, ) { - let parsed = parse_header(&payload); + let parsed = parse_header(&payload).expect("parse header-only payload"); assert_eq!(parsed.header(), &expected_header, "case: {case_name}"); assert_eq!( parsed.header_len(), @@ -97,7 +97,7 @@ fn parse_frame_headers( let mut payload_with_body = payload.clone(); payload_with_body.extend_from_slice(&[0xaa, 0xbb, 0xcc]); - let parsed_with_body = parse_header(&payload_with_body); + let parsed_with_body = parse_header(&payload_with_body).expect("parse header with body bytes"); assert_eq!( parsed_with_body.header(), &expected_header, @@ -182,12 +182,8 @@ fn build_continuation_header_payload(spec: ContinuationHeaderSpec) -> Vec { bytes.to_vec() } -#[expect( - clippy::unwrap_used, - reason = "test helper; header parse failure is a bug" -)] -fn parse_header(payload: &[u8]) -> ParsedFrameHeader { - TestAssembler.parse_frame_header(payload).unwrap() +fn parse_header(payload: &[u8]) -> io::Result { + TestAssembler.parse_frame_header(payload) } // ============================================================================= diff --git a/src/server/runtime/tests.rs b/src/server/runtime/tests.rs index b584ea308..b56970809 100644 --- a/src/server/runtime/tests.rs +++ b/src/server/runtime/tests.rs @@ -2,6 +2,7 @@ use std::{ io, + net::SocketAddr, sync::{ Arc, Mutex, @@ -40,13 +41,12 @@ async fn test_run_with_immediate_shutdown( ) -> Result<(), Box> { let server = bind_server(factory, free_listener?)?; let shutdown_future = async { tokio::time::sleep(Duration::from_millis(10)).await }; - let result = timeout( + timeout( Duration::from_secs(1), server.run_with_shutdown(shutdown_future), ) - .await; - assert!(result.is_ok()); - assert!(result.expect("server did not finish in time").is_ok()); + .await + .map_err(|_| "server did not finish in time")??; Ok(()) } @@ -132,13 +132,12 @@ async fn test_multiple_worker_creation( .bind_existing_listener(free_listener?) .expect("Failed to bind"); let shutdown_future = async { tokio::time::sleep(Duration::from_millis(10)).await }; - let result = timeout( + timeout( Duration::from_secs(1), server.run_with_shutdown(shutdown_future), ) - .await; - assert!(result.is_ok()); - assert!(result.expect("server did not finish in time").is_ok()); + .await + .map_err(|_| "server did not finish in time")??; Ok(()) } @@ -175,7 +174,6 @@ async fn test_accept_loop_shutdown_signal( } /// Creates a mock listener that fails with exponential backoff tracking. -#[expect(clippy::unwrap_used, reason = "mock setup; known-valid values")] fn setup_backoff_mock_listener( calls: &Arc>>, num_calls: usize, @@ -187,14 +185,17 @@ fn setup_backoff_mock_listener( .returning(move || { let call_log = Arc::clone(&call_log); Box::pin(async move { - call_log.lock().unwrap().push(Instant::now()); + call_log + .lock() + .map_err(|_| io::Error::other("mock call log mutex poisoned"))? + .push(Instant::now()); Err(io::Error::other("mock error")) }) }) .times(num_calls); listener .expect_local_addr() - .returning(|| Ok("127.0.0.1:0".parse().unwrap())) + .returning(|| Ok(SocketAddr::from(([127, 0, 0, 1], 0)))) .times(num_calls); listener } diff --git a/tests/response.rs b/tests/response.rs index e8846fc2c..dba6702d7 100644 --- a/tests/response.rs +++ b/tests/response.rs @@ -50,11 +50,20 @@ async fn send_response_encodes_and_frames() -> TestResult { .map_err(|e| format!("send_response failed: {e}"))?; let frames = decode_frames(&out)?; - assert_eq!(frames.len(), 1, "expected a single response frame"); + if frames.len() != 1 { + return Err(format!("expected a single response frame, got {}", frames.len()).into()); + } let frame = frames.first().ok_or("expected frame missing")?; let (decoded, _) = TestResp::from_bytes(frame).map_err(|e| format!("deserialize failed: {e}"))?; - assert_eq!(decoded, TestResp(7), "decoded payload mismatch"); + if decoded != TestResp(7) { + return Err(format!( + "decoded payload mismatch: expected {:?}, got {:?}", + TestResp(7), + decoded + ) + .into()); + } Ok(()) } @@ -188,9 +197,16 @@ async fn send_response_framed_sends_raw_serialized_payload() -> TestResult { .await .map_err(|e| format!("read framed output failed: {e}"))?; let decoded_frames = decode_frames(&out)?; - assert_eq!(decoded_frames.len(), 1); + if decoded_frames.len() != 1 { + return Err(format!("expected one response frame, got {}", decoded_frames.len()).into()); + } let frame = decoded_frames.first().ok_or("response frame missing")?; - assert_eq!(frame, &expected); + if frame != &expected { + return Err(format!( + "serialized response frame mismatch: expected {expected:?}, got {frame:?}" + ) + .into()); + } Ok(()) } @@ -212,10 +228,19 @@ async fn send_response_framed_honours_buffer_capacity() -> TestResult { .await .map_err(|e| format!("read framed output failed: {e}"))?; let decoded_frames = decode_frames_with_max(&out, LARGE_FRAME)?; - assert_eq!(decoded_frames.len(), 1); + if decoded_frames.len() != 1 { + return Err(format!("expected one response frame, got {}", decoded_frames.len()).into()); + } let frame = decoded_frames.first().ok_or("response frame missing")?; let (decoded, _) = Large::from_bytes(frame).map_err(|e| format!("deserialize failed: {e}"))?; - assert_eq!(decoded.0.len(), payload.len()); + if decoded.0.len() != payload.len() { + return Err(format!( + "decoded payload length mismatch: expected {}, got {}", + payload.len(), + decoded.0.len() + ) + .into()); + } Ok(()) } @@ -234,10 +259,19 @@ async fn send_response_honours_buffer_capacity() -> TestResult { .map_err(|e| format!("send_response failed: {e}"))?; let frames = decode_frames_with_max(&out, LARGE_FRAME)?; - assert_eq!(frames.len(), 1, "expected a single response frame"); + if frames.len() != 1 { + return Err(format!("expected a single response frame, got {}", frames.len()).into()); + } let frame = frames.first().ok_or("response frame missing")?; let (decoded, _) = Large::from_bytes(frame).map_err(|e| format!("deserialize failed: {e}"))?; - assert_eq!(decoded.0.len(), payload.len()); + if decoded.0.len() != payload.len() { + return Err(format!( + "decoded payload length mismatch: expected {}, got {}", + payload.len(), + decoded.0.len() + ) + .into()); + } Ok(()) } @@ -260,12 +294,21 @@ async fn process_stream_honours_buffer_capacity() -> TestResult { let out = run_app(app, vec![frame], Some(10 * 1024 * 1024)).await?; let frames = decode_frames_with_max(&out, LARGE_FRAME)?; - assert_eq!(frames.len(), 1, "expected a single response frame"); + if frames.len() != 1 { + return Err(format!("expected a single response frame, got {}", frames.len()).into()); + } let frame = frames.first().ok_or("response frame missing")?; let (resp_env, _) = BincodeSerializer .deserialize::(frame) .map_err(|e| format!("deserialize failed: {e}"))?; let resp_len = resp_env.into_parts().into_payload().len(); - assert_eq!(resp_len, payload.len()); + if resp_len != payload.len() { + return Err(format!( + "response payload length mismatch: expected {}, got {}", + payload.len(), + resp_len + ) + .into()); + } Ok(()) } From d063a7dde2d0e5367897680f08e9aba636d56fc5 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 23 Sep 2026 22:19:46 +0200 Subject: [PATCH 2/5] Consolidate response test checks Extract fallible frame and payload-length checks shared by response tests. Keep the same predicates and diagnostic text while reducing CodeScene duplication in the test-quality layer. --- tests/response.rs | 81 ++++++++++++++++++++++------------------------- 1 file changed, 37 insertions(+), 44 deletions(-) diff --git a/tests/response.rs b/tests/response.rs index dba6702d7..ffd2425e4 100644 --- a/tests/response.rs +++ b/tests/response.rs @@ -38,6 +38,24 @@ struct TestResp(u32); #[derive(bincode::Encode, bincode::BorrowDecode, PartialEq, Debug)] struct Large(Vec); +fn single_response_frame<'a, T>( + frames: &'a [T], + count_description: &str, + missing_message: &'static str, +) -> TestResult<&'a T> { + if frames.len() != 1 { + return Err(format!("expected {count_description}, got {}", frames.len()).into()); + } + Ok(frames.first().ok_or(missing_message)?) +} + +fn check_payload_length(actual: usize, expected: usize, description: &str) -> TestResult { + if actual != expected { + return Err(format!("{description} mismatch: expected {expected}, got {actual}").into()); + } + Ok(()) +} + /// Tests that sending a response serializes and frames the data correctly, /// and that the response can be decoded and deserialized back to its original value asynchronously. #[tokio::test] @@ -50,10 +68,8 @@ async fn send_response_encodes_and_frames() -> TestResult { .map_err(|e| format!("send_response failed: {e}"))?; let frames = decode_frames(&out)?; - if frames.len() != 1 { - return Err(format!("expected a single response frame, got {}", frames.len()).into()); - } - let frame = frames.first().ok_or("expected frame missing")?; + let frame = + single_response_frame(&frames, "a single response frame", "expected frame missing")?; let (decoded, _) = TestResp::from_bytes(frame).map_err(|e| format!("deserialize failed: {e}"))?; if decoded != TestResp(7) { @@ -197,10 +213,11 @@ async fn send_response_framed_sends_raw_serialized_payload() -> TestResult { .await .map_err(|e| format!("read framed output failed: {e}"))?; let decoded_frames = decode_frames(&out)?; - if decoded_frames.len() != 1 { - return Err(format!("expected one response frame, got {}", decoded_frames.len()).into()); - } - let frame = decoded_frames.first().ok_or("response frame missing")?; + let frame = single_response_frame( + &decoded_frames, + "one response frame", + "response frame missing", + )?; if frame != &expected { return Err(format!( "serialized response frame mismatch: expected {expected:?}, got {frame:?}" @@ -228,19 +245,13 @@ async fn send_response_framed_honours_buffer_capacity() -> TestResult { .await .map_err(|e| format!("read framed output failed: {e}"))?; let decoded_frames = decode_frames_with_max(&out, LARGE_FRAME)?; - if decoded_frames.len() != 1 { - return Err(format!("expected one response frame, got {}", decoded_frames.len()).into()); - } - let frame = decoded_frames.first().ok_or("response frame missing")?; + let frame = single_response_frame( + &decoded_frames, + "one response frame", + "response frame missing", + )?; let (decoded, _) = Large::from_bytes(frame).map_err(|e| format!("deserialize failed: {e}"))?; - if decoded.0.len() != payload.len() { - return Err(format!( - "decoded payload length mismatch: expected {}, got {}", - payload.len(), - decoded.0.len() - ) - .into()); - } + check_payload_length(decoded.0.len(), payload.len(), "decoded payload length")?; Ok(()) } @@ -259,19 +270,10 @@ async fn send_response_honours_buffer_capacity() -> TestResult { .map_err(|e| format!("send_response failed: {e}"))?; let frames = decode_frames_with_max(&out, LARGE_FRAME)?; - if frames.len() != 1 { - return Err(format!("expected a single response frame, got {}", frames.len()).into()); - } - let frame = frames.first().ok_or("response frame missing")?; + let frame = + single_response_frame(&frames, "a single response frame", "response frame missing")?; let (decoded, _) = Large::from_bytes(frame).map_err(|e| format!("deserialize failed: {e}"))?; - if decoded.0.len() != payload.len() { - return Err(format!( - "decoded payload length mismatch: expected {}, got {}", - payload.len(), - decoded.0.len() - ) - .into()); - } + check_payload_length(decoded.0.len(), payload.len(), "decoded payload length")?; Ok(()) } @@ -294,21 +296,12 @@ async fn process_stream_honours_buffer_capacity() -> TestResult { let out = run_app(app, vec![frame], Some(10 * 1024 * 1024)).await?; let frames = decode_frames_with_max(&out, LARGE_FRAME)?; - if frames.len() != 1 { - return Err(format!("expected a single response frame, got {}", frames.len()).into()); - } - let frame = frames.first().ok_or("response frame missing")?; + let frame = + single_response_frame(&frames, "a single response frame", "response frame missing")?; let (resp_env, _) = BincodeSerializer .deserialize::(frame) .map_err(|e| format!("deserialize failed: {e}"))?; let resp_len = resp_env.into_parts().into_payload().len(); - if resp_len != payload.len() { - return Err(format!( - "response payload length mismatch: expected {}, got {}", - payload.len(), - resp_len - ) - .into()); - } + check_payload_length(resp_len, payload.len(), "response payload length")?; Ok(()) } From 87f28e06f4d8e5b9eb8f119dc07334c8cb88b274 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 24 Sep 2026 06:09:59 +0200 Subject: [PATCH 3/5] Test zero fixture budgets at compile time Share the const non-zero guard across budget fixtures and a trybuild UI case, so a zero budget has a checked compile error. Document when the client setup helper records its invocation and returns teardown state. --- src/client/tests/lifecycle.rs | 2 ++ src/fragment/tests/adapter_tests.rs | 12 ++++------- src/message_assembler/budget_tests.rs | 11 +--------- tests/compile_error.rs | 1 + tests/support/non_zero.rs | 14 ++++++++++++ tests/ui/zero_test_budget.rs | 9 ++++++++ tests/ui/zero_test_budget.stderr | 31 +++++++++++++++++++++++++++ 7 files changed, 62 insertions(+), 18 deletions(-) create mode 100644 tests/support/non_zero.rs create mode 100644 tests/ui/zero_test_budget.rs create mode 100644 tests/ui/zero_test_budget.stderr diff --git a/src/client/tests/lifecycle.rs b/src/client/tests/lifecycle.rs index b5763e2b8..988054157 100644 --- a/src/client/tests/lifecycle.rs +++ b/src/client/tests/lifecycle.rs @@ -18,6 +18,8 @@ fn record_count(count: Arc) -> impl Future + Send { }) } +/// Count setup when the callback future is first polled, then return the +/// state that the teardown callback receives. async fn record_setup(count: Arc) -> &'static str { record_count(count).await; "state" diff --git a/src/fragment/tests/adapter_tests.rs b/src/fragment/tests/adapter_tests.rs index 49787910b..abd60177e 100644 --- a/src/fragment/tests/adapter_tests.rs +++ b/src/fragment/tests/adapter_tests.rs @@ -37,17 +37,13 @@ impl Fragmentable for TestPacket { /// Result alias for fallible fragment-adapter test helpers. type TestResult = Result>; +include!("../../../tests/support/non_zero.rs"); + /// Fixture payload cap; an accidental zero fails during compilation. -const FRAGMENT_PAYLOAD_CAP: NonZeroUsize = match NonZeroUsize::new(4) { - Some(value) => value, - None => panic!("fragment payload cap must be non-zero"), -}; +const FRAGMENT_PAYLOAD_CAP: NonZeroUsize = nz!(4, "fragment payload cap must be non-zero"); /// Fixture maximum message size; an accidental zero fails during compilation. -const MAX_MESSAGE_SIZE: NonZeroUsize = match NonZeroUsize::new(64) { - Some(value) => value, - None => panic!("max message size must be non-zero"), -}; +const MAX_MESSAGE_SIZE: NonZeroUsize = nz!(64, "max message size must be non-zero"); fn adapter_config() -> FragmentationConfig { FragmentationConfig { diff --git a/src/message_assembler/budget_tests.rs b/src/message_assembler/budget_tests.rs index 8356655af..179ecee41 100644 --- a/src/message_assembler/budget_tests.rs +++ b/src/message_assembler/budget_tests.rs @@ -20,16 +20,7 @@ use crate::message_assembler::{ // Helpers // --------------------------------------------------------------------------- -/// Reject a zero test budget during compilation, before fixture setup runs. -macro_rules! nz { - ($value:expr) => {{ - const NON_ZERO: ::std::num::NonZeroUsize = match ::std::num::NonZeroUsize::new($value) { - Some(value) => value, - None => panic!("test budget must be non-zero"), - }; - NON_ZERO - }}; -} +include!("../../tests/support/non_zero.rs"); /// Build a [`FirstFrameHeader`] and [`FirstFrameInput`] in the caller's /// scope from a key, body slice, and finality flag. diff --git a/tests/compile_error.rs b/tests/compile_error.rs index 16d31ff18..ec15c6086 100644 --- a/tests/compile_error.rs +++ b/tests/compile_error.rs @@ -6,4 +6,5 @@ fn compile_tests() { t.pass("tests/ui/wireframe_result_default_no_protocol.rs"); t.compile_fail("tests/ui/wireframe_result_default_rejects_unit_protocol.rs"); t.compile_fail("tests/ui/prepared_app_rejects_route.rs"); + t.compile_fail("tests/ui/zero_test_budget.rs"); } diff --git a/tests/support/non_zero.rs b/tests/support/non_zero.rs new file mode 100644 index 000000000..685cc7268 --- /dev/null +++ b/tests/support/non_zero.rs @@ -0,0 +1,14 @@ +/// Turn a fixture budget into a constant `NonZeroUsize`, rejecting zero at +/// compile time. The optional message identifies the fixture that failed. +macro_rules! nz { + ($value:expr) => { + nz!($value, "test budget must be non-zero") + }; + ($value:expr, $message:literal) => {{ + const NON_ZERO: ::std::num::NonZeroUsize = match ::std::num::NonZeroUsize::new($value) { + Some(value) => value, + None => panic!($message), + }; + NON_ZERO + }}; +} diff --git a/tests/ui/zero_test_budget.rs b/tests/ui/zero_test_budget.rs new file mode 100644 index 000000000..36bfd7cf5 --- /dev/null +++ b/tests/ui/zero_test_budget.rs @@ -0,0 +1,9 @@ +//! A zero fixture budget must fail in the same guard used by unit tests. + +include!("../support/non_zero.rs"); + +const ZERO_BUDGET: std::num::NonZeroUsize = nz!(0); + +fn main() { + let _ = ZERO_BUDGET; +} diff --git a/tests/ui/zero_test_budget.stderr b/tests/ui/zero_test_budget.stderr new file mode 100644 index 000000000..771690507 --- /dev/null +++ b/tests/ui/zero_test_budget.stderr @@ -0,0 +1,31 @@ +error[E0080]: evaluation panicked: test budget must be non-zero + --> tests/ui/../support/non_zero.rs + | + | None => panic!($message), + | ^^^^^^^^^^^^^^^^ evaluation of `ZERO_BUDGET::NON_ZERO` failed here + | + ::: tests/ui/zero_test_budget.rs:5:45 + | +5 | const ZERO_BUDGET: std::num::NonZeroUsize = nz!(0); + | ------ in this macro invocation + | + = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `nz` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> tests/ui/../support/non_zero.rs + | + | NON_ZERO + | ^^^^^^^^ + | + ::: tests/ui/zero_test_budget.rs:5:45 + | +5 | const ZERO_BUDGET: std::num::NonZeroUsize = nz!(0); + | ------ in this macro invocation + | + = note: this note originates in the macro `nz` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: erroneous constant encountered + --> tests/ui/zero_test_budget.rs:8:13 + | +8 | let _ = ZERO_BUDGET; + | ^^^^^^^^^^^ From 51fb85de329ccec334554ee589b712092868b08b Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 24 Sep 2026 06:36:42 +0200 Subject: [PATCH 4/5] Document touched test helpers and UI entry points Explain when the client callback futures mutate their counters and state. Document the trybuild runner and zero-budget entry point to address the PR's changed-function documentation warning. Keep the compile-fail snapshot aligned with the new source line. --- src/client/tests/error_handling.rs | 1 + src/client/tests/lifecycle.rs | 2 ++ tests/compile_error.rs | 1 + tests/ui/zero_test_budget.rs | 1 + tests/ui/zero_test_budget.stderr | 4 ++-- 5 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/client/tests/error_handling.rs b/src/client/tests/error_handling.rs index b3d2c280c..fb7fff79c 100644 --- a/src/client/tests/error_handling.rs +++ b/src/client/tests/error_handling.rs @@ -28,6 +28,7 @@ use crate::{ serializer::Serializer, }; +/// Increment the error-hook count when its future is first polled. fn record_error_count(count: Arc) -> impl Future + Send { lazy(move |_| { count.fetch_add(1, Ordering::SeqCst); diff --git a/src/client/tests/lifecycle.rs b/src/client/tests/lifecycle.rs index 988054157..48ae6747c 100644 --- a/src/client/tests/lifecycle.rs +++ b/src/client/tests/lifecycle.rs @@ -12,6 +12,7 @@ use futures::future::lazy; use super::helpers::{counting_hook, test_error_hook_on_disconnect, test_with_client}; +/// Increment a lifecycle-hook count when its future is first polled. fn record_count(count: Arc) -> impl Future + Send { lazy(move |_| { count.fetch_add(1, Ordering::SeqCst); @@ -25,6 +26,7 @@ async fn record_setup(count: Arc) -> &'static str { "state" } +/// Store the setup state when the teardown callback future is first polled. fn record_teardown_state(value: Arc, state: usize) -> impl Future + Send { lazy(move |_| { value.store(state, Ordering::SeqCst); diff --git a/tests/compile_error.rs b/tests/compile_error.rs index ec15c6086..28e8b36d2 100644 --- a/tests/compile_error.rs +++ b/tests/compile_error.rs @@ -1,5 +1,6 @@ //! Compile-time trait-bound and public-default tests. +/// Check public type contracts and reject a zero fixture budget at compile time. #[test] fn compile_tests() { let t = trybuild::TestCases::new(); diff --git a/tests/ui/zero_test_budget.rs b/tests/ui/zero_test_budget.rs index 36bfd7cf5..09dacaa1e 100644 --- a/tests/ui/zero_test_budget.rs +++ b/tests/ui/zero_test_budget.rs @@ -4,6 +4,7 @@ include!("../support/non_zero.rs"); const ZERO_BUDGET: std::num::NonZeroUsize = nz!(0); +/// Reference the invalid fixture so const evaluation reports the guard failure. fn main() { let _ = ZERO_BUDGET; } diff --git a/tests/ui/zero_test_budget.stderr b/tests/ui/zero_test_budget.stderr index 771690507..560af9393 100644 --- a/tests/ui/zero_test_budget.stderr +++ b/tests/ui/zero_test_budget.stderr @@ -25,7 +25,7 @@ note: erroneous constant encountered = note: this note originates in the macro `nz` (in Nightly builds, run with -Z macro-backtrace for more info) note: erroneous constant encountered - --> tests/ui/zero_test_budget.rs:8:13 + --> tests/ui/zero_test_budget.rs:9:13 | -8 | let _ = ZERO_BUDGET; +9 | let _ = ZERO_BUDGET; | ^^^^^^^^^^^ From 08c834f76b138757d41b9722ef93ea3019b628d5 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 27 Sep 2026 04:56:48 +0200 Subject: [PATCH 5/5] Document non-zero test fixtures Explain both nz! forms, their compile-time zero check, and the shared include sites. Record the trybuild UI contract so new fixture values retain compile-fail coverage. --- docs/developers-guide.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index f57103ac9..b46b80499 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -1162,6 +1162,22 @@ describes one or more scenarios using the standard Given/When/Then syntax. Scenario functions are annotated with `#[scenario(path = "…", name = "…")]` and receive fixture parameters by name. +### Non-zero test values + +[`tests/support/non_zero.rs`](../tests/support/non_zero.rs) defines the shared +`nz!` macro for test fixtures that require a `NonZeroUsize`. Use `nz!(value)` +for the default diagnostic or `nz!(value, "message")` to identify the fixture. +The macro constructs the value in a constant expression, so a zero argument is +rejected at compile time. + +The message-assembler budget tests in +[`src/message_assembler/budget_tests.rs`](../src/message_assembler/budget_tests.rs) +and the fragment adapter tests in +[`src/fragment/tests/adapter_tests.rs`](../src/fragment/tests/adapter_tests.rs) +include this shared helper. Keep zero-value compile-fail coverage in +`tests/ui/`: the UI source includes the helper, and its case is registered in +`tests/compile_error.rs` with the expected `.stderr` diagnostic committed. + ### trybuild compile-time tests Compile-time API contracts live in