feat: add _is_version_installed function and corresponding tests#187
feat: add _is_version_installed function and corresponding tests#187shenxianpeng merged 1 commit intomainfrom
_is_version_installed function and corresponding tests#187Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #187 +/- ##
==========================================
+ Coverage 95.41% 95.72% +0.31%
==========================================
Files 4 4
Lines 109 117 +8
==========================================
+ Hits 104 112 +8
Misses 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdded a private helper Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_util.py (1)
336-351:⚠️ Potential issue | 🟡 MinorTest may not reflect actual code behavior when version is
None.This test mocks
_install_toolbut not_is_version_installed. Withshutil.whichreturningNone,_is_version_installedreturnsNonewithout hitting the version check, so the test passes. However, ifshutil.whichreturned a valid path,_is_version_installedwould crash onversion in result.stdoutwithNoneversion.Consider either:
- Fixing
_is_version_installedto handleNone(as suggested in util.py), or- Explicitly mocking
_is_version_installedin this test to make the test intent clearer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_util.py` around lines 336 - 351, The test test_resolve_install_with_none_default_version is relying on shutil.which returning None so _is_version_installed is never exercised; to make the test intent explicit, mock cpp_linter_hooks.util._is_version_installed in the test to return False (or an appropriate sentinel) so resolve_install("clang-format", None) follows the install path and you can assert _install_tool was called; alternatively, if you prefer a code fix, update util._is_version_installed to guard against a None version (e.g., return False immediately if version is None) so calling _is_version_installed(version=None, ...) cannot crash when it checks result.stdout.
🧹 Nitpick comments (2)
tests/test_util.py (1)
121-163: Consider adding a test forNoneversion input.The tests cover the main scenarios well. However, there's no test for when
versionisNone, which can occur whenpyproject.tomlis missing and defaults are unavailable. Adding this test would ensure the edge case is properly handled (once the fix is applied).🧪 Suggested additional test
`@pytest.mark.benchmark` def test_is_version_installed_none_version(): """Test _is_version_installed when version is None.""" mock_path = "/usr/bin/clang-format" with patch("shutil.which", return_value=mock_path): result = _is_version_installed("clang-format", None) assert result is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_util.py` around lines 121 - 163, Add a unit test to cover the edge case where version is None for the helper _is_version_installed: simulate the tool existing in PATH by patching shutil.which to return a mock path (e.g., "/usr/bin/clang-format"), call _is_version_installed("clang-format", None) and assert it returns None; place this new test (e.g., test_is_version_installed_none_version) alongside the other tests in tests/test_util.py and follow the same pytest.mark.benchmark decoration and mocking style used by test_is_version_installed_not_in_path.cpp_linter_hooks/util.py (1)
67-69: Substring version check may produce false positives.Using
version in result.stdoutcan incorrectly match similar versions. For example, checking for"20.1.7"would match output containing"20.1.70". Consider using a word-boundary check or parsing the version more precisely.♻️ Suggested fix using regex word boundary
+import re + def _is_version_installed(tool: str, version: str) -> Optional[Path]: """Return the tool path if the installed version matches, otherwise None.""" existing = shutil.which(tool) if not existing: return None result = subprocess.run([existing, "--version"], capture_output=True, text=True) - if version in result.stdout: + if re.search(rf"\b{re.escape(version)}\b", result.stdout): return Path(existing) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp_linter_hooks/util.py` around lines 67 - 69, The current substring check "version in result.stdout" can yield false positives; update the check in util.py where subprocess.run([...], ...) returns result to perform a precise match (e.g., use regex word-boundary matching or parse the version) — replace the naive membership test on result.stdout with a call like re.search(rf'\\b{re.escape(version)}\\b', result.stdout) (and import re) or otherwise parse the tool's version output to compare exact components, keeping variables existing, version, and result as the referenced identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp_linter_hooks/util.py`:
- Around line 62-70: The _is_version_installed function can raise TypeError when
its version parameter is None; update _is_version_installed to explicitly guard
against a None version before checking membership in result.stdout (the check
"version in result.stdout"), e.g., return None immediately if version is None or
convert to a safe string, so the subprocess output check only runs when version
is a non-None string; modify the function surrounding the variables existing,
result, and version to perform this None check and return None early if needed.
---
Outside diff comments:
In `@tests/test_util.py`:
- Around line 336-351: The test test_resolve_install_with_none_default_version
is relying on shutil.which returning None so _is_version_installed is never
exercised; to make the test intent explicit, mock
cpp_linter_hooks.util._is_version_installed in the test to return False (or an
appropriate sentinel) so resolve_install("clang-format", None) follows the
install path and you can assert _install_tool was called; alternatively, if you
prefer a code fix, update util._is_version_installed to guard against a None
version (e.g., return False immediately if version is None) so calling
_is_version_installed(version=None, ...) cannot crash when it checks
result.stdout.
---
Nitpick comments:
In `@cpp_linter_hooks/util.py`:
- Around line 67-69: The current substring check "version in result.stdout" can
yield false positives; update the check in util.py where subprocess.run([...],
...) returns result to perform a precise match (e.g., use regex word-boundary
matching or parse the version) — replace the naive membership test on
result.stdout with a call like re.search(rf'\\b{re.escape(version)}\\b',
result.stdout) (and import re) or otherwise parse the tool's version output to
compare exact components, keeping variables existing, version, and result as the
referenced identifiers.
In `@tests/test_util.py`:
- Around line 121-163: Add a unit test to cover the edge case where version is
None for the helper _is_version_installed: simulate the tool existing in PATH by
patching shutil.which to return a mock path (e.g., "/usr/bin/clang-format"),
call _is_version_installed("clang-format", None) and assert it returns None;
place this new test (e.g., test_is_version_installed_none_version) alongside the
other tests in tests/test_util.py and follow the same pytest.mark.benchmark
decoration and mocking style used by test_is_version_installed_not_in_path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a7e2a53-1018-4a1d-8ec7-097cd21f385b
📒 Files selected for processing (2)
cpp_linter_hooks/util.pytests/test_util.py
Merging this PR will degrade performance by 28.84%
|
shenxianpeng
force-pushed
the
fix/skip-install
branch
from
ccf80d5 to
db23d05
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_util.py (1)
336-351:⚠️ Potential issue | 🟡 MinorMisleading comment and potential production issue.
The comment states "Should fallback to hardcoded version when DEFAULT is None" but the assertion shows
_install_toolis called withNoneas the version. This would result inpip install clang-format==None, which will fail.Either:
- The production code should have a hardcoded fallback (and test updated to reflect that), or
- The comment should be corrected to reflect the actual behavior
✏️ Option 1: Fix comment to match actual behavior
- # Should fallback to hardcoded version when DEFAULT is None + # When DEFAULT is None and no version specified, None is passed through mock_install.assert_called_once_with("clang-format", None)✏️ Option 2: Add hardcoded fallback in production code and update test
In
cpp_linter_hooks/util.py, add a fallback:FALLBACK_VERSION = "20.1.0" # Or appropriate defaultThen in
resolve_install, after settinguser_versionfrom defaults:if user_version is None: user_version = FALLBACK_VERSIONAnd update this test's assertion accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_util.py` around lines 336 - 351, The test comment and assertion are inconsistent with production behavior: resolve_install currently passes None to _install_tool when DEFAULT_CLANG_FORMAT_VERSION/DEFAULT_CLANG_TIDY_VERSION are None, which would try to install "==None"; fix by adding a real fallback version constant (e.g., FALLBACK_VERSION) in cpp_linter_hooks.util and update resolve_install to use FALLBACK_VERSION when the default/user version is None, then update test_resolve_install_with_none_default_version to assert mock_install.assert_called_once_with("clang-format", FALLBACK_VERSION) (or alternatively, if you prefer to keep current behavior, change the test comment to state that None is forwarded to _install_tool and leave resolve_install unchanged).
🧹 Nitpick comments (1)
tests/test_util.py (1)
121-164: Add a test case forNoneversion parameter.The tests cover the main scenarios well, but there's no test verifying behavior when
versionisNone. This would help validate the fix for the potentialTypeErrorin_is_version_installed.🧪 Proposed additional test
`@pytest.mark.benchmark` def test_is_version_installed_none_version(): """Test _is_version_installed when version is None.""" mock_path = "/usr/bin/clang-format" with patch("shutil.which", return_value=mock_path): result = _is_version_installed("clang-format", None) assert result is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_util.py` around lines 121 - 164, Add a new test in tests/test_util.py that calls _is_version_installed("clang-format", None) and asserts it returns None; mock shutil.which to return a fake path (e.g., "/usr/bin/clang-format") so the function proceeds to the early-version handling and ensure no exception is raised—place the test alongside the other test_is_version_installed_* tests and use the same pytest.mark.benchmark decorator and patching style used in test_is_version_installed_version_matches to keep consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/test_util.py`:
- Around line 336-351: The test comment and assertion are inconsistent with
production behavior: resolve_install currently passes None to _install_tool when
DEFAULT_CLANG_FORMAT_VERSION/DEFAULT_CLANG_TIDY_VERSION are None, which would
try to install "==None"; fix by adding a real fallback version constant (e.g.,
FALLBACK_VERSION) in cpp_linter_hooks.util and update resolve_install to use
FALLBACK_VERSION when the default/user version is None, then update
test_resolve_install_with_none_default_version to assert
mock_install.assert_called_once_with("clang-format", FALLBACK_VERSION) (or
alternatively, if you prefer to keep current behavior, change the test comment
to state that None is forwarded to _install_tool and leave resolve_install
unchanged).
---
Nitpick comments:
In `@tests/test_util.py`:
- Around line 121-164: Add a new test in tests/test_util.py that calls
_is_version_installed("clang-format", None) and asserts it returns None; mock
shutil.which to return a fake path (e.g., "/usr/bin/clang-format") so the
function proceeds to the early-version handling and ensure no exception is
raised—place the test alongside the other test_is_version_installed_* tests and
use the same pytest.mark.benchmark decorator and patching style used in
test_is_version_installed_version_matches to keep consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5b075c9-2054-40b6-83a6-2df7c60e5199
📒 Files selected for processing (2)
cpp_linter_hooks/util.pytests/test_util.py
There was a problem hiding this comment.
Pull request overview
This PR adds a utility helper to detect whether a requested clang tool version is already present on PATH and updates the install resolution logic (plus tests) to reuse an existing matching installation instead of always reinstalling.
Changes:
- Added
_is_version_installed(tool, version)to checktool --versionoutput and return the executable path on a match. - Updated
resolve_install()to prefer an already-installed matching version before falling back to_install_tool(). - Extended
tests/test_util.pyto cover version-detection and correct/mismatched scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
cpp_linter_hooks/util.py |
Adds _is_version_installed and updates resolve_install to short-circuit installation when a matching version is already available. |
tests/test_util.py |
Adds tests for _is_version_installed and adjusts resolve_install tests to account for version checking via --version. |
You can also share your feedback on Copilot code review. Take the survey.
shenxianpeng
force-pushed
the
fix/skip-install
branch
from
db23d05 to
19d86c3
Compare
|
Summary by CodeRabbit
Refactor
Tests