Add advanced fuzzing tests for push & stream handling#155
Conversation
Reviewer's GuideIntroduce an advanced-tests feature flag with proptest and loom dependencies, update documentation, add fuzzing and concurrency tests under a new tests/advanced directory, and schedule a daily GitHub Actions workflow to run these tests. Class diagram for advanced test modules and dependenciesclassDiagram
class interaction_fuzz {
+proptest::proptest
+test_push_response()
+test_stream_response()
}
class concurrency_loom {
+loom::model
+test_concurrent_push_delivery()
}
class proptest {
<<external crate>>
}
class loom {
<<external crate>>
}
interaction_fuzz ..> proptest : uses
concurrency_loom ..> loom : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughA new advanced testing workflow is introduced, featuring property-based and concurrency tests using Loom and proptest. The Cargo manifest is updated to support these tools and a new feature flag. Documentation is updated to reflect completed test coverage. Two new advanced test files are added, targeting concurrent and fuzzed interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Runner
participant Rust Toolchain
participant Test Suite
GitHub Actions->>Runner: Trigger advanced-tests workflow
Runner->>Rust Toolchain: Install Rust, rustfmt, clippy
Runner->>Runner: Cache Cargo dependencies
Runner->>Test Suite: Run cargo test --features advanced-tests (RUSTFLAGS="--cfg loom")
Test Suite-->>Runner: Execute advanced concurrency and fuzz tests
Runner-->>GitHub Actions: Report test results
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
🧬 Code Graph Analysis (2)tests/advanced/concurrency_loom.rs (3)
tests/advanced/interaction_fuzz.rs (3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (6)
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/advanced/interaction_fuzz.rs:36` </location>
<code_context>
+proptest! {
</code_context>
<issue_to_address>
Consider adding tests for empty and maximal input cases.
Explicitly test cases with an empty actions vector and with the maximum allowed size (5 high, 5 low, 5 stream frames) to ensure correct handling of boundary conditions.
Suggested implementation:
```rust
proptest! {
#[test]
fn random_push_and_stream(actions in actions_strategy()) {
let rt = tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()
.unwrap();
rt.block_on(async {
let (queues, handle) = PushQueues::bounded(16, 16);
let shutdown = CancellationToken::new();
});
}
}
// Test with empty actions vector
#[tokio::test]
async fn test_empty_actions_vector() {
let (queues, handle) = PushQueues::bounded(16, 16);
let shutdown = CancellationToken::new();
let actions: Vec<Action> = Vec::new();
// Insert logic to process actions as in the property-based test
for action in actions {
// No actions to process, but this ensures no panics or errors
match action {
Action::High(n) => { /* handle high */ }
Action::Low(n) => { /* handle low */ }
Action::Stream(frames) => { /* handle stream */ }
}
}
// Optionally assert state if needed
}
// Test with maximal input case (5 high, 5 low, 5 stream frames)
#[tokio::test]
async fn test_maximal_actions_vector() {
let (queues, handle) = PushQueues::bounded(16, 16);
let shutdown = CancellationToken::new();
let mut actions: Vec<Action> = Vec::new();
for n in 0..5 {
actions.push(Action::High(n));
}
for n in 0..5 {
actions.push(Action::Low(n));
}
let stream_frames = (0..5).collect::<Vec<_>>();
actions.push(Action::Stream(stream_frames));
// Insert logic to process actions as in the property-based test
for action in actions {
match action {
Action::High(n) => { /* handle high */ }
Action::Low(n) => { /* handle low */ }
Action::Stream(frames) => { /* handle stream */ }
}
}
// Optionally assert state if needed
}
```
- You may need to replace the comment blocks with the actual logic for processing actions, as used in your property-based test.
- If there are assertions or state checks in the property-based test, consider adding them to these new tests as well.
- Ensure that `Action`, `PushQueues`, and `CancellationToken` are in scope for these new tests.
</issue_to_address>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}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/advanced-tests.yml(1 hunks)Cargo.toml(1 hunks)docs/asynchronous-outbound-messaging-roadmap.md(1 hunks)tests/advanced/concurrency_loom.rs(1 hunks)tests/advanced/interaction_fuzz.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
`docs/**/*.md`: Use the markdown files within the `docs/` directory as a knowled...
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Documentation in thedocs/directory must use en-GB-oxendict spelling and grammar, except for the word 'license'.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.md
`**/*.md`: Validate Markdown files using `markdownlint *.md **/*.md`. Run `mdfor...
**/*.md: Validate Markdown files usingmarkdownlint *.md **/*.md.
Runmdformat-allafter any documentation changes to format all Markdown files and fix table markup.
Validate Markdown Mermaid diagrams using thenixieCLI by runningnixie *.md **/*.md.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown files must be wrapped at 120 columns.
Tables and headings in Markdown files must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.md
`docs/*`: Each file in the docs/ directory provides design, roadmap, testing, op...
docs/*: Each file in the docs/ directory provides design, roadmap, testing, operations, or reference documentation for the project. Use these files as a reference for understanding the project's architecture, development plans, testing strategies, operational guidelines, and documentation conventions.
📄 Source: CodeRabbit Inference Engine (docs/contents.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.md
`docs/**/*.md`: Use British English spelling based on the Oxford English Diction...
docs/**/*.md: Use British English spelling based on the Oxford English Dictionary, except retain US spelling in API names (e.g., 'color').
Use the Oxford comma in lists.
Write headings in sentence case and use Markdown heading levels in order without skipping.
Follow markdownlint recommendations for Markdown formatting.
Always use fenced code blocks with a language identifier; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change.
Prefer inline links using 'text' or angle brackets around the URL.
Expand any uncommon acronym on first use, e.g., Continuous Integration (CI).
Wrap paragraphs at 80 columns, code at 120 columns, and do not wrap tables.
Use footnotes referenced with '[^label]'.
When embedding figures, use '' and provide concise alt text describing the content.
Add a short description before each Mermaid diagram for screen readers.
📄 Source: CodeRabbit Inference Engine (docs/documentation-style-guide.md)
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.md
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-oxendic spelling and grammar.
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
docs/asynchronous-outbound-messaging-roadmap.md
`Cargo.toml`: Use explicit version ranges in `Cargo.toml` and keep dependencies up-to-date.
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
Cargo.toml
`**/*.rs`: Every Rust module must begin with a module level (`//!`) comment expl...
**/*.rs: Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
tests/advanced/concurrency_loom.rstests/advanced/interaction_fuzz.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments must use en-GB-oxendict spelling and grammar.
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/advanced/concurrency_loom.rstests/advanced/interaction_fuzz.rs
🧬 Code Graph Analysis (2)
tests/advanced/concurrency_loom.rs (3)
tests/push_policies.rs (1)
rt(47-52)tests/connection_actor.rs (1)
queues(18-18)src/push.rs (1)
bounded(135-149)
tests/advanced/interaction_fuzz.rs (3)
tests/push_policies.rs (1)
rt(47-52)tests/connection_actor.rs (1)
queues(18-18)src/push.rs (3)
bounded(135-149)push_high_priority(66-72)push_low_priority(81-87)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test
🔇 Additional comments (7)
Cargo.toml (1)
24-25: LGTM! Feature flag correctly enables advanced tests.The
advanced-testsfeature flag properly gates the new testing functionality and aligns with the PR objectives.docs/asynchronous-outbound-messaging-roadmap.md (1)
36-37: LGTM! Accurately reflects completed test coverage.The documentation update correctly marks the stream and push delivery tests as complete, aligning with the newly implemented advanced tests.
.github/workflows/advanced-tests.yml (1)
1-24: LGTM! Well-structured workflow for advanced testing.The workflow correctly configures:
- Manual and scheduled triggers for flexibility
- Proper permissions and environment setup
- Required flags for loom testing (
RUSTFLAGS="--cfg loom")- Secure action versions using commit hashes
This ensures the advanced tests run regularly and can be triggered on demand.
tests/advanced/concurrency_loom.rs (1)
11-54: LGTM! Comprehensive concurrency test using loom.The test effectively validates concurrent push delivery by:
- Using loom model checking for deterministic concurrency testing
- Testing both high and low priority message delivery
- Properly using loom-compatible synchronisation primitives
- Verifying that both concurrent pushes are successfully delivered
The bounded queues with capacity 1 create backpressure scenarios that help expose potential race conditions.
tests/advanced/interaction_fuzz.rs (3)
19-34: LGTM! Well-designed proptest strategy for action generation.The strategy effectively generates diverse combinations of high priority, low priority, and stream actions with reasonable bounds (0-5 elements each).
36-82: LGTM! Comprehensive property-based testing of push and stream interactions.The test effectively validates the integration of:
- Random sequences of push operations
- Stream frame handling with proper replacement semantics
- Correct output ordering and completeness
The async runtime setup and proptest integration are implemented correctly.
65-79: Ordering assumptions validated:tokio::select! biasedenforces High → Low → StreamBoth the push loop and the connection actor use a biased
tokio::select!to process high-priority messages first, then low-priority, and finally streamed responses:
- src/push.rs
tokio::select! { biased; res = self.high_priority_rx.recv() => … // High res = self.low_priority_rx.recv() => … // Low }- src/connection.rs
tokio::select! { biased; () = Self::wait_shutdown(...), if state.is_active() => … res = Self::recv_push(self.high_rx.as_mut(), …) => … // High res = Self::recv_push(self.low_rx.as_mut(), …) => … // Low res = Self::next_response(&mut self.response), if … => … // Stream }The test’s construction of
expected(high frames, then low frames, then stream frames) matches this implementation.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
Cargo.toml (1)
21-22: LGTM! Explicit version ranges implemented correctly.The explicit version ranges using caret notation (^1.0, ^0.7) properly address the coding guidelines requirement for explicit version ranges in Cargo.toml.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Cargo.toml(1 hunks)tests/advanced/interaction_fuzz.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`Cargo.toml`: Use explicit version ranges in `Cargo.toml` and keep dependencies up-to-date.
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
Cargo.toml
`**/*.rs`: Every Rust module must begin with a module level (`//!`) comment expl...
**/*.rs: Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
📄 Source: CodeRabbit Inference Engine (AGENTS.md)
List of files the instruction was applied to:
tests/advanced/interaction_fuzz.rs
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments must use en-GB-oxendict spelling and grammar.
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/advanced/interaction_fuzz.rs
🧬 Code Graph Analysis (1)
tests/advanced/interaction_fuzz.rs (2)
tests/connection_actor.rs (1)
queues(18-18)src/push.rs (1)
bounded(135-149)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-test
- GitHub Check: Analyze (actions)
🔇 Additional comments (2)
Cargo.toml (1)
24-25: Feature flag implementation looks good.The
advanced-testsfeature flag is properly configured as an empty feature to gate the advanced test suites.tests/advanced/interaction_fuzz.rs (1)
48-51: Verify stream overwriting behaviour is intentional.The logic overwrites previous stream data when multiple
Streamactions occur. This matches the implementation inrun_actionsbut may be worth documenting.Please confirm this behaviour is intentional - should multiple streams be concatenated instead of overwritten?
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
Testing
make fmtmake lintmake testnixierun attempted but failed due to event loop errorshttps://chatgpt.com/codex/tasks/task_e_6861972605248322ac1de3f9ba0bf9f5
Summary by Sourcery
Add advanced fuzzing and concurrency tests for push and stream handling, update the roadmap to mark them complete, and schedule daily runs via GitHub Actions
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests: