Refactor BDD harness to per-scenario TestWorld with rstest-bdd#251
Refactor BDD harness to per-scenario TestWorld with rstest-bdd#251
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughSummarise the migration from cucumber to rstest-bdd, add TestWorld and many BDD step modules, introduce PATH override plumbing for stdlib which lookups, refactor manifest target expansion to evaluate static when clauses, update CI toolchain matrix and test deps, and route tracing logs to stderr. (48 words) Changes
Sequence Diagram(s)sequenceDiagram
%% Styling for clarity
rect rgb(240,248,255)
participant Runner as Test Runner / Scenario
participant World as TestWorld
participant Stdlib as Stdlib (StdlibConfig)
participant Which as WhichResolver
participant Env as EnvSnapshot
participant FS as Filesystem / PATH
end
Runner->>World: instantiate TestWorld
Runner->>Stdlib: request render (template + optional path)
Stdlib->>Stdlib: build StdlibConfig (includes path_override)
Stdlib->>Which: create WhichConfig(path_override)
Which->>Which: WhichResolver::new(path_override)
Stdlib->>Which: resolve helper command
Which->>Env: EnvSnapshot::capture(path_override?)
Env->>FS: read PATH (prefer path_override)
FS-->>Env: PATH entries
Env-->>Which: resolved path entries
Which-->>Stdlib: resolved executable path or not found
Stdlib-->>World: store render output or error
World-->>Runner: assertion/verification step
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file //! Step definitions for CLI parsing scenarios.
❌ New issue: String Heavy Function Arguments |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file //! Step definitions for manifest parsing scenarios.
❌ New issue: String Heavy Function Arguments |
|
@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/bdd/steps/stdlib/rendering.rs Comment on file //! Render-related stdlib step helpers used by BDD scenarios.
❌ New issue: String Heavy Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +619 to +643 fn target_rule_is(index: usize, rule: String) -> Result<()> {
let rule = RuleName::new(rule);
ensure!(index > 0, "target index is 1-based");
with_world(|world| {
let result = world.manifest.with_ref(|m| {
let target = m
.targets
.get(index - 1)
.with_context(|| format!("missing target {index}"))?;
match &target.recipe {
Recipe::Rule { rule: actual } => {
let actual_str = get_string_from_string_or_list(actual, "rule")?;
ensure!(
actual_str == rule.as_str(),
"expected target {index} rule '{}', got '{actual_str}'",
rule
);
Ok(())
}
other => bail!("Expected rule recipe, got: {other:?}"),
}
});
result.context("manifest has not been parsed")?
})
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file //! Step definitions for manifest parsing scenarios.
❌ New issue: String Heavy Function Arguments |
|
@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/bdd/steps/stdlib/rendering.rs Comment on file //! Render-related stdlib step helpers used by BDD scenarios.
❌ New issue: String Heavy Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file //! Step definitions for manifest parsing scenarios.
❌ New issue: String Heavy Function Arguments |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +80 to +87 fn assert_target_name_eq(target_index: usize, actual: &str, expected: &TargetName) -> Result<()> {
ensure!(
actual == expected.as_str(),
"expected target {target_index} name '{}', got '{actual}'",
expected
);
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +82 to +101 fn assert_string_eq<T>(
context: &str,
index: Option<usize>,
field: &str,
actual: &str,
expected: &T,
) -> Result<()>
where
T: AsRef<str> + std::fmt::Display,
{
let location = match index {
Some(idx) => format!("{context} {idx}"),
None => context.to_string(),
};
ensure!(
actual == expected.as_ref(),
"expected {location} {field} '{expected}', got '{actual}'"
);
Ok(())
}❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
leynos
changed the title
Add function-level #[expect(...)] attributes in BDD test step modules to suppress clippy lints caused by rstest-bdd macro-generated code. This cleans up file-wide blanket allows, making lint management more targeted and maintainable. Also apply minor code cleanups for style and clarity across BDD steps, such as using method references, trimming strings before processing, passing references, and simplifying code formatting. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/bdd/steps/stdlib/parsing.rs (1)
7-10: Add doc comments to public functions.Both
parse_iso_timestampandparse_expected_offsetarepub(crate)but lack///doc comments, whilst the private helpers (parse_sign,parse_optional_component) are documented. Document the public API for cargo doc generation and maintainability.🔎 Proposed doc comments
+/// Parse an ISO8601 timestamp string into an `OffsetDateTime`. +/// +/// # Errors +/// +/// Returns an error if the input does not conform to ISO8601 format. pub(crate) fn parse_iso_timestamp(raw: &str) -> Result<OffsetDateTime> { OffsetDateTime::parse(raw, &Iso8601::DEFAULT) .with_context(|| format!("parse ISO8601 timestamp from '{raw}'")) }+/// Parse a UTC offset string such as `+00:00`, `-05:30`, or `Z`. +/// +/// Supports hours, optional minutes, and optional seconds components. +/// The special case-insensitive token `"Z"` is recognized as UTC. +/// +/// # Errors +/// +/// Returns an error if the format is invalid or components are out of range. pub(crate) fn parse_expected_offset(raw: &str) -> Result<UtcOffset> { let trimmed = raw.trim();Also applies to: 37-63
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
tests/bdd/steps/cli.rstests/bdd/steps/ir.rstests/bdd/steps/manifest/mod.rstests/bdd/steps/manifest/targets.rstests/bdd/steps/manifest_command.rstests/bdd/steps/mod.rstests/bdd/steps/ninja.rstests/bdd/steps/process.rstests/bdd/steps/stdlib/assertions.rstests/bdd/steps/stdlib/parsing.rstests/bdd/steps/stdlib/policy.rstests/bdd/steps/stdlib/rendering.rstests/bdd/steps/stdlib/workspace.rstests/bdd/types.rstests/logging_stderr_tests.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/logging_stderr_tests.rstests/bdd/steps/manifest_command.rstests/bdd/steps/ir.rstests/bdd/steps/mod.rstests/bdd/steps/ninja.rstests/bdd/steps/manifest/targets.rstests/bdd/steps/manifest/mod.rstests/bdd/steps/stdlib/parsing.rstests/bdd/steps/process.rstests/bdd/steps/stdlib/workspace.rstests/bdd/steps/cli.rstests/bdd/steps/stdlib/policy.rstests/bdd/steps/stdlib/rendering.rstests/bdd/types.rstests/bdd/steps/stdlib/assertions.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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 and docs must follow en-GB-oxendict (-ize / -yse / -our) 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.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- 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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/logging_stderr_tests.rstests/bdd/steps/manifest_command.rstests/bdd/steps/ir.rstests/bdd/steps/mod.rstests/bdd/steps/ninja.rstests/bdd/steps/manifest/targets.rstests/bdd/steps/manifest/mod.rstests/bdd/steps/stdlib/parsing.rstests/bdd/steps/process.rstests/bdd/steps/stdlib/workspace.rstests/bdd/steps/cli.rstests/bdd/steps/stdlib/policy.rstests/bdd/steps/stdlib/rendering.rstests/bdd/types.rstests/bdd/steps/stdlib/assertions.rs
🧬 Code graph analysis (5)
tests/bdd/steps/ir.rs (3)
tests/bdd/helpers/parse_store.rs (1)
store_parse_outcome(36-51)src/manifest/mod.rs (1)
from_path(158-160)src/ir/from_manifest.rs (1)
from_manifest(24-35)
tests/bdd/steps/manifest/targets.rs (1)
tests/bdd/steps/manifest/mod.rs (2)
get_string_from_string_or_list(41-61)with_manifest_error_context(30-39)
tests/bdd/steps/manifest/mod.rs (3)
tests/bdd/helpers/parse_store.rs (1)
store_parse_outcome(36-51)test_support/src/error.rs (1)
display_error_chain(17-27)src/manifest/mod.rs (1)
from_path(158-160)
tests/bdd/steps/process.rs (5)
test_support/src/lib.rs (2)
ensure_manifest_exists(195-214)fake_ninja(96-128)tests/bdd/fixtures/mod.rs (1)
world(167-169)test_support/src/env.rs (2)
prepend_dir_to_path(196-210)override_ninja_env(255-266)test_support/src/check_ninja.rs (1)
fake_ninja_check_build_file(82-110)src/runner/process/mod.rs (1)
run_ninja(151-160)
tests/bdd/steps/stdlib/policy.rs (2)
tests/bdd/fixtures/mod.rs (1)
world(167-169)src/stdlib/network/mod.rs (1)
policy(360-360)
🔇 Additional comments (43)
tests/logging_stderr_tests.rs (1)
1-24: LGTM! Clean integration test with proper idioms.The test correctly verifies that error logs are written to stderr. The implementation is idiomatic, using
assert_cmdandpredicateseffectively, and follows all applicable coding guidelines. The module and function doc comments are clear and follow en-GB conventions. Previous issues (formatting, unnecessarymutbinding, manual output parsing) have been resolved.tests/bdd/steps/stdlib/parsing.rs (2)
1-2: LGTM: module documentation is clear and accurate.The module-level doc comment correctly describes the parsing helpers for timestamp and offset formats, and the removal of host-extraction mention aligns with the migration of those concerns to centralized fixtures.
37-63: LGTM: variable rename improves clarity.The rename to
trimmedis semantically accurate, and the logic correctly uses the trimmed value for parsing whilst preserving the originalrawinput in error messages for debugging context.tests/bdd/steps/cli.rs (7)
16-23: LGTM!The
apply_clihelper cleanly composes token building, parsing with normalisation, and outcome storage. Error handling is delegated appropriately tostore_parse_outcome.
62-74: LGTM!The
build_token_listfunction now usesshlex::splitfor shell-like argument handling with a silent fallback to whitespace splitting, correctly addressing the previous review feedback. The documentation clearly explains the behaviour.
91-125: LGTM!The
ExpectedCommandenum provides a clean abstraction for command verification. The#[expect]attribute is correctly scoped to the function with an accurate reason explaining why const evaluation is not possible.
137-149: LGTM!The
verify_job_countfunction now uses the simplified pattern that separates data retrieval from assertion, as suggested in the previous review.
276-287: LGTM!The lint suppression is correctly scoped with a clear reason and a FIXME link for tracking. The step function properly converts the string parameter to the
CliArgsnewtype at the boundary before delegating to the typed helper.
293-366: LGTM!The Then step functions follow a consistent pattern: accept string parameters from rstest-bdd, convert to semantic newtypes at the boundary, and delegate to typed verification helpers. Error propagation is clean via direct Result returns.
67-74: No action required. The BDD feature file already contains comprehensive scenarios validating theshlex::splitimplementation with quoted and space-containing arguments, including single quotes, double quotes, escaped spaces, and multiple quoted arguments.tests/bdd/steps/stdlib/policy.rs (3)
1-6: LGTM: module doc and imports are well structured.The module doc correctly uses en-GB spelling and clearly explains the module's purpose. Imports are clean and organized.
24-36: LGTM: step functions correctly propagate errors.Both
allow_schemeandallow_hostfollow single responsibility, returnResult<()>, and use?to propagate errors from the underlying policy methods. The duplication in the take/modify/store pattern is addressed in the earlier comment.
38-54: LGTM: lint suppression is appropriately scoped and documented.The
#[expect]attribute ondefault_denyis narrowly scoped, includes a clear reason, and references the upstream issue. Bothdefault_denyandblock_hostcorrectly manage policy state.tests/bdd/steps/process.rs (4)
41-75: Clean error handling and clear phase separation.The past review comment has been properly addressed. The function now has three well-separated phases: extract temp path, extract CLI manifest path, resolve and update. Error messages are explicit and use proper context.
The
??pattern on line 62 is idiomatic for unwrappingOption<Result<T, E>>and provides clear error messages for both theNoneandErrcases.
103-191: LGTM: Given steps follow BDD patterns with proper guards.The Given steps provide clear, focused setup actions. Unix-specific steps are properly guarded with
#[cfg(unix)], error handling is consistent throughout, and helper reuse (viainstall_test_ninja) reduces duplication effectively.
193-244: LGTM: When steps with justified lint suppression.The lint suppression at lines 197–200 is properly scoped with
#[expect], includes a clear reason referencing an upstream rstest-bdd issue, and the if-let-else pattern is necessary to handle theprogram_pathlifetime correctly.Unix-specific subcommand execution is appropriately guarded with
#[cfg(unix)].
246-287: LGTM: Then steps provide clear assertions.The assertion steps use
ensure!with descriptive messages and proper error context. Fragment matching viacontains()is appropriate for BDD scenario verification.tests/bdd/steps/stdlib/rendering.rs (4)
1-12: Module structure and imports are well-organized.The module doc comment is present, newtypes are imported from the centralized
types.rs, and the separation of concerns between helpers and step definitions is clear. Previous review comments about consolidating newtypes have been addressed.
18-87: Configuration extraction and rendering helpers are well-structured.The
RenderConfigstruct groups related parameters as per coding guidelines. Therender_template_with_contextfunction handles multiple optional configurations cleanly with proper error contexts. The pattern of extracting config before applying avoids borrowing issues.
114-167:ContextSourceenum provides clean abstraction for context value sources.The enum pattern with
key()andget_value()methods eliminates duplication across rendering steps while maintaining type safety. Therender_with_context_sourcehelper composes these cleanly.
173-223: Step definitions follow the established pattern correctly.Each step function:
- Has narrowly-scoped
#[expect]with reason and FIXME link- Converts
Stringparameters to newtypes at the step boundary- Delegates to typed internal helpers
- Returns
Result<()>This aligns with the PR objectives and coding guidelines.
tests/bdd/steps/stdlib/workspace.rs (6)
1-21: Imports and constants are well-organized.The
concat!()macro forLINES_FIXTUREfollows the coding guideline to useconcat!()for combining long string literals rather than escaping newlines.
26-52:ensure_workspacecorrectly manages workspace lifecycle.The function:
- Uses early return for idempotency
- Handles cross-platform symlink differences with
#[cfg]attributes- Stores the
TempDirhandle in world to prevent premature cleanup- Provides proper error contexts throughout
54-71:compile_and_register_helperuses generics to reduce duplication.The generic approach allows reuse across different helper compilation functions while maintaining type safety.
163-205: Cross-platform executable handling is comprehensive.The functions
resolve_executable_path,executable_script, andmark_executablecorrectly handle platform differences:
- Windows: adds
.cmdextension, uses batch script syntax, skips chmod- Unix: uses shell script syntax, sets 0o755 permissions
265-278: Environment variable mutation correctly usestest_support::env::set_varwith proper locking.The function correctly:
- Tracks previous values for restoration via
world.track_env_var- Sets both
HOMEandUSERPROFILEon Windows- Wraps unsafe environment access with
EnvLockserialisation to prevent concurrent mutationThe
test_support::env::set_varimplementation acquires a shared guard before calling the underlyingstd::env::set_var, ensuring thread-safe mutation as required by Rust 2024.
211-263: Given step definitions follow the established pattern and require no lint suppressions.Each step converts
Stringparameters to newtypes at the boundary (HelperName::from(),TemplatePath::new(),FileContents::new(),PathEntries::new()), delegates to typed internal helpers, and returnsResult<()>for proper error propagation.The absence of
#[expect(clippy::shadow_reuse)]is correct. Unlike@whensteps in rendering.rs,@givensteps do not trigger this warning. This difference stems from how the rstest-bdd macro generates wrapper code for each step type—a known issue documented in leynos/rstest-bdd#381.tests/bdd/steps/stdlib/assertions.rs (8)
1-28: Module imports and initial assertion are correct.The
assert_stdlib_outputfunction properly uses.get()with.context()for error handling, avoiding panics.
30-47: Helper functions correctly handle output retrieval.The
stdlib_outputfunction properly handles both success and error cases viastdlib_output_or_error, and theas_deref()usage correctly convertsOption<String>toOption<&str>.
49-81: Purity assertions correctly consolidated intoassert_purity_state.The helper function eliminates duplication between impure and pure checks, and the public step functions delegate cleanly. This addresses the past review comment.
83-110: Fetch cache assertion handles path construction correctly.The function properly:
- Parses the URL with error context
- Constructs the cache path relative to workspace root
- Uses
try_existsrather than assuming existence
112-147: Path and executable assertions are implemented correctly.The assertions properly delegate to shared helpers and provide clear error messages. The
resolve_executable_pathcall ensures cross-platform compatibility for executable paths.
149-177: Timestamp assertions handle parsing and validation correctly.The 5-second tolerance for UTC timestamp validation is reasonable, and offset parsing uses the dedicated parsing module functions.
179-216: File content assertions include proper validation.The
assert_stdlib_output_file_uniformfunction correctly validates that the expected pattern is both:
- A single character (line 196)
- A single byte (line 201)
This handles potential multi-byte UTF-8 characters appropriately, ensuring the uniform byte check is valid.
218-232: Final assertion completes the file correctly.The
assert_stdlib_output_equals_textfunction properly retrieves both the expected text and file content, comparing them with a clear error message.tests/bdd/steps/mod.rs (1)
1-27: LGTM!The module documentation is clear, and the lint suppression approach now complies with coding guidelines. The file-wide
#[expect]attributes have been removed (as confirmed in past review), and function-level suppressions with FIXME tracking are appropriately documented.tests/bdd/steps/manifest/mod.rs (1)
1-281: LGTM!The manifest step definitions follow best practices:
- Function-level
#[expect]attributes are properly scoped with FIXME tracking- All step functions use
Resultand?for error propagation- Newtypes are applied at step boundaries as intended
- Helper functions have clear purposes and appropriate documentation
tests/bdd/steps/ninja.rs (1)
1-144: LGTM!The Ninja step definitions demonstrate excellent practices:
- All helpers use
Resultand appropriate error context- The
shlex::splitoptional result is properly converted toResult(line 41)- Index 2 usage is now well-documented (lines 123-126) per past review
- No lint suppressions needed, no panicking code paths
tests/bdd/steps/ir.rs (1)
1-145: LGTM!The IR step definitions demonstrate excellent refactoring outcomes:
- Generic
assert_graph_collection_counthelper eliminates duplication (lines 25-44)- Duplicate step handlers consolidated into single function with multiple annotations (lines 72-74), addressing past review
compile_manifest_implnow returns()instead ofResult<()>(line 139), addressing past review- All error handling uses
Resultand?appropriatelytests/bdd/steps/manifest_command.rs (1)
1-186: LGTM!The manifest command step definitions demonstrate that past review feedback has been addressed:
- The trivial
check_file_existshelper was inlined (line 53 now callspath.exists()directly)store_run_resultordering improved (line 87 clonesstderrearly for use in both branches)- All step functions use newtypes at boundaries and proper error propagation
- Function-scoped
#[expect]attributes are appropriately appliedtests/bdd/steps/manifest/targets.rs (3)
22-47: Helper types are well-designed.
FieldLocationprovides excellent abstraction for assertion error messages, usingconst fnfor construction and clear formatting logic. The pattern of grouping location context improves readability over passing individual parameters.
49-83: Excellent macro-based abstraction for reducing boilerplate.The combination of generic
assert_string_eqand thedefine_assertion!macro effectively eliminates duplication whilst maintaining type safety and clear error messages. The macro invocations generate five distinct assertion functions with minimal code.
128-142:with_targetprovides excellent generic abstraction.The helper function is well-designed:
- Generic over return type enables flexible usage
- 1-based index validated explicitly (line 133)
- Consistent error context chaining via
with_manifest_error_context- Closure-based API keeps target access logic centralised
…et access helpers - Moved domain-specific typed assertions and target helper functions from `targets.rs` to a new `helpers.rs` module. - Updated `mod.rs` to include the new `helpers` module. - Simplified `targets.rs` by importing helper functions from `helpers`. - This reduces duplication and improves code organization and error context for BDD step definitions related to manifest targets. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
tests/bdd/steps/process.rs (2)
22-41: LGTM!The UTF-8 validation now uses explicit
to_str().ok_or_else()with a clear error message, addressing the previous review concern. The guard lifecycle management is correct.
43-77: LGTM!The refactored implementation separates concerns clearly with explicit comments for each step. The
ok_or_else(...)??pattern at line 64 correctly handles theOption<Result<...>>return type fromwith_ref.tests/bdd/steps/stdlib/policy.rs (1)
61-64: Apply the same simplifications asallow_schemeandallow_host.The
Ok(p.block_host(host)?)pattern can be simplified to justp.block_host(host), and thehostparameter should be converted to aHostNamenewtype at the step boundary for consistency with the broader refactoring pattern.🔎 Proposed simplification
#[given("the stdlib network policy blocks host {host:string}")] pub(crate) fn block_host(world: &TestWorld, host: &str) -> Result<()> { - modify_policy(world, |p| Ok(p.block_host(host)?)) + modify_policy(world, |p| p.block_host(host)) }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
tests/bdd/steps/cli.rstests/bdd/steps/manifest/helpers.rstests/bdd/steps/manifest/mod.rstests/bdd/steps/manifest/targets.rstests/bdd/steps/process.rstests/bdd/steps/stdlib/policy.rstests/bdd/steps/stdlib/workspace.rstests/bdd/types.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/bdd/steps/manifest/mod.rstests/bdd/steps/process.rstests/bdd/steps/manifest/targets.rstests/bdd/steps/stdlib/workspace.rstests/bdd/steps/manifest/helpers.rstests/bdd/types.rstests/bdd/steps/stdlib/policy.rstests/bdd/steps/cli.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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 and docs must follow en-GB-oxendict (-ize / -yse / -our) 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.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- 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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/bdd/steps/manifest/mod.rstests/bdd/steps/process.rstests/bdd/steps/manifest/targets.rstests/bdd/steps/stdlib/workspace.rstests/bdd/steps/manifest/helpers.rstests/bdd/types.rstests/bdd/steps/stdlib/policy.rstests/bdd/steps/cli.rs
🧠 Learnings (1)
📚 Learning: 2025-08-08T22:52:32.915Z
Learnt from: leynos
Repo: leynos/netsuke PR: 82
File: tests/support/mod.rs:151-155
Timestamp: 2025-08-08T22:52:32.915Z
Learning: In Rust code, when encountering the pattern:
```rust
#[allow(unfulfilled_lint_expectations, reason = "...")]
#[expect(dead_code, reason = "...")]
```
Recommend replacing it with:
```rust
#[cfg_attr(not(any(...)), expect(dead_code, reason = "..."))]
```
This uses conditional compilation to apply the `expect` attribute only when needed, avoiding the forbidden `#[allow]` attribute while properly handling cross-crate test utility usage scenarios.
Applied to files:
tests/bdd/types.rs
🧬 Code graph analysis (5)
tests/bdd/steps/process.rs (5)
test_support/src/lib.rs (2)
ensure_manifest_exists(195-214)fake_ninja(96-128)tests/bdd/fixtures/mod.rs (1)
world(167-169)test_support/src/env.rs (2)
prepend_dir_to_path(196-210)override_ninja_env(255-266)test_support/src/check_ninja.rs (1)
fake_ninja_check_build_file(82-110)src/runner/process/mod.rs (1)
run_ninja(151-160)
tests/bdd/steps/manifest/helpers.rs (2)
tests/bdd/steps/manifest/mod.rs (1)
with_manifest_error_context(32-41)tests/bdd/fixtures/mod.rs (1)
world(167-169)
tests/bdd/types.rs (2)
tests/bdd/steps/stdlib/types.rs (1)
as_str(18-20)src/runner/mod.rs (1)
into_string(42-44)
tests/bdd/steps/stdlib/policy.rs (3)
tests/bdd/fixtures/mod.rs (1)
world(167-169)src/stdlib/network/mod.rs (1)
policy(360-360)src/stdlib/network/policy/mod.rs (1)
deny_all_hosts(226-229)
tests/bdd/steps/cli.rs (5)
tests/bdd/helpers/parse_store.rs (1)
store_parse_outcome(36-51)tests/bdd/fixtures/mod.rs (3)
world(167-169)is_some(181-181)is_some(199-201)src/cli_policy.rs (1)
network_policy(47-65)tests/bdd/types.rs (6)
new(368-370)value(373-375)as_path(89-91)as_path(137-139)as_str(298-300)as_str(344-349)src/stdlib/network/policy/mod.rs (1)
evaluate(286-319)
🔍 Remote MCP
Let me search for more specific information about the PR's technical concerns and rstest-bdd capabilities.## Summary of Additional Context for PR Review
Based on my search for relevant information, here is the key context that will help with reviewing this pull request:
rstest-bdd Framework Capabilities
rstest-bdd brings BDD to Rust without requiring adoption of a bespoke runner or monolithic "world" object. The key architectural decision in this PR aligns well with rstest-bdd's design philosophy: rstest-bdd provides first-class fixtures that can be shared between unit, integration, and BDD tests.
Critical advantage for this PR: Since the generated test is an rstest, fixture dependency graph Just Works™, which directly supports the PR's goal of per-scenario TestWorld isolation through rstest fixtures.
Fixture Injection and Per-Test Isolation
The core idea is that you can inject test dependencies by passing them as test arguments, and a fixture can be injected by another fixture. This enables the PR's centralized TestWorld approach where each scenario gets a fresh fixture instance automatically.
Rust 1.89.0 and MSRV Context
The PR updates the Rust version to 1.89.0. Rust 1.89.0 was released on August 7, 2025 and stabilizes features including repr128, unpredictable_function_pointer_comparisons expansion, and avx512/kl/widekl target features for x86. The PR's MSRV bump from 1.75 to 1.89.0 is significant; reviewers should verify all new code uses only stable features available in 1.89.0.
Interior Mutability via RefCell
The PR's heavy use of RefCell<Option<T>> for TestWorld fields is a standard Rust pattern for shared test state. This design allows step definitions to mutate world state without taking mutable references, enabling the flexibility needed for BDD scenarios where steps are registered globally and called in sequence.
Compile-Time Step Registration
.feature files are processed at compile time; scenarios are static, and step definitions must be known at compile time (no dynamic registration). This supports the PR's transition from cucumber's dynamic registration to rstest-bdd's static, compile-time approach visible in the new tests/bdd_tests.rs entrypoint.
Key Technical Points for Review
-
TestWorld as Central Fixture: The new
tests/bdd/fixtures/TestWorldstruct properly leverages rstest's per-test fixture isolation, eliminating the shared mutable state issues of the oldCliWorld. -
Type-Safe Step Parameters: The new
tests/bdd/types.rsdomain newtypes (CliArgs, TargetName, etc.) address the reviewers' repeated feedback about replacing bare String parameters with semantic types at the step boundary. -
Toolchain Alignment: The PR updates documentation to reflect migration from nightly dependency to stable toolchain support (while CI validates both stable and MSRV), though you should verify the docs in
docs/adr-002anddocs/rstest-bdd-users-guide.mdare internally consistent on this point. -
PATH Override Mechanism: Changes to
src/stdlib/which/modules introduce explicit path override parameters flowing throughWhichResolver,EnvSnapshot::capture, andWhichConfig::new, enabling per-instance path overrides without global mutation.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: build-test (ubuntu-latest, stable)
- GitHub Check: build-test (ubuntu-latest, 1.89.0)
🔇 Additional comments (24)
tests/bdd/steps/process.rs (8)
1-16: LGTM!Module doc comment present, imports are well-organized, and dependencies are appropriate for the BDD step definitions.
79-89: LGTM!Simple delegation with clear error propagation.
91-103: LGTM!Clear and concise result recording pattern.
109-165: LGTM!Given steps are well-structured with appropriate platform-specific guards. The
no_ninjastep correctly simulates a missing executable by referencing a non-existent path.
167-193: LGTM!The
build_dir_existsstep correctly creates a directory namedbuild.ninjato test the error path when the build file is actually a directory.
199-222: LGTM!The
#[expect]attribute is correctly scoped with a clear reason and includes a FIXME link for tracking. The if-let-else pattern provides better readability for path resolution.
224-246: LGTM!Good extraction of
run_subcommandhelper to avoid duplication betweenrun_cleanandrun_graph. The{e:#}format preserves the error chain.
252-289: LGTM!Then steps provide clear assertions with descriptive error messages. The
ensure!macro is appropriate for BDD step validation.tests/bdd/steps/stdlib/policy.rs (3)
1-1: Module documentation is clear and compliant.The module-level doc comment meets the requirement to explain the module's purpose and utility.
12-35: Helper extraction successfully eliminates duplication.The
modify_policyandmodify_policy_infalliblehelpers consolidate the repeated pattern of take-modify-store across all step functions, directly addressing the previous review feedback. The generic closure approach provides flexibility whilst maintaining type safety.
51-59: Lint suppression is properly scoped and documented.The
#[expect(clippy::unnecessary_wraps)]is correctly scoped to this single function, includes a clear reason referencing the rstest-bdd macro constraint, and provides a FIXME link to the upstream issue. This follows the strict lint guidelines.tests/bdd/steps/manifest/targets.rs (1)
1-251: Past review concern about file length appears resolved.The past review flagged that this file exceeded 400 lines (421 lines). The provided code shows the file is now 251 lines, which complies with the 400-line guideline. The suggested modularisation into
helpers.rs(for helper types and assertion utilities) has been implemented, as evidenced by the imports fromsuper::helperson lines 6-11.tests/bdd/steps/manifest/helpers.rs (3)
1-193: Excellent use of macro-based code generation to reduce duplication.The
define_assertion!macro (lines 68-78) effectively generates typed assertion wrappers around the genericassert_string_eqfunction, eliminating repetitive boilerplate whilst maintaining type safety and clear error messages. The invocations on lines 80-84 create five assertion functions with consistent signatures and behaviour.
19-48: FieldLocation struct provides good error context.The
FieldLocationstruct captures location information (context, optional index, field name) and theformat_prefixmethod (lines 42-47) formats it appropriately for error messages. The use ofconst fnforwith_index(line 29) enables compile-time construction where possible.
130-143: with_target encapsulates the 1-based indexing pattern effectively.The
with_targethelper (lines 130-143) consolidates the common pattern of:
- Validating 1-based index
- Accessing manifest via
with_ref- Retrieving target with bounds check
- Applying a closure
- Wrapping result with manifest error context
This reduces duplication across step functions in
targets.rsand ensures consistent error handling.tests/bdd/steps/cli.rs (5)
1-5: Module documentation properly expanded.The module doc now explains both purpose and utility, integrating with TestWorld as required by coding guidelines.
20-60: Helper functions are well-structured.Proper error context and Result usage throughout.
95-274: Verification helpers are well-implemented.The typed verification approach with ExpectedCommand enum and individual verify functions provides good separation of concerns. Past review feedback has been addressed: job count verification simplified, lint reason corrected, and bail! used consistently.
280-370: Step definitions follow BDD best practices.Proper newtype conversion at step boundaries, consistent Result handling, and appropriate lint suppression for the rstest-bdd macro constraint.
71-78: Test coverage for quoted and space-containing arguments already exists in the BDD scenarios. The cli.feature file includes explicit cases for single-quoted arguments (line 90–94), double-quoted arguments (line 96–100), escaped spaces (line 102–106), and multiple quoted arguments (line 108–112). The shlex code path is fully exercised by these scenarios. No action is required.Likely an incorrect or invalid review comment.
tests/bdd/types.rs (4)
82-103: PathString properly implements path-centric API.The
AsRef<Path>trait implementation was added as requested in previous reviews, complementingas_path()andto_path_buf().
135-152: ManifestPath implements complete path-centric API.The
AsRef<Path>trait andto_path_buf()method were added as requested in previous reviews, providing the standard path wrapper interface.
249-356: Domain-specific type extensions are well-designed.Appropriate helper methods for each domain:
FileContents::as_bytes(),TokenList::to_vec(), and theHelperNamedistinction using&'static str. TheContentNameenum with itsas_str()andDisplayimplementation is particularly clean.
362-382: JobCount properly models non-string domain value.Appropriate use of a non-tuple struct with semantic constructor and accessor, with proper derives for comparison.
|
@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/bdd/steps/manifest/helpers.rs Comment on lines +182 to +193 pub(super) fn assert_macro_count(world: &TestWorld, expected: usize) -> Result<()> {
let count = world
.manifest
.with_ref(|m| m.macros.len())
.context("manifest has not been parsed");
let actual = with_manifest_error_context(world, count)?;
ensure!(
actual == expected,
"expected manifest to have {expected} macros, got {actual}"
);
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
…orkspace helpers - Introduce a generic function `assert_manifest_count` to handle manifest collection count assertions, replacing duplicated code for targets and macros in BDD test steps. - Simplify `assert_target_count` and `assert_macro_count` by delegating to the new generic function. - Add detailed documentation and caching logic to `ensure_workspace` for stdlib test workspace setup. - Add utility functions `resolve_template_path` and `resolve_executable_path` with platform-specific handling. - Minor cleanup in BDD type wrappers documentation with improved clippy expectations and allowance reasons. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
- Added newtype wrappers Scheme and HostName to bdd/types.rs for URL scheme and host name - Updated policy manipulation functions to use these newtypes for improved type safety and clarity - Modified error handling in modify_policy to accept any error convertible to anyhow::Error - Refactored allow_scheme, allow_host, and block_host steps to use the newtypes instead of raw strings Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
…] for API completeness Replaced instances of #[allow(dead_code)] with #[expect(dead_code)] annotations in the newtype and path wrappers. This change improves lint clarity by specifying that dead code warnings are expected and justified due to varying usage across type instantiations rather than simply allowing them. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/bdd/types.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/bdd/types.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- 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 and docs must follow en-GB-oxendict (-ize / -yse / -our) 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.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- 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()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- 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/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/bdd/types.rs
🧬 Code graph analysis (1)
tests/bdd/types.rs (2)
src/manifest/expand.rs (1)
as_str(148-152)src/runner/mod.rs (1)
into_string(42-44)
🪛 GitHub Actions: CI
tests/bdd/types.rs
[error] 28-28: this lint expectation is unfulfilled.
🪛 GitHub Check: build-test (ubuntu-latest, 1.89.0)
tests/bdd/types.rs
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 34-34:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
🪛 GitHub Check: build-test (ubuntu-latest, nightly, true)
tests/bdd/types.rs
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 34-34:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
🪛 GitHub Check: build-test (ubuntu-latest, stable)
tests/bdd/types.rs
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 34-34:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
[failure] 28-28:
this lint expectation is unfulfilled
🔍 Remote MCP
Based on my search of the repository documentation, here is the key additional context relevant to reviewing this pull request:
Summary of Additional Context Found
rstest-bdd Library Information
rstest-bdd brings BDD to Rust without requiring a bespoke runner or monolithic "world" object, instead building on rstest's excellent fixture and parametrization model. The framework allows scenarios to be executed with cargo test, enables first-class fixtures shared between unit and BDD tests, and provides ergonomic step definitions.
Migration Motivation and Related Work
The Deepwiki search confirmed that the repository currently uses a three-tier testing strategy with:
- rstest for fixture-based unit tests
- cucumber-rs for BDD with the
CliWorldstruct managing state - insta for snapshot testing
The PR's migration from cucumber-rs with CliWorld to rstest-bdd with TestWorld fixtures directly addresses the architectural goal of per-scenario isolation mentioned in the PR summary.
Test Framework Capabilities
Steps are registered at compile time into a global registry, at runtime the framework matches each Gherkin line against the registry and invokes the correct function, and if a step has no matching implementation you get a compile error.
Related Architecture from Deepwiki
The repository's overall architecture uses a six-stage pipeline for transformations:
- File ingestion
- YAML parsing
- Template expansion (foreach/when)
- Deserialization and string rendering
- IR generation and validation
- Ninja synthesis and execution
This context helps validate that the BDD infrastructure changes (this PR) are foundational to supporting the full pipeline testing strategy documented in the design.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
🔇 Additional comments (1)
tests/bdd/types.rs (1)
134-137: The#[expect(dead_code)]attribute at line 134 is correct—ManifestPath::to_path_buf()is genuinely unused throughout the codebase. The method exists for API completeness as a path wrapper, and the expectation is narrowly scoped with a clear reason. No changes required.
…] in macro-generated code The macro-generated newtypes provide complete APIs, but some methods are unused depending on instantiation. Since #[expect(dead_code)] requires the lint to fire and its usage varies, it was replaced with #[allow(dead_code)] to suppress warnings more consistently. Additionally, a module-level expect for clippy::allow_attributes was added to justify using #[allow(dead_code] within the macro. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
1 participant
Summary
Changes
Shared BDD harness and world lifecycle
Refactored BDD steps to consume shared helpers
Project structure and fixtures
CI, toolchains, and test execution
Documentation and references
Compatibility and breaking changes
Notes for maintainers
Task