Conversation
Reviewer's GuideThis PR enhances coverage parsing by implementing line coverage extraction from lcov files, centralizing Cobertura XML parsing, integrating both parsers into the Rust and Python scripts, updating dependencies, and expanding tests to verify two-decimal formatting and edge cases. Class diagram for new and refactored coverage parsing functionsclassDiagram
class coverage_parsers {
+get_line_coverage_percent_from_cobertura(xml_file: Path) str
}
class run_rust {
+get_line_coverage_percent_from_lcov(lcov_file: Path) str
}
coverage_parsers <.. run_rust : uses
Flow diagram for coverage percent extraction logic in Rust scriptflowchart TD
A[Start: Run Rust coverage] --> B{Format?}
B -- lcov --> C[get_line_coverage_percent_from_lcov]
B -- cobertura --> D[get_line_coverage_percent_from_cobertura]
B -- other --> E[extract_percent]
C --> F[Write percent to GitHub output]
D --> F
E --> F
Flow diagram for coverage percent extraction logic in Python scriptflowchart TD
A[Start: Run Python coverage] --> B{Format?}
B -- coveragepy --> C[get_line_coverage_percent_from_cobertura]
B -- other --> D[get_line_coverage_percent_from_cobertura]
C --> E[Write percent to GitHub output]
D --> E
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughIntroduce new functions to extract line coverage percentages from LCOV and Cobertura XML files. Update main coverage extraction logic in Rust and Python scripts to use these functions based on input format. Replace XML parsing library and logic in the Python script. Add tests covering edge cases and malformed input scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CoverageScript
participant CoverageFile
User->>CoverageScript: Run coverage script with format parameter
CoverageScript->>CoverageFile: Read coverage report file
alt Format is LCOV
CoverageScript->>CoverageScript: Parse LCOV file for LF and LH
else Format is Cobertura XML
CoverageScript->>CoverageScript: Parse XML with lxml, count covered and total lines
else Other format
CoverageScript->>CoverageScript: Use fallback text parsing method
end
CoverageScript->>User: Output coverage percentage with two decimals
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ 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.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `.github/actions/generate-coverage/tests/test_scripts.py:26` </location>
<code_context>
out = tmp_path / "cov.lcov"
gh = tmp_path / "gh.txt"
+ out.write_text("LF:200\nLH:163\n")
shell_stubs.register(
"cargo",
</code_context>
<issue_to_address>
Consider adding tests for edge cases in lcov parsing, such as zero lines found or missing tags.
Currently, only the standard case is tested. Please add tests for scenarios like LF = 0, missing LH, or malformed lcov files to ensure robust parsing.
Suggested implementation:
```python
out.write_text("LF:200\nLH:163\n")
shell_stubs.register(
"cargo",
stdout="Coverage: 81.5%\n",
data = gh.read_text().splitlines()
assert f"file={out}" in data
assert "percent=81.50" in data
def test_lcov_zero_lines_found(tmp_path: Path, shell_stubs: StubManager) -> None:
out = tmp_path / "cov_zero.lcov"
gh = tmp_path / "gh_zero.txt"
# LF = 0, LH = 0
out.write_text("LF:0\nLH:0\n")
shell_stubs.register(
"cargo",
stdout="Coverage: 0.0%\n",
)
# Simulate the rest of the test as in the main test, or call the function under test
# For demonstration, we assume the same output file is written
data = ["file={}".format(out), "percent=0.00"]
assert f"file={out}" in data
assert "percent=0.00" in data
def test_lcov_missing_lh_tag(tmp_path: Path, shell_stubs: StubManager) -> None:
out = tmp_path / "cov_missing_lh.lcov"
gh = tmp_path / "gh_missing_lh.txt"
# Only LF present
out.write_text("LF:100\n")
shell_stubs.register(
"cargo",
stdout="Coverage: 0.0%\n",
)
# Simulate the rest of the test as in the main test, or call the function under test
# For demonstration, we assume the output is handled gracefully
data = ["file={}".format(out), "percent=0.00"]
assert f"file={out}" in data
assert "percent=0.00" in data
def test_lcov_malformed_file(tmp_path: Path, shell_stubs: StubManager) -> None:
out = tmp_path / "cov_malformed.lcov"
gh = tmp_path / "gh_malformed.txt"
# Malformed: non-integer values and missing LF
out.write_text("LF:abc\nLH:xyz\n")
shell_stubs.register(
"cargo",
stdout="Coverage: 0.0%\n",
)
# Simulate the rest of the test as in the main test, or call the function under test
# For demonstration, we assume the output is handled gracefully
data = ["file={}".format(out), "percent=0.00"]
assert f"file={out}" in data
assert "percent=0.00" in data
```
- If the actual lcov parsing logic is in a helper function, you may want to call that function directly in these tests and assert on its return value or raised exceptions.
- Adjust the test logic to match the actual output and error handling of your codebase (e.g., if errors are logged or exceptions are raised, assert accordingly).
- If the output file or percent calculation is handled differently, update the assertions to match the real behavior.
</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 (2)
.github/actions/generate-coverage/scripts/run_rust.py(2 hunks).github/actions/generate-coverage/tests/test_scripts.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.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
🧬 Code Graph Analysis (1)
.github/actions/generate-coverage/scripts/run_rust.py (1)
.github/actions/ratchet-coverage/scripts/run_coverage.py (1)
extract_percent(19-25)
🔇 Additional comments (3)
.github/actions/generate-coverage/scripts/run_rust.py (1)
90-90: Excellent conditional logic for format-specific parsing.The modification correctly routes LCOV format processing to the new function while preserving existing behaviour for other formats. This maintains backward compatibility and leverages the structured nature of LCOV files for more reliable parsing.
.github/actions/generate-coverage/tests/test_scripts.py (2)
26-26: Correct LCOV test data format.The test data correctly simulates LCOV format output with appropriate LF (lines found) and LH (lines hit) entries. The values 200 and 163 produce a clean 81.50% calculation for verification.
64-64: Correct assertion update for new formatting.The assertion properly reflects the new 2-decimal place formatting introduced by
percent_from_lcov. The expected value "81.50" correctly matches the calculation from the test data (163/200 * 100).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/actions/generate-coverage/tests/test_scripts.py (3)
160-167: Approve edge case testing for zero lines found.This test correctly addresses the past review feedback by testing the zero-division edge case where no lines are found in the LCOV file.
169-174: Approve testing for missing LH tag scenario.This test correctly addresses the past review feedback by testing the edge case where the LH tag is missing from the LCOV file, ensuring graceful handling.
176-181: Approve malformed file testing but verify error handling.This test correctly addresses the past review feedback by testing malformed LCOV files. However, verify that the current implementation handles mixed numeric/non-numeric values gracefully, as
map(int, ...)could raiseValueErrorfor partially malformed files.Run this script to verify error handling behaviour:
#!/bin/bash # Description: Test how the function handles mixed numeric/non-numeric LCOV values cd .github/actions/generate-coverage python3 -c " from pathlib import Path import tempfile import sys sys.path.insert(0, 'scripts') from run_rust import get_line_coverage_percent_from_lcov with tempfile.NamedTemporaryFile(mode='w', suffix='.lcov', delete=False) as f: f.write('LF:100\nLH:abc\n') # Mixed valid/invalid f.flush() try: result = get_line_coverage_percent_from_lcov(Path(f.name)) print(f'Result: {result}') except Exception as e: print(f'Exception: {type(e).__name__}: {e}') "
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/actions/generate-coverage/scripts/run_rust.py(2 hunks).github/actions/generate-coverage/tests/test_scripts.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.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
🔇 Additional comments (3)
.github/actions/generate-coverage/scripts/run_rust.py (1)
101-105: LGTM! Clean conditional logic for format-specific parsing.The conditional logic correctly branches between LCOV file parsing and stdout parsing based on the format parameter whilst maintaining backward compatibility.
.github/actions/generate-coverage/tests/test_scripts.py (2)
26-57: Excellent test fixture for isolated module testing.The fixture correctly implements dynamic module loading with stubbed dependencies, enabling isolated testing of internal functions without external dependencies. The approach of using importlib and monkeypatch is well-executed.
64-64: Approve test updates for LCOV format consistency.The test correctly pre-populates the coverage file with LCOV format data and asserts the percentage with two decimal places, matching the new parsing function's output format.
Also applies to: 102-102
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/actions/generate-coverage/scripts/run_rust.py (1)
51-73: Add error handling for file operations and malformed content.The function lacks error handling for file reading failures and malformed content. Add try-except blocks to handle potential exceptions gracefully.
def get_line_coverage_percent_from_lcov(lcov_file: Path) -> str: """Return the overall line coverage percentage from an ``lcov.info`` file. Parameters ---------- lcov_file : Path Path to the coverage file to read. Returns ------- str The coverage percentage with two decimal places. """ - text = lcov_file.read_text(encoding="utf-8") + try: + text = lcov_file.read_text(encoding="utf-8") + except (FileNotFoundError, PermissionError, UnicodeDecodeError) as exc: + typer.echo(f"Could not read LCOV file {lcov_file}: {exc}", err=True) + raise typer.Exit(code=1) from exc def total(tag: str) -> int: - return sum(map(int, re.findall(rf"^{tag}:(\d+)$", text, flags=re.MULTILINE))) + try: + return sum(map(int, re.findall(rf"^{tag}:(\d+)$", text, flags=re.MULTILINE))) + except ValueError as exc: + typer.echo(f"Malformed LCOV file {lcov_file}: invalid {tag} values", err=True) + raise typer.Exit(code=1) from exc lines_found = total("LF") lines_hit = total("LH") if lines_found == 0: return "0.00" return f"{lines_hit / lines_found * 100:.2f}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/actions/generate-coverage/scripts/run_python.py(3 hunks).github/actions/generate-coverage/scripts/run_rust.py(3 hunks).github/actions/generate-coverage/tests/test_scripts.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.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
🧬 Code Graph Analysis (3)
.github/actions/generate-coverage/scripts/run_rust.py (2)
.github/actions/generate-coverage/scripts/run_python.py (3)
get_line_coverage_percent_from_cobertura(46-85)num_or_zero(65-67)lines_from_detail(69-72).github/actions/ratchet-coverage/scripts/run_coverage.py (1)
extract_percent(19-25)
.github/actions/generate-coverage/scripts/run_python.py (1)
.github/actions/generate-coverage/scripts/run_rust.py (4)
get_line_coverage_percent_from_cobertura(76-115)num_or_zero(95-97)lines_from_detail(99-102)total(66-67)
.github/actions/generate-coverage/tests/test_scripts.py (4)
.github/actions/generate-coverage/tests/conftest.py (1)
shell_stubs(29-40)shellstub.py (1)
StubManager(39-153).github/actions/generate-coverage/scripts/run_rust.py (2)
get_line_coverage_percent_from_lcov(51-73)get_line_coverage_percent_from_cobertura(76-115).github/actions/generate-coverage/scripts/run_python.py (1)
get_line_coverage_percent_from_cobertura(46-85)
🔇 Additional comments (6)
.github/actions/generate-coverage/scripts/run_rust.py (2)
4-4: LGTM!The addition of
lxmldependency is necessary for the new XML parsing functionality.
143-149: LGTM!The conditional logic correctly selects the appropriate parser based on the format while maintaining backward compatibility.
.github/actions/generate-coverage/scripts/run_python.py (1)
126-126: LGTM!The updates correctly use the new Cobertura parser for both coverage formats.
Also applies to: 129-129
.github/actions/generate-coverage/tests/test_scripts.py (3)
26-57: LGTM!The fixtures provide excellent isolation for testing internal functions without external dependencies. The monkeypatching approach is well-implemented.
Also applies to: 183-216
64-64: LGTM!The test correctly validates the new LCOV parsing logic with proper file content and two-decimal precision formatting.
Also applies to: 102-102
218-260: LGTM!The Cobertura tests comprehensively cover the parsing logic including detail extraction, fallback to root totals, and edge cases.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- I don’t see explicit imports of the new coverage_parsers functions in run_python.py/run_rust.py—make sure you add
from coverage_parsers import get_line_coverage_percent_from_cobertura, get_line_coverage_percent_from_lcov(or similar) so the scripts actually compile. - The run_rust_module and run_python_module fixtures share a ton of duplicated setup/stubbing logic—consider extracting the common parts into helper functions or shared fixtures to reduce boilerplate and improve readability.
- In get_line_coverage_percent_from_lcov, you re-raise ValueErrors inside the
totalhelper then catch them to return “0.00”—it might be cleaner to catch parsing errors directly insidetotal(and call typer.Exit) so the function has a single, consistent error-handling path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- I don’t see explicit imports of the new coverage_parsers functions in run_python.py/run_rust.py—make sure you add `from coverage_parsers import get_line_coverage_percent_from_cobertura, get_line_coverage_percent_from_lcov` (or similar) so the scripts actually compile.
- The run_rust_module and run_python_module fixtures share a ton of duplicated setup/stubbing logic—consider extracting the common parts into helper functions or shared fixtures to reduce boilerplate and improve readability.
- In get_line_coverage_percent_from_lcov, you re-raise ValueErrors inside the `total` helper then catch them to return “0.00”—it might be cleaner to catch parsing errors directly inside `total` (and call typer.Exit) so the function has a single, consistent error-handling path.
## Individual Comments
### Comment 1
<location> `.github/actions/generate-coverage/scripts/run_rust.py:90` </location>
<code_context>
+ except ValueError:
+ return "0.00"
+
+ if lines_found == 0:
+ return "0.00"
+ return f"{lines_hit / lines_found * 100:.2f}"
</code_context>
<issue_to_address>
Returning 0.00 for zero lines found may obscure configuration issues.
Consider adding a warning or error log when no lines are found to help identify empty or misconfigured lcov files.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if lines_found == 0:
return "0.00"
return f"{lines_hit / lines_found * 100:.2f}"
=======
if lines_found == 0:
import logging
logging.warning("No lines found in lcov data. This may indicate an empty or misconfigured lcov file.")
return "0.00"
return f"{lines_hit / lines_found * 100:.2f}"
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `.github/actions/generate-coverage/scripts/coverage_parsers.py:38` </location>
<code_context>
+ typer.echo(f"Failed to parse coverage XML {xml_file}: {exc}", err=True)
+ return "0.00"
+
+ def num_or_zero(expr: str) -> int:
+ n = root.xpath(f"number({expr})")
+ return 0 if n != n else int(n)
+
+ def lines_from_detail() -> tuple[int, int]:
</code_context>
<issue_to_address>
NaN check via n != n is non-obvious.
Consider using math.isnan(n) for clarity, or add a comment if you prefer the current approach.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def num_or_zero(expr: str) -> int:
n = root.xpath(f"number({expr})")
return 0 if n != n else int(n)
=======
import math
def num_or_zero(expr: str) -> int:
n = root.xpath(f"number({expr})")
return 0 if math.isnan(n) else int(n)
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `.github/actions/generate-coverage/scripts/run_rust.py:52` </location>
<code_context>
return match[1]
+def get_line_coverage_percent_from_lcov(lcov_file: Path) -> str:
+ """Return the overall line coverage percentage from an ``lcov.info`` file.
+
</code_context>
<issue_to_address>
Consider moving the LCOV parser to a shared module and replacing the nested ternary with if/elif/else statements for clarity.
```suggestion
I’d extract the LCOV parser into `coverage_parsers.py` alongside Cobertura, then replace the nested ternary in `main()` with a simple if/elif/else. This keeps all parsing logic in one module and makes the flow more readable.
1) Move the LCOV helper into `coverage_parsers.py`:
```python
# coverage_parsers.py
from pathlib import Path
import re
import typer
def get_line_coverage_percent_from_lcov(lcov_file: Path) -> str:
"""Return the overall line coverage percentage from an lcov.info file."""
try:
text = lcov_file.read_text(encoding="utf-8")
except OSError as exc:
typer.echo(f"Could not read {lcov_file}: {exc}", err=True)
raise typer.Exit(1) from exc
def total(tag: str) -> int:
values = re.findall(rf"^{tag}:(\d+)$", text, flags=re.MULTILINE)
return sum(int(v) for v in values)
try:
lines_found = total("LF")
lines_hit = total("LH")
except ValueError:
return "0.00"
if lines_found == 0:
return "0.00"
return f"{lines_hit / lines_found * 100:.2f}"
```
2) Remove the inline LCOV function from your runner and update `main()` to:
```python
from coverage_parsers import (
get_line_coverage_percent_from_lcov,
get_line_coverage_percent_from_cobertura,
)
# ...
try:
retcode, stdout, stderr = cargo[args].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(retcode or 1)
typer.echo(stdout)
if fmt == "lcov":
percent = get_line_coverage_percent_from_lcov(out)
elif fmt == "cobertura":
percent = get_line_coverage_percent_from_cobertura(out)
else:
percent = extract_percent(stdout)
```
This keeps all parsing functions co-located, and the selection logic becomes linear and easy to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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 (1)
.github/actions/generate-coverage/scripts/coverage_parsers.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
.github/actions/generate-coverage/scripts/coverage_parsers.py (1)
.github/actions/generate-coverage/tests/test_scripts.py (2)
getroot(81-82)xpath(56-75)
🔇 Additional comments (2)
.github/actions/generate-coverage/scripts/coverage_parsers.py (2)
1-26: Excellent import structure and dependency management.The conditional lxml import with immediate failure feedback and the TYPE_CHECKING pattern for Path are well-implemented best practices.
28-87: Well-structured function with comprehensive error handling.The two-tier parsing approach (detailed lines first, then root attributes) with Decimal-based precision calculations is robust and appropriate for Cobertura XML parsing.
1 participant
Summary
Testing
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_688588977888832283e0264c1eb19b1e
Summary by Sourcery
Improve coverage parsing by extracting line coverage from lcov and Cobertura outputs via new helper functions, integrating them into the Rust and Python coverage scripts, and bolstering error handling and test coverage.
New Features:
Enhancements:
Tests: