Decompose oversized fragment transport test into modular files#425
Merged
Decompose oversized fragment transport test into modular files#425
Conversation
Decompose the 513-line test file into a modular structure: - Extract TestError and shared helpers to tests/common/fragment_helpers.rs - Move rejection tests (FragmentRejectionSetup, mutators, parameterized cases) to tests/fragment_transport/rejection.rs - Move eviction test to tests/fragment_transport/eviction.rs - Keep round-trip tests and API toggle test in the main file The main file is now 150 lines, well under the 400-line guideline. All quality gates pass (check-fmt, lint, test). Closes #401 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Reviewer's GuideRefactors the monolithic fragment transport integration test file into a small shared helper module plus two focused test modules for rejection and eviction behavior, without changing core runtime logic. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughSummarise the PR: add a shared test helper module for fragment-transport tests and split the large fragmentation test into focused submodules (eviction, rejection) that reuse the new helpers for setup, fragmentation, I/O and assertions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Fragmenter as Fragmenter
participant WireframeApp as WireframeApp
participant Reassembler as Reassembler
participant Handler as Handler
Client->>Fragmenter: fragment_envelope(request Envelope)
Client->>WireframeApp: send_envelopes(framed stream)
WireframeApp->>Reassembler: receive fragment frames
Reassembler->>Reassembler: attempt reassembly / timeout eviction
Reassembler-->>Handler: deliver reassembled payload (if valid)
Handler-->>WireframeApp: produce response Envelope
WireframeApp-->>Client: framed response (possibly fragmented)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![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 left some high level feedback:
- In
tests/fragment_transport/rejection.rs, several items (FragmentRejectionSetup,test_fragment_rejection,FragmentMutator, and the mutator functions) are declaredpubbut appear to be used only within this module; consider reducing their visibility topub(crate)or private to avoid exposing unnecessary test API surface. - Now that
spawn_appexists intests/common/fragment_helpers.rs, theexpired_fragments_are_evictedtest intests/fragment_transport/eviction.rscould be simplified by reusingspawn_appinstead of duplicating the codec/duplex setup logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `tests/fragment_transport/rejection.rs`, several items (`FragmentRejectionSetup`, `test_fragment_rejection`, `FragmentMutator`, and the mutator functions) are declared `pub` but appear to be used only within this module; consider reducing their visibility to `pub(crate)` or private to avoid exposing unnecessary test API surface.
- Now that `spawn_app` exists in `tests/common/fragment_helpers.rs`, the `expired_fragments_are_evicted` test in `tests/fragment_transport/eviction.rs` could be simplified by reusing `spawn_app` instead of duplicating the codec/duplex setup logic.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 3
🤖 Fix all issues with AI agents
In `@tests/common/fragment_helpers.rs`:
- Around line 228-241: The function assert_handler_observed uses assert_eq!
which panics inside a TestResult-returning function; change it to perform an
explicit equality check on the observed and expected slices and on mismatch
return Err(TestError::Assertion(...)) instead of panicking. Locate
assert_handler_observed (params rx, expected) and replace the assert_eq! block
with an if observed != expected { return
Err(TestError::Assertion(format!("observed payload mismatch: expected
{expected:?}, got {observed:?}"))); } so the function returns a
TestError::Assertion on failure and still returns Ok(()) on success.
In `@tests/fragment_transport.rs`:
- Around line 3-7: Update the module-level comment at the top of
tests/fragment_transport.rs: replace the British spelling "organised" with the
Oxford -ize form "organized" in the overview sentence so it reads "Tests are
organized into submodules by concern:"; no code changes beyond that comment edit
are required.
In `@tests/fragment_transport/mod.rs`:
- Around line 1-5: Update the module-level doc comment at the top of the file
(the //! comment block in tests/fragment_transport/mod.rs) to use Oxford -ize
spelling: replace the word "organisation" with "organization" in the first line
so the docstring reads "Module organization for fragment transport integration
tests."
- Use Oxford -ize spelling ('organized', 'organization') per coding
guidelines in module doc comments
- Replace assert_eq! with conditional TestError::Assertion return in
assert_handler_observed to avoid triggering clippy::panic_in_result_fn
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
- Use Oxford -ize spelling ('organized', 'organization') per coding
guidelines in module doc comments
- Replace assert_eq! with conditional TestError::Assertion return in
assert_handler_observed to avoid triggering clippy::panic_in_result_fn
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/fragment_transport.rs (1)
42-64: Fix the panic-inducing assertion on line 64 to return an explicit error.The function
run_round_trip_testreturnsTestResult<Vec<u8>>and usesassert_eq!on line 64, which triggers thepanic_in_result_fnlint (denied in Cargo.toml). Replace the assertion with explicit error handling:Fix
- assert_eq!(response, payload); + if response != payload { + return Err(TestError::Assertion(format!( + "observed payload mismatch: expected {payload:?}, got {response:?}" + )) + .into()); + }Per coding guidelines: Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
🤖 Fix all issues with AI agents
In `@tests/common/fragment_helpers.rs`:
- Around line 84-105: Add missing `# Errors` sections to all public helper
functions that return TestResult (e.g., fragmentation_config and
fragmentation_config_with_timeout) describing when they return TestError::Setup
(message limit zero, frame budget failure, or invalid timeout), and add short
doc comments for the helper functions that currently lack any docs (the public
helpers around the noted lines). Ensure each existing doc comment that describes
behavior also includes a `# Errors` block that mentions the specific error
variant(s) (TestError::Setup) and the conditions that trigger them so Clippy's
missing_errors_doc lint is satisfied.
- Around line 174-186: Add a doc comment and #[must_use] to the make_handler
factory: update the function signature for make_handler (returning
Handler<Envelope>) to include the #[must_use] attribute and add a Rust doc
comment that describes the return value should not be ignored and includes a "#
Panics" section explaining that the closure assert!(tx.send(payload).is_ok())
will panic if sending to the channel fails; ensure the doc mentions that the
produced handler must be used by callers so the assert can run.
- Use Oxford -ize spelling ('organized', 'organization') per coding
guidelines in module doc comments
- Replace assert_eq! with conditional TestError::Assertion return in
assert_handler_observed and run_round_trip_test to avoid triggering
clippy::panic_in_result_fn
- Reduce visibility of test-internal items in rejection.rs to private
- Simplify eviction.rs by reusing spawn_app helper
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add # Errors sections to all Result-returning public functions and # Panics section to make_handler. Add #[must_use] attribute to make_handler to satisfy Clippy lints. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/fragment_transport.rs (2)
105-155: Refactor asserts to error returns and remove the lint expectation.Replace
assert_eq!andassert!withTestError::Assertion-returning logic to eliminate the unnecessaryclippy::panic_in_result_fnsuppression. This pattern is already used elsewhere in the test suite (e.g.,run_round_trip_test).Proposed fix
-#[expect( - clippy::panic_in_result_fn, - reason = "asserts provide clearer diagnostics in tests" -)] async fn fragmentation_can_be_disabled_via_public_api() -> TestResult { @@ - assert_eq!( - observed, payload, - "observed payload mismatch: expected {payload:?}, got {observed:?}" - ); + if observed != payload { + return Err(TestError::Assertion(format!( + "observed payload mismatch: expected {payload:?}, got {observed:?}" + )) + .into()); + } @@ - assert!( - decode_fragment_payload(&response)?.is_none(), - "expected no fragmentation when fragmentation is disabled" - ); + if decode_fragment_payload(&response)?.is_some() { + return Err(TestError::Assertion( + "expected no fragmentation when fragmentation is disabled".to_string(), + ) + .into()); + }
84-103: Remove the#[expect(...)]lint suppression and useTestError::Assertioninstead ofassert!()to comply with the coding guideline prohibiting lint silencing.Replace the panic-based assertion with an explicit error return. The
TestError::Assertionvariant is already defined and used successfully in helper functions elsewhere in the codebase.Proposed fix
-#[tokio::test] -#[expect( - clippy::panic_in_result_fn, - reason = "asserts provide clearer diagnostics in tests" -)] async fn unfragmented_request_and_response_round_trip() -> TestResult { let buffer_capacity = 512; let config = fragmentation_config(buffer_capacity)?; let cap = config.fragment_payload_cap.get(); let payload_len = cap.saturating_sub(8).max(1); let payload = vec![b's'; payload_len]; let response = run_round_trip_test(buffer_capacity, payload, false).await?; - assert!( - decode_fragment_payload(&response)?.is_none(), - "small payload should pass through unfragmented" - ); + if decode_fragment_payload(&response)?.is_some() { + return Err(TestError::Assertion( + "small payload should pass through unfragmented".to_string(), + ) + .into()); + } Ok(()) }
🤖 Fix all issues with AI agents
In `@tests/common/fragment_helpers.rs`:
- Around line 81-100: The function fragmentation_config currently uses
saturating_mul(16) which hides overflow; change the message_limit computation to
use checked_mul(16).and_then(NonZeroUsize::new) and map None to the existing
TestError::Setup("non-zero message limit") so overflow or zero yields an error;
keep the subsequent call to FragmentationConfig::for_frame_budget(...) and its
TestError::Setup("frame budget must exceed fragment overhead") unchanged.
- Use checked_mul instead of saturating_mul in fragmentation_config to properly implement documented overflow error behaviour - Remove #[expect(clippy::panic_in_result_fn)] lint suppressions - Replace assert!/assert_eq! with TestError::Assertion returns in unfragmented_request_and_response_round_trip and fragmentation_can_be_disabled_via_public_api tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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
Why
Test plan
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/cd1c2264-724a-4112-abd1-037d5231f12a