Conversation
Contributor
Reviewer's GuideThis PR refactors the generate-coverage and rust-build-release test suites to drop custom stubs for Typer, plumbum, lxml and related modules in favor of loading real dependencies. It simplifies the dynamic module loader by prepending actual paths, removes extensive fake‐module setup, and updates fixtures, CLI invocations and exit assertions to use real Typer integration. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
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. Summary by CodeRabbit
WalkthroughReplace mocked Typer and stubbed dependencies in tests with real Typer and real imports. Unify exit handling by extracting exit codes from Exit exceptions. Simplify module loading in coverage tests. Add conditional run_cmd patching in rust-build-release test harness. Switch CLI tests to Typer’s CliRunner and assert errors on stderr. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test
participant L as _load_module
participant I as Python Import System
participant S as scripts/run_*
Note over T,L: New flow: import with real dependencies
T->>L: request module "run_rust" / "run_python"
L->>I: purge cache for target and coverage_parsers
L->>I: import scripts.<name>
I-->>S: load module with real deps
S-->>L: module object
L-->>T: return ModuleType
T->>S: invoke CLI via Typer
S-->>T: raise typer.Exit (exit_code/code)
T->>T: assert extracted exit code
sequenceDiagram
autonumber
participant TH as Test Harness (module_harness)
participant M as Target Module
participant H as Harness
Note over TH,M: Conditional run_cmd patching
TH->>M: inspect for attribute run_cmd
alt run_cmd present
TH->>H: patch_run_cmd()
H-->>TH: patched
else run_cmd absent
TH->>TH: skip patching
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consolidate the repeated exit code extraction logic (getattr on
exit_codeorcode) into a small helper or assertion function to reduce boilerplate and improve readability across tests. - Since you’re using
typer.testing.CliRunnerwithprog_namein multiple tests, consider defining a default runner (or fixture) that sets theprog_nameonce to avoid repeating it in each invocation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consolidate the repeated exit code extraction logic (getattr on `exit_code` or `code`) into a small helper or assertion function to reduce boilerplate and improve readability across tests.
- Centralize the module‐loading monkeypatch logic (syspath prepends and module cleanup) into a shared fixture in conftest to DRY up setup and simplify individual test modules.
- Since you’re using `typer.testing.CliRunner` with `prog_name` in multiple tests, consider defining a default runner (or fixture) that sets the `prog_name` once to avoid repeating it in each invocation.
## Individual Comments
### Comment 1
<location> `.github/actions/generate-coverage/tests/test_scripts.py:481-485` </location>
<code_context>
-def test_lcov_file_missing(tmp_path: Path, run_rust_module: types.ModuleType) -> None:
+def test_lcov_file_missing(tmp_path: Path, run_rust_module: ModuleType) -> None:
"""Non-existent file triggers ``SystemExit``."""
- with pytest.raises(SystemExit) as excinfo:
+ with pytest.raises(run_rust_module.typer.Exit) as excinfo:
run_rust_module.get_line_coverage_percent_from_lcov(tmp_path / "nope.lcov")
- assert excinfo.value.code == 1
+ exit_code = getattr(excinfo.value, "exit_code", None) or getattr(
+ excinfo.value, "code", None
+ )
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for when the lcov file exists but is empty.
Adding a test for an empty lcov file will help verify that the function handles this edge case correctly and returns the appropriate result or exit code.
</issue_to_address>
### Comment 2
<location> `.github/actions/generate-coverage/tests/test_scripts.py:518` </location>
<code_context>
-def test_cobertura_detail(tmp_path: Path, run_python_module: types.ModuleType) -> None:
+def test_cobertura_detail(tmp_path: Path, run_python_module: ModuleType) -> None:
"""``get_line_coverage_percent_from_cobertura`` handles per-line detail."""
xml = tmp_path / "cov.xml"
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for malformed Cobertura XML files.
Tests for malformed lcov files exist, but similar coverage for Cobertura XML is missing. Adding tests for cases like missing elements or invalid structure would improve error handling in the parser.
</issue_to_address>
### Comment 3
<location> `.github/actions/generate-coverage/tests/test_detect.py:30` </location>
<code_context>
+def test_invalid_format(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing for valid formats with empty or malformed output files.
Currently, only error handling for the 'lcov' format is tested. Please add cases for empty and malformed output files to improve coverage of error scenarios.
</issue_to_address>
### Comment 4
<location> `.github/actions/rust-build-release/tests/test_action_setup.py:104` </location>
<code_context>
+ assert "contains invalid characters" in result.stderr
def test_script_validate_step_reports_error() -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding assertions to 'test_script_validate_step_reports_error'.
The test only has a 'pass' statement. Please add assertions to check the expected error, or remove the test if it's unnecessary.
Suggested implementation:
```python
def test_script_validate_step_reports_error() -> None:
result = runner.invoke(
action_setup_module.app,
["validate-step", "invalid step"],
prog_name="action-setup",
)
assert result.exit_code != 0
assert "contains invalid characters" in result.stderr
```
If `runner` and `action_setup_module` are not available in the scope of this test, you will need to import or define them as in the previous test. Also, ensure that `"validate-step"` and `"invalid step"` are the correct arguments for triggering the error you want to test.
</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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/generate-coverage/tests/test_scripts.py (1)
206-209: Fix exit-code extraction (0 is falsy).Use None checks rather than
or.Apply this diff:
- assert ( - getattr(excinfo.value, "exit_code", None) - or getattr(excinfo.value, "code", None) - ) == 1 + code = getattr(excinfo.value, "exit_code", None) + if code is None: + code = getattr(excinfo.value, "code", None) + assert code == 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/actions/generate-coverage/tests/test_detect.py(2 hunks).github/actions/generate-coverage/tests/test_scripts.py(11 hunks).github/actions/rust-build-release/tests/conftest.py(1 hunks).github/actions/rust-build-release/tests/test_action_setup.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
.github/actions/rust-build-release/tests/test_action_setup.py (1)
.github/actions/rust-build-release/tests/conftest.py (1)
action_setup_module(203-209)
.github/actions/generate-coverage/tests/test_scripts.py (2)
.github/actions/rust-build-release/tests/conftest.py (1)
_load_module(50-67).github/actions/generate-coverage/scripts/coverage_parsers.py (1)
get_line_coverage_percent_from_lcov(92-123)
⏰ 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). (3)
- GitHub Check: Sourcery review
- GitHub Check: python-tests (ubuntu-latest)
- GitHub Check: python-tests (windows-latest)
🔇 Additional comments (8)
.github/actions/rust-build-release/tests/test_action_setup.py (3)
11-11: Switch to Typer’s CliRunner is correct.Import
CliRunnerfromtyper.testingto align with Typer’s API.
76-88: Invoke with prog_name to stabilise usage text.Passing
prog_name="action-setup"is appropriate for consistent error/help output.
95-101: Assert error on stderr (correct stream).Validly check
result.stderrfor the error text; Typer emits errors on stderr..github/actions/rust-build-release/tests/conftest.py (1)
118-120: Guard run_cmd patching to avoid AttributeError.Patch
run_cmdonly when present on the target module to keep the harness generic..github/actions/generate-coverage/tests/test_scripts.py (4)
16-17: Type-hint ModuleType under TYPE_CHECKING only.Keep runtime clean while preserving types.
173-173: Use the new loader consistently.Load
run_rustvia_load_moduleacross Windows‑specific tests to exercise real deps.Also applies to: 195-195, 216-216, 239-239, 252-252
458-460: LCOV helpers: behaviour assertions read cleanly.Tests for zero/missing/malformed data match parser semantics; keep as is.
Also applies to: 467-472, 474-479
512-516: Python/Cobertura tests align with real imports.Fixtures and assertions are coherent with the new import strategy.
Also applies to: 518-541, 544-551, 554-561
Owner
Author
|
@coderabbitai pause |
Contributor
✅ Actions performedReviews paused. |
leynos
force-pushed
the
codex/refactor-rust-build-release-tests-with-pytest-mock
branch
from
8b87b76 to
4856575
Compare
leynos
deleted the
codex/refactor-rust-build-release-tests-with-pytest-mock
branch
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
Testing
https://chatgpt.com/codex/tasks/task_e_68cb3dc058a0832298a90dd7a95c12ec
Summary by Sourcery
Refactor test suites to rely on real Typer integration, simplify module loading, and standardize CLI testing with Typer's runner.
Enhancements:
Tests: