test: add coverage for add_pr_comments PyGithub interaction paths - #243
test: add coverage for add_pr_comments PyGithub interaction paths#243shenxianpeng wants to merge 5 commits into
Conversation
The add_pr_comments function had zero test coverage for its core PyGithub interaction logic. Only the disabled and fork-pr early-return paths were tested. Add 8 new test cases covering: - Creating a new comment when none exists (success and failure paths) - Skipping update when comment is already up-to-date - Updating the last comment and deleting stale older ones - Missing GITHUB_TOKEN - GithubException(403) with warning output - GithubException(non-403) with error output - Generic Exception handling
📝 WalkthroughWalkthroughThe pull request adds comprehensive ChangesTest coverage and CI updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Commit-Check ✔️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main_test.py`:
- Around line 853-868: Update test_generic_exception_returns_zero to assert that
the captured final print message includes the ValueError detail "unexpected
error" in addition to the existing "Error posting PR comment" context. Keep the
current return-code and exception setup unchanged.
- Around line 796-807: Update test_missing_token_returns_zero to preserve
GITHUB_REPOSITORY while setting GITHUB_TOKEN to an empty value instead of
mocking every os.getenv call. Capture the mocked builtins.print output and
assert it mentions GITHUB_TOKEN, while retaining the rc == 0 assertion to verify
the intended missing-token branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| def test_missing_token_returns_zero(self): | ||
| """GITHUB_TOKEN not set: catches ValueError, returns 0.""" | ||
| with ( | ||
| patch("main.PR_COMMENTS_ENABLED", True), | ||
| patch("main.is_fork_pr", return_value=False), | ||
| patch("main.get_pr_number", return_value=42), | ||
| patch.dict(os.environ, {"GITHUB_REPOSITORY": "test/repo"}), | ||
| patch("os.getenv", return_value=None), | ||
| patch("builtins.print"), | ||
| ): | ||
| rc = main.add_pr_comments() | ||
| self.assertEqual(rc, 0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the missing-token test prove the intended branch.
Mocking every os.getenv lookup and checking only rc == 0 allows unrelated failures to satisfy this test. Set GITHUB_TOKEN to an empty value while preserving GITHUB_REPOSITORY, then assert the output mentions GITHUB_TOKEN.
Proposed test adjustment
- patch.dict(os.environ, {"GITHUB_REPOSITORY": "test/repo"}),
- patch("os.getenv", return_value=None),
- patch("builtins.print"),
+ patch.dict(
+ os.environ,
+ {"GITHUB_TOKEN": "", "GITHUB_REPOSITORY": "test/repo"},
+ ),
+ patch("builtins.print") as mock_print,
...
self.assertEqual(rc, 0)
+ error_text = mock_print.call_args_list[-1][0][0]
+ self.assertIn("GITHUB_TOKEN", error_text)📝 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.
| def test_missing_token_returns_zero(self): | |
| """GITHUB_TOKEN not set: catches ValueError, returns 0.""" | |
| with ( | |
| patch("main.PR_COMMENTS_ENABLED", True), | |
| patch("main.is_fork_pr", return_value=False), | |
| patch("main.get_pr_number", return_value=42), | |
| patch.dict(os.environ, {"GITHUB_REPOSITORY": "test/repo"}), | |
| patch("os.getenv", return_value=None), | |
| patch("builtins.print"), | |
| ): | |
| rc = main.add_pr_comments() | |
| self.assertEqual(rc, 0) | |
| def test_missing_token_returns_zero(self): | |
| """GITHUB_TOKEN not set: catches ValueError, returns 0.""" | |
| with ( | |
| patch("main.PR_COMMENTS_ENABLED", True), | |
| patch("main.is_fork_pr", return_value=False), | |
| patch("main.get_pr_number", return_value=42), | |
| patch.dict( | |
| os.environ, | |
| {"GITHUB_TOKEN": "", "GITHUB_REPOSITORY": "test/repo"}, | |
| ), | |
| patch("builtins.print") as mock_print, | |
| ): | |
| rc = main.add_pr_comments() | |
| self.assertEqual(rc, 0) | |
| error_text = mock_print.call_args_list[-1][0][0] | |
| self.assertIn("GITHUB_TOKEN", error_text) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main_test.py` around lines 796 - 807, Update test_missing_token_returns_zero
to preserve GITHUB_REPOSITORY while setting GITHUB_TOKEN to an empty value
instead of mocking every os.getenv call. Capture the mocked builtins.print
output and assert it mentions GITHUB_TOKEN, while retaining the rc == 0
assertion to verify the intended missing-token branch.
| def test_generic_exception_returns_zero(self): | ||
| """Generic Exception: prints error, returns 0.""" | ||
| mock_github, mock_repo, _ = self._mock_github_chain() | ||
| mock_repo.get_issue.side_effect = ValueError("unexpected error") | ||
| with ( | ||
| patch("main.PR_COMMENTS_ENABLED", True), | ||
| patch("main.is_fork_pr", return_value=False), | ||
| patch("github.Github", return_value=mock_github), | ||
| patch("main.get_pr_number", return_value=42), | ||
| self._patch_github_env(), | ||
| patch("builtins.print") as mock_print, | ||
| ): | ||
| rc = main.add_pr_comments() | ||
| self.assertEqual(rc, 0) | ||
| error_text = mock_print.call_args_list[-1][0][0] | ||
| self.assertIn("Error posting PR comment", error_text) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the generic exception context.
The test should verify that "unexpected error" is included in the printed message; otherwise it only proves that some generic error prefix was emitted.
Proposed assertion
self.assertIn("Error posting PR comment", error_text)
+ self.assertIn("unexpected error", error_text)📝 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.
| def test_generic_exception_returns_zero(self): | |
| """Generic Exception: prints error, returns 0.""" | |
| mock_github, mock_repo, _ = self._mock_github_chain() | |
| mock_repo.get_issue.side_effect = ValueError("unexpected error") | |
| with ( | |
| patch("main.PR_COMMENTS_ENABLED", True), | |
| patch("main.is_fork_pr", return_value=False), | |
| patch("github.Github", return_value=mock_github), | |
| patch("main.get_pr_number", return_value=42), | |
| self._patch_github_env(), | |
| patch("builtins.print") as mock_print, | |
| ): | |
| rc = main.add_pr_comments() | |
| self.assertEqual(rc, 0) | |
| error_text = mock_print.call_args_list[-1][0][0] | |
| self.assertIn("Error posting PR comment", error_text) | |
| def test_generic_exception_returns_zero(self): | |
| """Generic Exception: prints error, returns 0.""" | |
| mock_github, mock_repo, _ = self._mock_github_chain() | |
| mock_repo.get_issue.side_effect = ValueError("unexpected error") | |
| with ( | |
| patch("main.PR_COMMENTS_ENABLED", True), | |
| patch("main.is_fork_pr", return_value=False), | |
| patch("github.Github", return_value=mock_github), | |
| patch("main.get_pr_number", return_value=42), | |
| self._patch_github_env(), | |
| patch("builtins.print") as mock_print, | |
| ): | |
| rc = main.add_pr_comments() | |
| self.assertEqual(rc, 0) | |
| error_text = mock_print.call_args_list[-1][0][0] | |
| self.assertIn("Error posting PR comment", error_text) | |
| self.assertIn("unexpected error", error_text) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main_test.py` around lines 853 - 868, Update
test_generic_exception_returns_zero to assert that the captured final print
message includes the ValueError detail "unexpected error" in addition to the
existing "Error posting PR comment" context. Keep the current return-code and
exception setup unchanged.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
faf2540 to
4c3c694
Compare
d335cf9 to
e69fedd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/commit-check.yml:
- Around line 19-21: Disable persisted checkout credentials by adding
persist-credentials: false to the actions/checkout step in
.github/workflows/commit-check.yml at lines 19-21 and
.github/workflows/coverage.yml at lines 16-19; no other workflow behavior should
change.
In @.gitignore:
- Around line 1-4: Add coverage.xml to the coverage-related entries in
.gitignore so the generated XML report is excluded from version control
alongside the existing coverage artifacts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b23260a-f7c4-4717-be9d-b672c9754b6a
📒 Files selected for processing (4)
.github/workflows/commit-check.yml.github/workflows/coverage.yml.gitignoreREADME.md
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| fetch-depth: 0 # Required for merge-base checks |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow files =="
git ls-files '.github/workflows/*.yml' '.github/workflows/*.yaml' || true
echo "== relevant checkout lines =="
sed -n '1,120p' .github/workflows/commit-check.yml 2>/dev/null || true
printf '\n--- coverage ---\n'
sed -n '1,140p' .github/workflows/coverage.yml 2>/dev/null || true
echo "== persist-credentials occurrences =="
rg -n "persist-credentials|actions/checkout|pull-requests: write|permissions:" .github/workflows || trueRepository: commit-check/commit-check-action
Length of output: 3120
🌐 Web query:
actions/checkout persist-credentials default true README
💡 Result:
In the actions/checkout GitHub Action, the persist-credentials input defaults to true [1][2]. When set to true, the action configures the authentication token or SSH key with the local Git configuration [1][2]. This enables your workflow steps to run authenticated Git commands, such as git fetch or git push, without needing to manually handle credentials [1][2][3]. Security and Usage Details: - Removal: The credentials are removed during the post-job cleanup process [1][2]. - Improved Storage: As of version v6.0.0, the action was updated to store these credentials in a separate file under $RUNNER_TEMP instead of directly in the.git/config file, improving security [3][4]. - Opt-out: You can disable this behavior by setting persist-credentials: false in your workflow step [1][2].
Citations:
- 1: https://github.com/actions/checkout/blob/main/README.md
- 2: https://github.com/actions/checkout?tab=readme-ov-file
- 3: https://github.com/marketplace/actions/checkout
- 4: https://github.com/actions/checkout/tree/v6.0.0
Disable persisted checkout credentials in both workflows.
Both workflow steps leave the GitHub token as the default configured Git credentials for later repository-controlled steps. Set persist-credentials: false unless auth is explicitly required by subsequent steps.
.github/workflows/commit-check.yml#L19-L21: addpersist-credentials: false; this job also haspull-requests: write..github/workflows/coverage.yml#L16-L19: addpersist-credentials: false.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 19-21: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 2 files
.github/workflows/commit-check.yml#L19-L21(this comment).github/workflows/coverage.yml#L16-L19
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/commit-check.yml around lines 19 - 21, Disable persisted
checkout credentials by adding persist-credentials: false to the
actions/checkout step in .github/workflows/commit-check.yml at lines 19-21 and
.github/workflows/coverage.yml at lines 16-19; no other workflow behavior should
change.
Source: Linters/SAST tools
| .coverage | ||
| .coverage.* | ||
| *.pyc | ||
| __pycache__/ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ignore the generated XML coverage report.
The workflow creates coverage.xml at line 26, but this ignore file does not exclude it, so local coverage runs can leave an untracked generated artifact.
Proposed fix
.coverage
.coverage.*
+coverage.xml
*.pyc
__pycache__/📝 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.
| .coverage | |
| .coverage.* | |
| *.pyc | |
| __pycache__/ | |
| .coverage | |
| .coverage.* | |
| coverage.xml | |
| *.pyc | |
| __pycache__/ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.gitignore around lines 1 - 4, Add coverage.xml to the coverage-related
entries in .gitignore so the generated XML report is excluded from version
control alongside the existing coverage artifacts.
2 participants
Summary
The
add_pr_comments()function (main.py:386-469) is one of the Action's most complex features — it manages creating, updating, and deleting PR comments via PyGithub, with error handling for 403, missing tokens, and generic exceptions.Despite this complexity, only the early-return paths (disabled, fork PR) were tested. The core PyGithub interaction had zero coverage.
Changes
Tests
Added 8 new test cases to
TestAddPrCommentsinmain_test.py:test_new_comment_created_when_failuretest_new_comment_created_when_successtest_comment_up_to_date_skipstest_comment_updated_old_deletedtest_missing_token_returns_zeroGITHUB_TOKENnot set → catches ValueError, returns 0test_403_forbidden_returns_warningGithubException(403)→ warning, returns 0test_github_exception_returns_zeroGithubException(non-403)→ error, returns 0test_generic_exception_returns_zeroException→ error, returns 0CI
.github/workflows/coverage.yml— runspytest --covon PRs and pushes to main, uploads to Codecov.gitignoreto prevent.coverage/__pycache__from being committedVerification
unittest.mockto simulate the full PyGithub chain without network callsSummary by CodeRabbit
Tests
Documentation
Chores