Merged
Conversation
Contributor
Reviewer's GuideThis PR adds a new get_line_coverage_percent_from_cobertura function in run_rust.py that robustly parses Cobertura XML—handling missing files, permissions, invalid syntax, missing lxml dependency, and unexpected failures—then calculates and formats the overall line coverage percentage with fallback logic and two-decimal precision. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Summary by CodeRabbit
WalkthroughReplace the import of the Cobertura coverage parser in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant run_rust.py
participant lxml.etree
User->>run_rust.py: Run script with --format cobertura
run_rust.py->>lxml.etree: Import lxml.etree (runtime check)
run_rust.py->>run_rust.py: get_line_coverage_percent_from_cobertura(xml_file)
run_rust.py->>lxml.etree: Parse XML, extract line coverage
run_rust.py->>run_rust.py: _format_percent(covered, total)
run_rust.py-->>User: Output coverage percentage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 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}
Contributor
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/scripts/run_rust.py:88` </location>
<code_context>
+ else:
+ return 0 if math.isnan(n) else int(n)
+
+ def lines_from_detail() -> tuple[int, int]:
+ try:
+ total = int(root.xpath("count(//class/lines/line)"))
+ covered = int(
+ root.xpath("count(//class/lines/line[number(@hits) > 0])")
+ )
+ except Exception: # noqa: BLE001 - defensive
+ return 0, 0
+ else:
+ return covered, total
</code_context>
<issue_to_address>
lines_from_detail may silently mask parsing errors.
Returning (0, 0) for any exception may hide underlying XML or data issues. Consider logging or exposing errors to aid debugging.
</issue_to_address>
### Comment 2
<location> `.github/actions/generate-coverage/scripts/run_rust.py:100` </location>
<code_context>
+ return covered, total
+
+ covered, total = lines_from_detail()
+ if total == 0:
+ covered = num_or_zero("/coverage/@lines-covered")
+ total = num_or_zero("/coverage/@lines-valid")
+
</code_context>
<issue_to_address>
Fallback to summary attributes may not always be reliable.
If both detailed and summary attributes are missing or malformed, the function returns 0.00, which may hide upstream issues. Please review whether this fallback logic is sufficient for all Cobertura XML variants.
</issue_to_address>
### Comment 3
<location> `.github/actions/generate-coverage/scripts/run_rust.py:63` </location>
<code_context>
return match[1]
+def get_line_coverage_percent_from_cobertura(xml_file: Path) -> str:
+ """Return the overall line coverage percentage from a Cobertura XML file."""
+ try:
</code_context>
<issue_to_address>
Consider refactoring the function to use a top-level percentage formatter, specific exception handling, and direct XPath queries for clarity and maintainability.
```markdown
You can flatten and reuse logic, remove broad `except Exception` blocks, and keep your XML‐parsing logic DRY. For example:
1. Extract a small helper to format percentages at the top level.
2. Only catch the specific IO/XML exceptions in your main parser.
3. Use two clear XPath calls (one for class/lines, one for the `<coverage/>` attributes) instead of nested helpers.
```python
from decimal import Decimal, ROUND_HALF_UP
from lxml import etree
import typer
from pathlib import Path
def _format_percent(covered: int, total: int) -> str:
pct = Decimal(covered) * Decimal("100") / Decimal(total)
return str(pct.quantize(Decimal("0.01"), rounding=ROUND_HALF_UP))
def get_line_coverage_percent_from_cobertura(xml_file: Path) -> str:
"""Return overall line coverage % from a Cobertura XML file."""
try:
root = etree.parse(str(xml_file)).getroot()
except (FileNotFoundError, PermissionError) as e:
typer.echo(f"Could not read {xml_file}: {e}", err=True)
raise typer.Exit(1) from e
except etree.XMLSyntaxError as e:
typer.echo(f"Invalid XML in {xml_file}: {e}", err=True)
raise typer.Exit(1) from e
# count all lines and those with hits > 0
total = int(root.xpath("count(//class/lines/line)"))
covered = int(root.xpath("count(//class/lines/line[@hits>0])"))
# fallback to root attributes if no <class> info
if total == 0:
total = int(root.xpath("number(/coverage/@lines-valid)"))
covered = int(root.xpath("number(/coverage/@lines-covered)"))
if total == 0:
return "0.00"
return _format_percent(covered, total)
```
This keeps all functionality, removes nested helpers and broad catches, and uses clear XPath expressions with one small formatting utility.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
leynos
deleted the
codex/add-error-handling-to-get_line_coverage_percent_from_cobertu
branch
![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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/actions/generate-coverage/scripts/run_rust.py(2 hunks)
🔇 Additional comments (5)
.github/actions/generate-coverage/scripts/run_rust.py (5)
11-11: Import changes look good.The addition of
Decimalimports for precise percentage calculations and the removal of the external Cobertura parser import align well with the PR's objective to implement defensive parsing internally.Also applies to: 15-16
17-24: Excellent defensive handling of the lxml dependency.The runtime import with clear error messaging provides exactly the graceful fallback mechanism described in the PR objectives.
62-65: Well-structured percentage formatting helper.Good extraction of the formatting logic with precise decimal arithmetic and consistent rounding behaviour.
128-129: Clean integration of the new Cobertura parser.The local implementation seamlessly replaces the external dependency while maintaining the existing control flow.
4-4: Verify the interaction between uv dependency declaration and runtime availability.The script header declares
lxmlas a dependency for uv, but the runtime import check suggests it might still be missing. Confirm that the runtime check is necessary when using uv, or document when this fallback would trigger.Also applies to: 17-24
Comment on lines
+67
to
+97
| def get_line_coverage_percent_from_cobertura(xml_file: Path) -> str: | ||
| """Return overall line coverage % from a Cobertura XML file.""" | ||
| try: | ||
| root = etree.parse(str(xml_file)).getroot() | ||
| except (FileNotFoundError, PermissionError) as exc: | ||
| typer.echo(f"Could not read {xml_file}: {exc}", err=True) | ||
| raise typer.Exit(1) from exc | ||
| except etree.XMLSyntaxError as exc: | ||
| typer.echo(f"Invalid XML in {xml_file}: {exc}", err=True) | ||
| raise typer.Exit(1) from exc | ||
|
|
||
| try: | ||
| total = int(root.xpath("count(//class/lines/line)")) | ||
| covered = int(root.xpath("count(//class/lines/line[@hits>0])")) | ||
| except etree.XPathError as exc: | ||
| typer.echo(f"Malformed Cobertura data: {exc}", err=True) | ||
| raise typer.Exit(1) from exc | ||
|
|
||
| if total == 0: | ||
| try: | ||
| covered = int(root.xpath("number(/coverage/@lines-covered)")) | ||
| total = int(root.xpath("number(/coverage/@lines-valid)")) | ||
| except etree.XPathError as exc: | ||
| typer.echo(f"Cobertura summary missing: {exc}", err=True) | ||
| raise typer.Exit(1) from exc | ||
|
|
||
| if total == 0: | ||
| return "0.00" | ||
|
|
||
| return _format_percent(covered, total) | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Swap the order of lines 87-88 for consistency.
The implementation provides excellent error handling that addresses the past review concerns. However, maintain consistent ordering by extracting total before covered:
- covered = int(root.xpath("number(/coverage/@lines-covered)"))
- total = int(root.xpath("number(/coverage/@lines-valid)"))
+ total = int(root.xpath("number(/coverage/@lines-valid)"))
+ covered = int(root.xpath("number(/coverage/@lines-covered)"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def get_line_coverage_percent_from_cobertura(xml_file: Path) -> str: | |
| """Return overall line coverage % from a Cobertura XML file.""" | |
| try: | |
| root = etree.parse(str(xml_file)).getroot() | |
| except (FileNotFoundError, PermissionError) as exc: | |
| typer.echo(f"Could not read {xml_file}: {exc}", err=True) | |
| raise typer.Exit(1) from exc | |
| except etree.XMLSyntaxError as exc: | |
| typer.echo(f"Invalid XML in {xml_file}: {exc}", err=True) | |
| raise typer.Exit(1) from exc | |
| try: | |
| total = int(root.xpath("count(//class/lines/line)")) | |
| covered = int(root.xpath("count(//class/lines/line[@hits>0])")) | |
| except etree.XPathError as exc: | |
| typer.echo(f"Malformed Cobertura data: {exc}", err=True) | |
| raise typer.Exit(1) from exc | |
| if total == 0: | |
| try: | |
| covered = int(root.xpath("number(/coverage/@lines-covered)")) | |
| total = int(root.xpath("number(/coverage/@lines-valid)")) | |
| except etree.XPathError as exc: | |
| typer.echo(f"Cobertura summary missing: {exc}", err=True) | |
| raise typer.Exit(1) from exc | |
| if total == 0: | |
| return "0.00" | |
| return _format_percent(covered, total) | |
| def get_line_coverage_percent_from_cobertura(xml_file: Path) -> str: | |
| """Return overall line coverage % from a Cobertura XML file.""" | |
| try: | |
| root = etree.parse(str(xml_file)).getroot() | |
| except (FileNotFoundError, PermissionError) as exc: | |
| typer.echo(f"Could not read {xml_file}: {exc}", err=True) | |
| raise typer.Exit(1) from exc | |
| except etree.XMLSyntaxError as exc: | |
| typer.echo(f"Invalid XML in {xml_file}: {exc}", err=True) | |
| raise typer.Exit(1) from exc | |
| try: | |
| total = int(root.xpath("count(//class/lines/line)")) | |
| covered = int(root.xpath("count(//class/lines/line[@hits>0])")) | |
| except etree.XPathError as exc: | |
| typer.echo(f"Malformed Cobertura data: {exc}", err=True) | |
| raise typer.Exit(1) from exc | |
| if total == 0: | |
| try: | |
| total = int(root.xpath("number(/coverage/@lines-valid)")) | |
| covered = int(root.xpath("number(/coverage/@lines-covered)")) | |
| except etree.XPathError as exc: | |
| typer.echo(f"Cobertura summary missing: {exc}", err=True) | |
| raise typer.Exit(1) from exc | |
| if total == 0: | |
| return "0.00" | |
| return _format_percent(covered, total) |
🤖 Prompt for AI Agents
In .github/actions/generate-coverage/scripts/run_rust.py between lines 67 and
97, swap the order of the XPath extraction lines so that the variable total is
assigned before covered in the block where total is zero. This means extracting
total first using the XPath expression for lines-valid, then extracting covered
using lines-covered. This change maintains consistent ordering of variable
assignments for clarity and readability.
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
lxmldependency gracefullyTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68871f6211bc8322a621281e6b25b6e7
Summary by Sourcery
Add a defensive Cobertura XML parsing function to extract overall line coverage percentage with robust error handling.
New Features:
Enhancements: