Add cucumber-rs support to generate-coverage#48
Conversation
Reviewer's GuideThis PR enhances the generate-coverage action with optional cucumber-rs support by adding inputs, installing cargo-cucumber, extending the Rust coverage script to run and merge cucumber scenarios, and updating tests and documentation accordingly. Class diagram for run_rust.py main function changesclassDiagram
class main {
+lang: str
+fmt: str
+github_output: Path
+cucumber_rs_features: str
+cucumber_rs_args: str
+with_cucumber_rs: bool
+get_cargo_coverage_cmd()
+get_line_coverage_percent_from_lcov()
}
class cargo_cucumber {
<<external>>
}
main --|> cargo_cucumber : uses when with_cucumber_rs
main : +merge_cucumber_coverage()
main : +handle_cucumber_args()
main : +merge-cobertura (external)
main : +uvx (external)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughExtend the GitHub action Changes
Sequence Diagram(s)sequenceDiagram
participant GitHubAction as GitHub Action
participant run_rust_py as run_rust.py
participant cargo as cargo
participant uvx as uvx (merge tool)
participant Output as Output File
GitHubAction->>run_rust_py: Invoke with inputs (including Cucumber.rs options)
run_rust_py->>cargo: Run cargo llvm-cov (standard tests)
cargo-->>run_rust_py: Standard coverage output
alt with_cucumber_rs enabled
run_rust_py->>cargo: Run cargo llvm-cov (cucumber tests with features/args)
cargo-->>run_rust_py: Cucumber coverage output
alt format is cobertura
run_rust_py->>uvx: Merge standard + cucumber coverage XML
uvx-->>run_rust_py: Merged coverage XML
run_rust_py->>Output: Write merged output
else other formats
run_rust_py->>Output: Concatenate outputs
end
else
run_rust_py->>Output: Write standard coverage output
end
run_rust_py-->>GitHubAction: Exit with result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (5)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- Add assertions to the cucumber test that verify the merged coverage file contains the expected combined content and that the temporary cucumber file is cleaned up to cover the full merge+cleanup behavior.
- Manually concatenating lcov files can lead to malformed records; consider using an lcov-aware merge tool or adding header/footer validation to ensure the combined file remains valid.
- There’s a lot of duplicated logic in run_rust.py for running cargo llvm-cov and handling errors—extracting a helper function would DRY up command construction and improve maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add assertions to the cucumber test that verify the merged coverage file contains the expected combined content and that the temporary cucumber file is cleaned up to cover the full merge+cleanup behavior.
- Manually concatenating lcov files can lead to malformed records; consider using an lcov-aware merge tool or adding header/footer validation to ensure the combined file remains valid.
- There’s a lot of duplicated logic in run_rust.py for running cargo llvm-cov and handling errors—extracting a helper function would DRY up command construction and improve maintainability.
## Individual Comments
### Comment 1
<location> `.github/actions/generate-coverage/tests/test_scripts.py:162` </location>
<code_context>
+def test_run_rust_with_cucumber(tmp_path: Path, shell_stubs: StubManager) -> None:
</code_context>
<issue_to_address>
Missing test for Cobertura format and merge-cobertura logic.
Please add a test that covers the Cobertura format, including the 'merge-cobertura' logic and its error handling, to ensure full coverage of the new code.
</issue_to_address>
### Comment 2
<location> `.github/actions/generate-coverage/scripts/run_rust.py:134` </location>
<code_context>
typer.echo(stdout)
+
+ cucumber_file: Path | None = None
+ if with_cucumber_rs and cucumber_rs_features:
+ cucumber_file = out.with_name(f"{out.stem}.cucumber{out.suffix}")
+ c_args = get_cargo_coverage_cmd(
</code_context>
<issue_to_address>
Consider extracting the cucumber.rs coverage logic into a separate function to simplify the main function.
```suggestion
# extract the cucumber.rs coverage logic into its own function
def run_cucumber_rs_coverage(
out: Path,
fmt: str,
features: str,
with_default: bool,
cucumber_rs_features: str,
cucumber_rs_args: str,
) -> None:
cucumber_file = out.with_name(f"{out.stem}.cucumber{out.suffix}")
cmd = get_cargo_coverage_cmd(fmt, cucumber_file, features, with_default=with_default)
cmd += ["--", "--test", "cucumber", "--", "cucumber", "--features", cucumber_rs_features]
if cucumber_rs_args:
cmd += cucumber_rs_args.split()
try:
retcode, stdout, stderr = cargo[cmd].run(retcode=None)
except ProcessExecutionError as exc:
retcode, stdout, stderr = exc.retcode, exc.stdout, exc.stderr
if retcode != 0:
typer.echo(f"cargo llvm-cov failed with code {retcode}: {stderr}", err=True)
raise typer.Exit(code=retcode or 1)
typer.echo(stdout)
if fmt == "cobertura":
from plumbum.cmd import uvx
try:
merged = uvx["merge-cobertura", str(out), str(cucumber_file)]()
except ProcessExecutionError as exc:
typer.echo(f"merge-cobertura failed with code {exc.retcode}: {exc.stderr}", err=True)
raise typer.Exit(code=exc.retcode or 1) from exc
out.write_text(merged)
else:
out.write_text(out.read_text() + cucumber_file.read_text())
cucumber_file.unlink()
# in main(), replace the big nested block with a single call
if with_cucumber_rs and cucumber_rs_features:
- cucumber_file = out.with_name(f"{out.stem}.cucumber{out.suffix}")
- # ... 20 lines of nested logic ...
- cucumber_file.unlink()
+ run_cucumber_rs_coverage(
+ out,
+ fmt,
+ features,
+ with_default,
+ cucumber_rs_features,
+ cucumber_rs_args,
+ )
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/actions/generate-coverage/README.md(2 hunks).github/actions/generate-coverage/action.yml(3 hunks).github/actions/generate-coverage/scripts/run_rust.py(3 hunks).github/actions/generate-coverage/tests/test_scripts.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
.github/actions/*/README.md
📄 CodeRabbit Inference Engine (AGENTS.md)
.github/actions/*/README.md: Each action must have aREADME.mdcontaining a one-liner summary, table of inputs, table of outputs, usage example withuses: ./.github/actions/<name>@<major>, and a release history link to CHANGELOG.
Add aDEPRECATED:banner to README and repository description when deprecating an action.
Files:
.github/actions/generate-coverage/README.md
.github/actions/*/tests/**
📄 CodeRabbit Inference Engine (AGENTS.md)
.github/actions/*/tests/**: Tests for each action must be present in atests/directory within the action's directory.
Contract tests must validate that declared inputs and outputs round-trip correctly.
Files:
.github/actions/generate-coverage/tests/test_scripts.py
.github/actions/*/action.yml
📄 CodeRabbit Inference Engine (AGENTS.md)
.github/actions/*/action.yml: Each action must have anaction.ymlfile containing every input and output, with required ones clearly marked.
Composite actions referencing sibling scripts must use${{ github.action_path }}for portability.
Files:
.github/actions/generate-coverage/action.yml
🧬 Code Graph Analysis (1)
.github/actions/generate-coverage/tests/test_scripts.py (2)
.github/actions/generate-coverage/tests/conftest.py (1)
shell_stubs(29-40)shellstub.py (4)
StubManager(39-153)register(49-84)env(98-103)calls_of(86-95)
🔇 Additional comments (9)
.github/actions/generate-coverage/README.md (2)
43-45: Documentation correctly describes the new cucumber-rs inputs.The input descriptions are clear and consistent with the existing documentation style.
103-112: Usage example demonstrates the cucumber-rs feature effectively.The example shows proper configuration of all three cucumber-rs inputs with realistic values.
.github/actions/generate-coverage/tests/test_scripts.py (1)
162-211: Test comprehensively validates cucumber-rs functionality.The test properly verifies that:
- Two cargo commands are executed when cucumber-rs is enabled
- The second command includes the expected cucumber-specific arguments
- Environment variables are correctly processed
- Output files are handled appropriately
.github/actions/generate-coverage/action.yml (3)
30-39: Input definitions follow consistent structure.The new cucumber-rs inputs are properly defined with appropriate descriptions and defaults.
97-100: Conditional installation logic is correct.The step properly checks both the cucumber-rs flag and language detection before installing cargo-cucumber.
110-112: Environment variables correctly map inputs to script.The new environment variables properly pass the cucumber-rs configuration to the run_rust.py script.
.github/actions/generate-coverage/scripts/run_rust.py (3)
34-36: Typer options correctly defined for cucumber-rs inputs.The new options follow the established pattern and properly map to environment variables.
112-114: Function parameters properly integrate cucumber-rs options.The new parameters are correctly added to the main function signature.
164-178: Coverage merging logic handles different formats appropriately.The implementation correctly:
- Uses merge-cobertura tool for cobertura format
- Falls back to concatenation for other formats
- Removes temporary files after merging
leynos
deleted the
codex/add-support-for-cucumber-rs-tests-in-generate-coverage
branch
1 participant
Summary
generate-coverageaction to optionally run cucumber-rs scenarioscargo-cucumberwhen requestedrun_rust.pyTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_6888c262e78c83228f0d5b60d867a5a4
Summary by Sourcery
Enable cucumber-rs coverage in the generate-coverage action and merge its results with standard Rust coverage
New Features:
Build:
Documentation:
Tests: