Merged
Conversation
The build and release targets incorrectly referenced target files without the .rlib extension, causing make target enumeration to fail. Update to match the actual pattern rule that creates .rlib files.
Add rstest-bdd and rstest-bdd-macros dependencies with compile-time validation enabled. Create tests/bdd/ directory structure with fixtures, steps, and scenarios modules. Update Cargo.toml with bdd test target and Makefile with test-bdd and test-cucumber targets. Phase 0 complete: parallel infrastructure ready without disrupting existing Cucumber tests.
Convert CorrelationWorld from Cucumber to rstest-bdd: - Create correlation fixture with rstest #[fixture] annotation - Migrate step definitions from async to sync with separate Runtime - Create scenario tests (sync, not async) - All 3 correlation scenarios passing Key discovery: Scenarios must be sync (not async) to allow steps to create their own tokio::Runtime for async operations. The async scenario approach from the plan doesn't work due to 'Cannot start runtime within runtime' error. Steps create Runtime::new() and use block_on() instead.
Update execplan with critical discovery from CorrelationWorld migration: - Scenarios must be sync (not async) to avoid runtime-within-runtime error - Steps create Runtime::new() for async operations - Remove all #[tokio::test] references from plan examples - Update progress: Phase 0 complete, Phase 1 in progress (1/2 done)
Complete Phase 1 of the migration plan with the second pilot world. This migration demonstrates the pattern for synchronous worlds without async operations. ## Changes - Created `tests/bdd/fixtures/request_parts.rs` with `RequestPartsWorld` fixture - Created `tests/bdd/steps/request_parts_steps.rs` with all step definitions - Created `tests/bdd/scenarios/request_parts_scenarios.rs` with 6 scenarios - Updated module files to include new request_parts modules - Fixed doc_markdown clippy lints across all BDD files - Added module-level `#[expect(unused_braces)]` to fixture files to suppress clippy/rustfmt conflict on single-line fixture functions ## Test Results All 6 request_parts scenarios pass, matching Cucumber output: - Create request parts with all fields - Request parts inherit missing correlation id - Request parts override mismatched correlation id - Request parts preserve correlation when source is absent - Empty metadata is valid - Metadata can be modified after construction Both frameworks report: 6 scenarios (6 passed), 20 steps (20 passed) ## Migration Notes RequestPartsWorld is purely synchronous with no async operations, so no Runtime creation is needed in steps. All step functions are simple sync wrappers around World methods. Phase 1 is now complete. Ready to proceed with Phase 2.
Begin Phase 2 with the first medium-complexity world. PanicWorld demonstrates the server spawning pattern with panic resilience testing. ## Changes - Created `tests/bdd/fixtures/panic.rs` with `PanicWorld` and `PanicServer` helper - Created `tests/bdd/steps/panic_steps.rs` with async step definitions using `Runtime::new()` - Created `tests/bdd/scenarios/panic_scenarios.rs` with 1 scenario - Updated module files to include new panic modules ## Test Results The panic resilience scenario passes, matching Cucumber output: - Cucumber: 1 scenario (1 passed), 4 steps (4 passed) - rstest-bdd: 1 scenario (1 passed), 4 steps (4 passed) ## Migration Notes PanicWorld demonstrates: - Server spawning with `WireframeServer` and custom panic handler - Helper struct (`PanicServer`) with Drop implementation for cleanup - Async world methods requiring `Runtime::new()` in steps - Connection counting across multiple "When" steps All tests passing. First world of Phase 2 complete.
Continue Phase 2 with the second medium-complexity world. MultiPacketWorld demonstrates channel operations, message ordering, and back-pressure handling. ## Changes - Created `tests/bdd/fixtures/multi_packet.rs` with `MultiPacketWorld` and `WireframeRunError` helper - Created `tests/bdd/steps/multi_packet_steps.rs` with async/sync step mix - Created `tests/bdd/scenarios/multi_packet_scenarios.rs` with 3 scenarios - Updated module files to include new multi_packet modules ## Test Results All 3 multi-packet scenarios pass, matching Cucumber output: - Cucumber: 3 scenarios (3 passed), 6 steps (6 passed) - rstest-bdd: 3 scenarios (3 passed), 6 steps (6 passed) ## Migration Notes MultiPacketWorld demonstrates: - Channel operations with `Response::with_channel` and mpsc channels - Back-pressure handling with channel capacity overflow - Mixed async/sync step definitions (async "when" steps, sync "then" steps) - Custom error type wrapping (`WireframeRunError`) - Spawned tasks with `tokio::spawn` for concurrent message production All tests passing. Second world of Phase 2 complete.
Reflect current rstest-bdd progress and align examples with the\nasync-scenario guidance from the user guide. This keeps the\nplan in sync with the codebase and removes outdated patterns.
Add StreamEnd fixtures, steps, and scenarios to the rstest-bdd\nstack and wire the terminator helper into the bdd test crate.\nUpdate the migration execplan to reflect Phase 2 progress.
Add codec stateful fixtures, steps, and scenarios to the bdd\ntest suite and update the execplan to mark Phase 2 complete.
Add client runtime fixtures, steps, and scenarios to the bdd\ntest suite and update the execplan to record Phase 3\nprogress.
Add client messaging fixtures, steps, and scenarios to the bdd\nsuite and update the migration execplan to record Phase 3\nprogress.
Add client lifecycle fixtures, steps, and scenarios to the bdd\nsuite and update the execplan to mark Phase 3 progress.
Add rstest-bdd fixture, steps, and scenarios for client preamble behaviours, keeping async work inside per-step runtimes. Disambiguate client preamble step text so cucumber and rstest-bdd registrations do not clash, and update the execplan status and feature-file guidance accordingly.
Add rstest-bdd fixtures, steps, and scenarios for message assembler header parsing, and use async scenarios for sync-only steps. Disambiguate metadata length wording to avoid step registry clashes, update the execplan progress, and remove the now-unused dead_code expectation for TestApp.
Add message assembly fixtures (including shared param types), steps, and scenario bindings, using async scenarios for sync-only steps. Update execplan progress to reflect the second Phase 4 world.
Add codec error fixtures, decoder operations, steps, and scenarios to cover taxonomy and recovery policy behaviour, using async scenarios for sync-only steps. Update execplan progress for Phase 4.
Add fragment fixtures, async rstest-bdd steps, and scenarios covering fragment series, reassembly, and overflow cases. Update fragment feature wording and cucumber steps to keep step definitions unambiguous, and mark progress in the execplan.
Record the scenario parity check between cucumber and rstest-bdd in the migration execplan.
Switch rstest-bdd-macros to strict compile-time validation and record the change in the migration execplan.
Add the hyperfine comparison outcome to the migration execplan.
Delete the cucumber runner, worlds, and step definitions, and remove the cucumber dependency/feature and Makefile target. Update integration test docs and execplan notes to reflect the rstest-bdd-only workflow.
Move fixtures, steps, and scenarios under tests/ and update module paths and documentation to match the new layout.
Contributor
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughSummarise: Replace the Cucumber BDD harness with rstest‑bdd — add fixtures, scenarios and synchronous step wrappers that create a Tokio runtime and block_on async world methods; remove the old Cucumber runner, world re‑exports and cucumber dev‑deps; update docs, Makefile and Cargo test entries. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test runner / Cargo test
participant Scenario as rstest-bdd scenario
participant StepFn as Step function (sync wrapper)
participant Tokio as Tokio Runtime
participant World as Fixture (async impl)
participant App as App / Server under test
Note over TestRunner,Scenario: invoke scenario
TestRunner->>Scenario: run scenario
Scenario->>StepFn: call step function
StepFn->>Tokio: create runtime / block_on
Tokio->>World: execute async step method
World->>App: perform async interactions (start/connect/send/verify)
App-->>World: return results/state
World-->>Tokio: return TestResult
Tokio-->>StepFn: deliver TestResult
StepFn-->>Scenario: return
Scenario-->>TestRunner: report outcome
style TestRunner fill:rgba(52,152,219,0.5) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
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 |
Contributor
Reviewer's GuideReplaces the legacy Cucumber-based BDD test harness with an rstest-bdd-based setup, introducing dedicated fixtures and scenario modules, updating step implementations and feature wording, raising the MSRV and Rust edition, wiring in rstest-bdd tooling (CLI, language server, strict macro validation), and cleaning up build, docs, and Makefile to standardize on the new framework. Sequence diagram for rstest_bdd language server navigation and diagnosticssequenceDiagram
actor Dev as Developer
participant Editor
participant LSP as rstest_bdd_lsp
participant Cargo as cargo_metadata
participant Reg as StepDefinitionRegistry
Dev->>Editor: Open workspace and feature/Rust files
Editor->>LSP: initialize
LSP->>Cargo: cargo metadata
Cargo-->>LSP: workspace packages
LSP-->>Editor: initialize result
loop On Rust file save
Editor->>LSP: textDocument/didSave(.rs)
LSP->>LSP: parse Rust file with syn
LSP->>Reg: update step definitions for file
LSP->>Reg: compile patterns per keyword
LSP->>LSP: recompute diagnostics for all features
LSP-->>Editor: publishDiagnostics for .rs and .feature
end
loop On feature file save
Editor->>LSP: textDocument/didSave(.feature)
LSP->>LSP: parse feature with gherkin
LSP->>Reg: query matching step definitions per keyword
LSP->>LSP: find unimplemented steps and table/docstring mismatches
LSP-->>Editor: publishDiagnostics for .feature
end
Dev->>Editor: Go to Implementation on feature step
Editor->>LSP: textDocument/implementation
LSP->>Reg: find matching compiled patterns
Reg-->>LSP: matching step definitions
LSP-->>Editor: locations of Rust step functions
Editor-->>Dev: jump to Rust implementation
Sequence diagram for cargo_bdd CLI querying skipped steps and scenariossequenceDiagram
actor Dev as Developer
participant CLI as cargo_bdd
participant Cargo as Cargo_test_runner
participant Test as bdd_test_binaries
participant RBdd as rstest_bdd_runtime
Dev->>CLI: cargo bdd steps --skipped --json
CLI->>Cargo: build all test targets
Cargo-->>CLI: test binaries
CLI->>Test: run with RSTEST_BDD_DUMP_STEPS=1 --dump-steps
Test->>RBdd: execute scenarios
RBdd-->>Test: ScenarioRecord list with ScenarioMetadata and ScenarioStatus
Test-->>CLI: merged JSON dump of step and skip data
CLI->>CLI: filter to skipped steps with context
CLI-->>Dev: JSON objects with feature, scenario, line, tags, reason, step
Dev->>CLI: cargo bdd skipped --reasons --json
CLI->>Test: run test binaries with dump flags
Test-->>CLI: scenario outcome snapshot
CLI->>CLI: extract skipped ScenarioRecord entries
CLI-->>Dev: JSON list of skipped scenarios with reasons and locations
Class diagram for migrated BDD test structure using rstest_bddclassDiagram
class ClientMessagingWorld {
+addr
+server
+client
+envelope
+sent_correlation_ids
+response
+last_error
+start_echo_server()
+connect_client()
}
class CorrelationWorld {
+expected
+frames
+set_expected(expected)
+process()
+verify()
}
class RequestPartsWorld {
+request_parts_state
+validate()
}
class ScenarioStateTrait {
<<trait>>
}
class Slot~T~ {
+set(value)
+get() T
+take() T
}
class TestResult {
<<type_alias>>
}
class StepResult~T~ {
<<alias_Result>>
}
class ScenarioMetadata {
+feature_path
+scenario_name
+line
+tags
+new(feature_path, scenario_name, line, tags)
}
class ScenarioRecord {
+from_metadata(metadata, status)
}
class ScenarioStatus {
<<enum>>
Skipped
Passed
Failed
}
class SkippedScenario {
+new(reason, has_messages, is_filtered)
}
class StepExecution {
+skipped(message)
}
class StepDefinitionRegistry {
+steps_for_keyword(keyword)
+steps_for_file(path)
}
class CompiledStepDefinition {
+keyword
+pattern
+parameters
+expects_datatable
+expects_docstring
}
class RstestBddMacros {
<<crate>>
+given
+when
+then
+scenario
+scenarios
}
class RstestBddRuntime {
<<crate>>
+StepContext
+StepExecution
+StepResult
}
class RstestBddServer {
<<crate>>
+LanguageServer
}
class LanguageServer {
+initialize()
+initialized()
+shutdown()
+text_document_did_save()
+goto_definition()
+goto_implementation()
+publish_diagnostics()
}
class CargoBddCli {
+steps()
+steps_skipped(json)
+unused()
+duplicates()
+skipped(reasons, json)
}
ScenarioStateTrait <|.. ClientMessagingWorld
ScenarioStateTrait <|.. CorrelationWorld
ScenarioStateTrait <|.. RequestPartsWorld
Slot --> ClientMessagingWorld
RstestBddRuntime --> StepExecution
RstestBddRuntime --> StepResult
StepExecution --> ScenarioRecord
ScenarioMetadata --> ScenarioRecord
ScenarioStatus --> ScenarioRecord
SkippedScenario --> ScenarioStatus
StepDefinitionRegistry --> CompiledStepDefinition
RstestBddServer --> LanguageServer
CargoBddCli --> ScenarioRecord
CargoBddCli --> ScenarioStatus
RstestBddMacros --> ClientMessagingWorld
RstestBddMacros --> CorrelationWorld
RstestBddMacros --> RequestPartsWorld
Flow diagram for phased migration from Cucumber to rstest_bddflowchart TD
A[Phase_0_Setup_parallel_rstest_bdd_infra] --> B[Phase_1_Migrate_simple_worlds_Correlation_RequestParts]
B --> C[Phase_2_Migrate_medium_worlds_Panic_MultiPacket_StreamEnd_CodecStateful]
C --> D[Phase_3_Migrate_complex_client_worlds_Runtime_Messaging_Lifecycle_Preamble]
D --> E[Phase_4_Migrate_specialized_worlds_MessageAssembler_Assembly_CodecError_Fragment]
E --> F[Phase_5_Validation_and_cleanup]
subgraph Phase_0
A1[Add_rstest_bdd_dev_dependencies]
A2[Add_bdd_test_target_in_Cargo_toml]
A3[Create_tests_bdd_fixtures_steps_scenarios]
A4[Add_Makefile_test_bdd_target]
end
A --> A1 --> A2 --> A3 --> A4
subgraph Phase_1
B1[Migrate_CorrelationWorld_to_fixture_steps_scenarios]
B2[Migrate_RequestPartsWorld_to_fixture_steps_scenarios]
end
B --> B1 --> B2
subgraph Phase_5
F1[Compare_Cucumber_and_rstest_bdd_results]
F2[Enable_strict_compile_time_validation]
F3[Remove_Cucumber_runner_worlds_steps_dependency]
F4[Simplify_Makefile_to_test_bdd_only]
end
F --> F1 --> F2 --> F3 --> F4
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.
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: tests/steps/message_assembly_steps.rs Comment on lines 85 to 96 fn when_final_continuation(
world: &mut MessageAssemblyWorld,
message_assembly_world: &mut MessageAssemblyWorld,
key: u64,
seq: u32,
sequence: u32,
body: String,
) -> TestResult {
world.accept_continuation(
message_assembly_world.accept_continuation(
ContinuationFrameParams::new(to_key(key), body.into_bytes())
.with_sequence(to_seq(seq))
.with_sequence(to_seq(sequence))
.final_frame(),
)
}❌ New issue: Code Duplication |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5302
Why This Duplication Is AcceptableEach of the four flagged functions:
Why Further Abstraction Would Harm the CodeAttempting to eliminate this structural similarity would require:
All of these approaches would make the test suite harder to understand and maintain for the sake of eliminating a small amount of structural similarity that serves a legitimate purpose. RecommendationNo further refactoring is needed. This is the same conclusion I reached for similar duplication earlier in this thread:
In all cases, some structural similarity is desirable in BDD test code because it:
Configure CodeSceneConsider adjusting CodeScene's thresholds for BDD test modules to accept this pattern. You could:
This will prevent false positives whilst still catching genuine code health issues elsewhere in the codebase. |
Add rustdoc for test fixtures and public re-exports, fix message assembly step literals, and tidy fixture helpers. Also update execplan wording to use typographical ellipses.
![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/fixtures/client_runtime.rs`:
- Around line 22-23: Replace the line comment above the public re-export with a
Rustdoc comment so the public API is documented: change the existing plain
comment to a triple-slash Rustdoc describing what TestResult is and why it’s
re-exported (referring to the symbol TestResult and the re-export statement pub
use crate::common::TestResult;), ensuring the docstring follows Rustdoc style
and documents the public re-export for downstream users.
- Around line 38-65: Replace the panic-throwing Default impl for
ClientRuntimeWorld with a fallible constructor by changing the fixture function
client_runtime_world to return TestResult<ClientRuntimeWorld> and constructing
the tokio runtime with map_err/...? (i.e., propagate the error instead of
panicking); remove or stop using ClientRuntimeWorld::default and instead build
and return Ok(ClientRuntimeWorld { runtime, addr: ..., server: ..., client: ...,
payload: ..., response: ..., last_error: ... }); then update the two scenario
functions client_runtime_send_receive and client_runtime_oversize_error to
accept client_runtime_world: TestResult<ClientRuntimeWorld> (and return
TestResult) so setup failures propagate to tests rather than panicking.
In `@tests/fixtures/message_assembler/message_assembler_asserts.rs`:
- Around line 68-78: The assertion currently leaks the primitive usize; change
assert_total_len to accept Option<BodyLength> instead of Option<usize>, create
DebugDisplay(expected) from that BodyLength-newtype, and in the closure return
DebugDisplay(header.total_body_len) (using the existing header.total_body_len
BodyLength field) so the newtype is carried through; update all call sites to
pass Option<BodyLength> values (or wrap usizes into BodyLength where needed).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/fixtures/client_preamble.rs (1)
289-297: Move the inline import to module level.The
use tokio::io::AsyncReadExt;statement inside the async block is functional but unconventional. Hoist it to the module imports for consistency with the rest of the file.🔧 Proposed diff
At line 18, add the import:
use tokio::{net::TcpListener, sync::oneshot, task::JoinHandle}; +use tokio::io::AsyncReadExt;At lines 289-297, remove the inline import:
.on_preamble_success(|_preamble, stream| { async move { - use tokio::io::AsyncReadExt; let mut buf = [0u8; 1]; stream.read_exact(&mut buf).await?; Ok(Vec::new()) } .boxed() })tests/fixtures/stream_end.rs (1)
184-198: Consolidate duplicate assertion methods.
verify()andverify_multi()have identical implementations. Extract a shared helper or have one delegate to the other to eliminate the duplication.♻️ Proposed fix
/// Verify that a terminator frame was appended to the stream. /// /// # Panics /// Panics if the expected terminator is missing. - pub fn verify(&self) { - assert_eq!(self.frames, vec![1, 2, 0]); + fn assert_terminator_present(&self) { + assert_eq!(self.frames, vec![1, 2, 0], "expected terminator frame appended"); } + /// Verify that a terminator frame was appended to the stream. + /// + /// # Panics + /// Panics if the expected terminator is missing. + pub fn verify(&self) { self.assert_terminator_present(); } + /// Verify that a multi-packet terminator frame was appended to the stream. /// /// # Panics /// Panics if the expected terminator is missing. - pub fn verify_multi(&self) { - assert_eq!(self.frames, vec![1, 2, 0]); - } + pub fn verify_multi(&self) { self.assert_terminator_present(); }
🤖 Fix all issues with AI agents
In `@docs/execplans/migrate-from-cucumber-to-rstest-bdd.md`:
- Around line 89-92: The sentence "For worlds with purely synchronous steps,
prefer async scenarios so the test body can `await` any extra async assertions
or cleanup logic." is missing a comma before the coordinating conjunction "so";
update that sentence (the bullet starting with "For worlds with purely
synchronous steps, prefer async scenarios so the test body...") to insert a
comma: "prefer async scenarios, so the test body can `await` any extra async
assertions or cleanup logic."
- Around line 40-47: Update the tier headings to use en dashes instead of
hyphens: replace "Tier 1 - Simple (115-200 lines)" with "Tier 1 – Simple
(115–200 lines)" and "Tier 2 - Medium (200-400 lines)" with "Tier 2 – Medium
(200–400 lines)"; ensure both the clause separator between the tier and
description and the numeric ranges (e.g., "115-200" and "200-400") use en dashes
(–) consistently in the document where these headings appear.
In `@tests/fixtures/client_lifecycle.rs`:
- Around line 33-34: The public re-export pub use crate::common::TestResult
currently lacks Rustdoc; replace the line comment with a Rustdoc comment
directly above the declaration so the public API is documented (add a ///
comment describing that TestResult is re-exported from crate::common for use in
steps and any short note about its purpose). This update should be applied to
the TestResult re-export line so tooling and generated docs include the
explanation.
- Around line 87-92: The fixture client_lifecycle_world unnecessarily uses
std::mem::take on a freshly created mutable world; replace the body to return
the default directly—either return ClientLifecycleWorld::default() or create let
world = ClientLifecycleWorld::default(); world—removing std::mem::take and the
mutable binding; update the function client_lifecycle_world to simply return the
default value for ClientLifecycleWorld.
In `@tests/fixtures/fragment/mod.rs`:
- Around line 58-63: Remove the #[rustfmt::skip] on the fixture function
fragment_world so rustfmt can apply project style (fn_single_line = true) and
let the function collapse to a single-line form that returns
FragmentWorld::default(); if the compiler emits an unused_braces lint after
formatting, add #[expect(unused_braces, reason = "fixture single-line
formatting")] above the fixture to silence it while preserving the standardized
formatting for fragment_world.
In `@tests/fixtures/panic.rs`:
- Around line 86-91: The fixture function panic_world unnecessarily uses
std::mem::take on a freshly created mutable PanicWorld; replace the taken value
with a direct return of PanicWorld::default() by removing the let mut world/
take sequence and returning PanicWorld::default() from the panic_world function
so the fixture mirrors the simpler pattern used in client_lifecycle.rs.
- Around line 12-14: Change the plain line comment above the public re-export to
a Rustdoc comment so the re-exported item is documented in the public API:
replace the current "// Re-export TestResult from common for use in steps" with
a triple-slash doc comment (/// ...) immediately above the pub use
crate::common::TestResult; declaration in tests/fixtures/panic.rs so the
TestResult re-export is picked up by rustdoc.
In `@tests/fixtures/stream_end.rs`:
- Around line 234-239: Remove the unnecessary rustfmt suppression and its
preceding comment from the fixture; specifically, in the stream_end_world
function (pub fn stream_end_world() -> StreamEndWorld {
StreamEndWorld::default() }), delete the #[rustfmt::skip] attribute and the
comment above it so the fixture matches other fixtures like panic_world and
client_lifecycle_world and avoids an unjustified lint suppression.
Remove rustfmt skips, simplify fixtures, and propagate client runtime init errors without panicking. Also flatten long step literals for message assembly steps.
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
🤖 Fix all issues with AI agents
In `@tests/fixtures/client_lifecycle.rs`:
- Around line 87-93: The fixture function client_lifecycle_world moves the field
world.last_error which partially moves `world` and prevents returning it; remove
the field access (drop the `_ = world.last_error;` line) and simply return the
default-initialized ClientLifecycleWorld (i.e., keep creating world via
ClientLifecycleWorld::default() but do not read or move any fields like
last_error before returning).
In `@tests/fixtures/client_runtime.rs`:
- Around line 81-87: The fixture function client_runtime_world contains a no-op
statement `let _ = world.runtime_error.as_deref();` that should be removed;
simplify the function by returning the ClientRuntimeWorld created by
ClientRuntimeWorld::new directly (i.e., remove the dummy access to
runtime_error), leaving the function body to construct and return world.
In `@tests/fixtures/fragment/mod.rs`:
- Around line 58-64: The fixture function fragment_world contains an unnecessary
dummy statement `let _ = world.series.is_none();`; remove that statement and
simply return FragmentWorld::default() (i.e., keep the function fragment_world
and the FragmentWorld::default() construction but delete the pointless `let _ =
...` line) so the fixture is concise and returns the default directly.
In `@tests/fixtures/stream_end.rs`:
- Around line 234-239: The fixture stream_end_world contains a no-op line that
calls world.frames.is_empty() and discards the result; remove that dead code and
simplify the function to return StreamEndWorld::default() directly. Update the
function body of pub fn stream_end_world() -> StreamEndWorld to a single-line
implementation that returns StreamEndWorld::default(), keeping the #[fixture]
attribute and leaving any other symbols (StreamEndWorld, frames) unchanged; run
tests (make test) after the change.
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: tests/fixtures/client_runtime.rs Comment on lines +147 to +163 pub fn send_payload(&self, size: usize) -> TestResult {
let payload = ClientPayload {
data: vec![7_u8; size],
};
let mut client = self
.client
.borrow_mut()
.take()
.ok_or("client not connected")?;
let runtime = self.runtime()?;
let response: ClientPayload = runtime.block_on(async { client.call(&payload).await })?;
*self.client.borrow_mut() = Some(client);
*self.payload.borrow_mut() = Some(payload);
*self.response.borrow_mut() = Some(response);
*self.last_error.borrow_mut() = None;
Ok(())
}❌ New issue: Code Duplication |
Contributor
|
Oops, something went wrong! Please try again later. 🐰 💔 |
Resolve open PR review comments across BDD fixtures and steps. - Deduplicate client runtime payload handling - Normalise fixture formatting to avoid unused_braces - Update message assembly steps and assertions - Tidy execplan typography
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/fixtures/stream_end.rs (1)
184-198: Consolidate identical verification methods.
verify()andverify_multi()have identical implementations. Extract the common assertion into a shared helper or delegate one to the other to reduce duplication.Proposed fix
/// Verify that a terminator frame was appended to the stream. /// /// # Panics /// Panics if the expected terminator is missing. - pub fn verify(&self) { - assert_eq!(self.frames, vec![1, 2, 0]); - } + pub fn verify(&self) { self.assert_terminator_present(); } /// Verify that a multi-packet terminator frame was appended to the stream. /// /// # Panics /// Panics if the expected terminator is missing. - pub fn verify_multi(&self) { - assert_eq!(self.frames, vec![1, 2, 0]); - } + pub fn verify_multi(&self) { self.assert_terminator_present(); } + + fn assert_terminator_present(&self) { + assert_eq!(self.frames, vec![1, 2, 0], "expected terminator frame missing"); + }
🤖 Fix all issues with AI agents
In `@docs/execplans/migrate-from-cucumber-to-rstest-bdd.md`:
- Line 484: Replace the phrase "Prior to" with "Before" in the sentence "Prior
to removing the Cucumber runner on 2026-01-25, both suites were run side by side
and passed the same scenario set." so it reads "Before removing the Cucumber
runner on 2026-01-25, both suites were run side by side and passed the same
scenario set."
- Line 687: Replace the hyphen separator between the link and descriptor on the
markdown line containing "[Plan Agent Output](https://claude.ai) - Agent ID:
a9eb419" so it uses an en dash (–) instead of a hyphen; specifically update the
text "- [Plan Agent Output](https://claude.ai) - Agent ID: a9eb419" to "- [Plan
Agent Output](https://claude.ai) – Agent ID: a9eb419" to match the style
guideline.
- Around line 543-567: In the Critical Files section of
docs/execplans/migrate-from-cucumber-to-rstest-bdd.md, replace the ASCII hyphen
separators (" - ") with en dashes (" – ") between filenames and their
descriptions for entries such as Cargo.toml, tests/bdd/mod.rs, Makefile and the
Phase lists (e.g., tests/bdd/fixtures/correlation.rs – First fixture, etc.);
update all occurrences in the shown block (Phase 0–4 listings) and then run
markdownlint and the repository formatter (make markdownlint; make fmt) to
ensure styling checks pass.
- Around line 203-395: Replace the ASCII hyphens used in phase headings and
status lines with en dashes (–) to match style: change "Phase 1: Pilot Migration
- Simple Worlds" to "Phase 1: Pilot Migration – Simple Worlds", "**Example -
CorrelationWorld**" to "**Example – CorrelationWorld**", all occurrences of
"**Status**: ✅ **COMPLETE** -" to "**Status**: ✅ **COMPLETE** –" (seen in the
blocks containing MessageAssemblerWorld/MessageAssemblyWorld/... and the later
Status lines), and "Phase 3: Complex Worlds - Client & Messaging" to "Phase 3:
Complex Worlds – Client & Messaging"; update the exact strings in
docs/execplans/migrate-from-cucumber-to-rstest-bdd.md and run make fmt to
normalize formatting.
In `@tests/fixtures/client_runtime.rs`:
- Around line 207-217: The verify_error method is incorrectly restricting
allowed errors to only ClientError::Disconnected and ClientError::Io(_); update
verify_error (which accesses self.last_error and calls self.await_server()) to
accept any ClientError variant instead of matching specific variants — either
remove the matches! check entirely and just confirm an error is present, or
broaden the match to cover all ClientError variants defined in the enum
(Serialize, Deserialize, PreambleEncode, PreambleWrite, PreambleRead,
PreambleTimeout, CorrelationMismatch, Disconnected, Io(_)); ensure the method
still calls self.await_server() and returns Ok(()) when an error is present.
In `@tests/fixtures/message_assembler/message_assembler_asserts.rs`:
- Around line 112-119: Update assert_header_len to accept the new HeaderLength
newtype instead of raw usize: change the signature to take HeaderLength,
retrieve the inner value with HeaderLength.0 when comparing to
parsed.header_len(), and update the error message to display the HeaderLength
value (e.g., expected {expected_val}, got {actual}). Ensure you reference the
HeaderLength type from the types module, keep the existing parsed.header_len()
call and return the same TestResult error path on mismatch.
In `@tests/fixtures/panic.rs`:
- Around line 79-83: The Rustdoc is placed after the #[derive(Debug, Default)]
attribute, so the documentation won't attach to the struct correctly; move the
doc comment /// Test world that drives a server which intentionally panics
during setup. immediately above the struct declaration for PanicWorld and keep
the #[derive(Debug, Default)] attribute directly above the struct (below the
doc) so the doc comment attaches to PanicWorld as intended.
In `@tests/steps/message_assembler_steps.rs`:
- Around line 120-126: The step function then_header_len currently forwards a
raw usize to MessageAssemblerWorld.assert_header_len; update it to wrap the
primitive in the HeaderLength newtype (e.g. construct a HeaderLength from
header_len) before calling assert_header_len, and add/import HeaderLength if
needed; also update the function signature or assert_header_len call if
assert_header_len now expects HeaderLength instead of usize so the types align
(use MessageAssemblerWorld::assert_header_len and then_header_len as the
locations to change).
In `@tests/steps/message_assembly_steps.rs`:
- Around line 330-341: The test creates an unused FrameId via
FrameId::with_key(key.0) but only uses its .key; simplify
then_error_missing_first_frame by constructing/using the MessageKey directly
(avoid FrameId::with_key) and pass that MessageKey into
message_assembly_world.is_missing_first_frame; update the
then_error_missing_first_frame function to call is_missing_first_frame with the
MessageKey (key.0) instead of frame_id.key and remove the FrameId creation.
- Around line 367-377: The test step then_key_evicted incorrectly uses
assert_error for a boolean state check; replace the assert_error call with
assert_condition so it directly asserts world.was_evicted(key.to_key()) without
referencing last_error(), e.g. update the invocation in then_key_evicted to call
assert_condition(message_assembly_world, |world|
world.was_evicted(key.to_key()), format!("expected key {} to be evicted",
key.0)); leave the MessageAssemblyWorld, was_evicted, key.to_key(), and message
text unchanged.
Address remaining review feedback in execplan text and test fixtures. - Standardise en dashes and wording in the migration plan - Add HeaderLength newtype and use it in assertions/steps - Simplify message assembly step checks and panic world docs - Relax client runtime error variant checking
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@docs/execplans/migrate-from-cucumber-to-rstest-bdd.md`:
- Around line 361-367: The Phase 2 status line incorrectly lists worlds from
Phase 4; update the Phase 2 status so it only reflects the worlds actually
migrated in Phase 2 (e.g., keep `MessageAssemblerWorld`, `MessageAssemblyWorld`,
`CodecErrorWorld`, `FragmentWorld`), and remove or relocate the `PanicWorld`,
`MultiPacketWorld`, `StreamEndWorld`, and `CodecStatefulWorld` entries to the
Phase 4 status line so progress reporting matches phase scope.
- Around line 541-555: Update the documented file paths in the Phase 0 and Phase
1 lists to reflect the post-rename layout by replacing occurrences of
"tests/bdd/..." with "tests/..." (e.g., change
`tests/bdd/fixtures/correlation.rs`, `tests/bdd/steps/correlation_steps.rs`,
`tests/bdd/scenarios/correlation_scenarios.rs`,
`tests/bdd/fixtures/request_parts.rs`, `tests/bdd/steps/request_parts_steps.rs`,
and `tests/bdd/scenarios/request_parts_scenarios.rs` to their `tests/...`
equivalents) so the doc matches the current repo structure.
In `@tests/fixtures/client_runtime.rs`:
- Around line 203-215: The verify_error function currently binds err then
immediately discards it with a no-op (let _ = err); remove that redundant
binding and collapse the existence check into a single statement by calling
self.last_error.borrow().as_ref().ok_or("expected client error was not
captured")? to short-circuit if missing, then proceed to call await_server();
update the body of verify_error (referencing verify_error, last_error,
await_server) to eliminate the unused variable and perform the existence check
inline.
- Around line 39-74: Add a centralized blocking helper on ClientRuntimeWorld
(e.g., a private method block_on<F, R>(&self, fut: F) -> TestResult<R>) that
first checks for an existing Tokio runtime via
tokio::runtime::Handle::try_current() and returns a clear TestResult error like
"nested tokio runtime detected; use async test or remove block_on" if a handle
exists; otherwise obtain the runtime via the existing runtime() method and call
runtime.block_on(fut). Replace direct calls to runtime().block_on(...) in
ClientRuntimeWorld methods with this helper so all blocking is guarded and fails
fast instead of panicking.
In `@tests/steps/message_assembly_steps.rs`:
- Around line 64-69: The doc comment for struct FrameId is inaccurate: it says
the sequence is optional but the field pub sequence: FrameSequence is mandatory;
update the comment for FrameId to reflect that sequence is required (or
alternatively change the field type to an Option<FrameSequence> if optional
behavior is intended). Locate the FrameId definition and either remove the word
“optional” from the docstring so it reads “Frame identification combining key
and sequence,” or change the sequence field type to Option<FrameSequence> and
adjust any related code/tests to handle None.
Align execplan status and critical file paths with current layout. - Add defensive runtime guard and tidy client runtime error handling - Update message assembly documentation and comments - Switch TestResult re-export comments to Rustdoc
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 (1)
tests/fixtures/codec_stateful.rs (1)
171-185: Add rustdoc for the public fixture and place attributes after it.Document the public fixture and keep the attributes after the doc comment to satisfy the public API documentation rule.
🛠️ Proposed fix
-// rustfmt collapses simple fixtures into one line, which triggers unused_braces. -#[rustfmt::skip] -#[fixture] -pub fn codec_stateful_world() -> CodecStatefulWorld { - CodecStatefulWorld::default() -} +/// Fixture for stateful codec scenarios used by rstest-bdd steps. +/// +/// Note: rustfmt collapses simple fixtures into one line, which triggers +/// unused_braces, so keep rustfmt::skip. +#[rustfmt::skip] +#[fixture] +pub fn codec_stateful_world() -> CodecStatefulWorld { + CodecStatefulWorld::default() +}As per coding guidelines, add rustdoc comments to public Rust APIs.
🤖 Fix all issues with AI agents
In `@docs/execplans/migrate-from-cucumber-to-rstest-bdd.md`:
- Line 526: Update the Mitigation sentence that currently reads "Migrate in
Phase 4 after patterns proven. Can use `scenarios!`" to use correct passive
construction by inserting the auxiliary verb "are" so it reads "Migrate in Phase
4 after patterns are proven. Can use `scenarios!`" (edit the text in the
docs/execplans/migrate-from-cucumber-to-rstest-bdd.md content where the
Mitigation line appears).
Add rustdoc to the public codec stateful fixture and keep rustfmt::skip documented for the unused_braces lint.
Fix grammar in the risk mitigation section to align with review feedback.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/execplans/migrate-from-cucumber-to-rstest-bdd.md`:
- Around line 47-48: Remove the trailing comma after `StreamEndWorld` in the
markdown list so the first line reads "- `PanicWorld`, `MultiPacketWorld`,
`StreamEndWorld`" and the continuation line remains " `MessageAssemblerWorld`,
`CodecStatefulWorld`", ensuring no extra comma or punctuation is left at the end
of the first line (update the text around the symbol StreamEndWorld).
Remove the trailing comma in the Phase 2 world list for consistent markdown formatting.
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 by Sourcery
Replace the Cucumber-based BDD test harness with rstest-bdd, including new fixtures, step definitions, and scenarios, while keeping existing behaviour and feature coverage intact.
New Features:
Enhancements:
Build:
Documentation: