test: increase test coverage from 87% to 99%#168
Merged
Conversation
Add 30 new test cases covering previously untested code paths: test_util.py additions: - test_version_non_numeric: Version() with non-numeric input - test_version_full_semver: full semantic version parsing - test_version_major_only: major-only version parsing - test_version_major_minor: major.minor version parsing - test_verify_sha512_valid/invalid/with_filename: sha512 verification - test_download_file_http_error: HTTP error handling - test_download_file_value_error: invalid URL handling - test_download_file_bad_status: non-200 status handling - test_download_file_with_progress_bar: progress bar code path - test_check_install_os_unsupported: unsupported OS check test_install.py additions: - test_is_installed_found: tool detection success path - test_is_installed_wrong_major_version: version mismatch - test_is_installed_not_found_in_path: shutil.which returns None - test_install_tool_sha512_mismatch: sha512 invalid triggers re-download - test_move_and_chmod_bin: direct test of binary move+chmod - test_move_and_chmod_bin_create_dir: auto-creates install dir - test_create_symlink_with_target: explicit target parameter - test_uninstall_tool_direct/nonexistent: direct uninstall tests - test_uninstall_tool_with_dead_symlink: dead symlink cleanup - test_install_dir_name_default: default directory logic - test_install_clang_tools_path_not_in_env: PATH warning test_main.py additions: - test_main_uninstall: main() with --uninstall flag - test_main_install: main() with --install flag - test_main_install_invalid_version: non-semver version handling - test_main_no_args: no arguments shows help message test_wheel.py additions: - test_get_parser: parser configuration test - test_get_parser_requires_tool: --tool is required Coverage: 87% → 98% (212 statements, 4 remaining uncovered lines)
for more information, see https://pre-commit.ci
Contributor
WalkthroughThis PR adds 405 lines of unit and integration tests across four test modules to cover clang tool installation utilities, utility functions, CLI main entrypoint, and wheel argument parser. No production code is modified. ChangesTest Coverage Expansion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
- Remove unused variable (ruff F841) - Fix to handle Linux default (~/.local/bin/) vs macOS/Windows default (sys.executable dir)
for more information, see https://pre-commit.ci
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #168 +/- ##
==========================================
+ Coverage 95.87% 99.63% +3.76%
==========================================
Files 9 9
Lines 339 554 +215
==========================================
+ Hits 325 552 +227
+ Misses 14 2 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shenxianpeng
changed the title
![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
🧹 Nitpick comments (3)
tests/test_util.py (1)
141-148: ⚡ Quick winConsider mocking the download instead of hitting the network.
This test (like the pre-existing
test_download_file) performs a real download of a multi-MB binary solely to exercise theno_progress_bar=Falsebranch, which adds CI latency and flakiness risk. Mockingurlopenwith a small in-memory response lets you cover the same code path deterministically and offline.♻️ Example using a mocked response
-def test_download_file_with_progress_bar( - monkeypatch: pytest.MonkeyPatch, tmp_path: Path -): - """Tests download_file with progress bar enabled (no_progress_bar=False).""" - monkeypatch.chdir(str(tmp_path)) - url = clang_tools_binary_url("clang-format", "21") - file_name = download_file(url, "file.tar.gz", False) - assert file_name is not None +def test_download_file_with_progress_bar( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +): + """Tests download_file with progress bar enabled (no_progress_bar=False).""" + monkeypatch.chdir(str(tmp_path)) + payload = b"x" * 64 + mock_response = Mock() + mock_response.status = 200 + mock_response.length = len(payload) + mock_response.read.side_effect = lambda n: payload[:n] + monkeypatch.setattr( + "clang_tools.util.urllib.request.urlopen", + Mock(return_value=mock_response), + ) + file_name = download_file("http://fake/file.tar.gz", "file.tar.gz", False) + assert file_name is not NoneNote: the
readstub above is illustrative; adjust it to advance through the buffer so the loop terminates correctly for your assertion.🤖 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 `@tests/test_util.py` around lines 141 - 148, The test test_download_file_with_progress_bar currently performs a real network download; replace that with a deterministic mock of the network call (patch the urlopen used by download_file) so the progress-bar branch is exercised without hitting the network. In practice, monkeypatch the module-level urlopen that download_file calls (or the urllib.request.urlopen used by download_file) to return a small in-memory response object with a working read(size) and getheader/getheaders/headers behaviors so the download loop and progress calculation terminate correctly; keep using clang_tools_binary_url to compute the URL but ignore it in the stubbed urlopen and assert the returned file name as before.tests/test_main.py (1)
102-117: ⚡ Quick winCheck message substrings in
test_main_install_invalid_version/test_main_no_args
main()printsThe version specified is not a semantic specification(so"not a semantic"matches) andNothing to do because \--install` and `--uninstall` was not specified(so"Nothing to do"` matches). Substring assertions still depend on wording, but they align with current output; consider asserting the full message (or a regex) to reduce drift risk.🤖 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 `@tests/test_main.py` around lines 102 - 117, The tests test_main_install_invalid_version and test_main_no_args currently assert substrings which can drift; update their assertions to check the full expected output (or use a regex) from main() so they are robust: in test_main_install_invalid_version replace the "not a semantic" substring assertion with a full string or re.match of "The version specified is not a semantic specification" coming from main(), and in test_main_no_args replace "Nothing to do" with the full message "Nothing to do because `--install` and `--uninstall` was not specified" (or equivalent regex) so the assertions exactly match main()'s printed messages. Ensure you reference main() invocation in both tests (test_main_install_invalid_version, test_main_no_args) and update the capsys assertions accordingly.tests/test_install.py (1)
135-151: 💤 Low valueConsider using platform-specific exe_name construction.
The test always constructs
exe_namewith a version suffix (line 139), but the realis_installedfunction uses platform-specific logic: on Windows, the executable name is{tool_name}{suffix}without the version, while on Linux/macOS it includes-{major_version}.Although the mocks bypass this difference and the test correctly validates version-matching logic, mirroring the production code's naming would improve test accuracy and catch potential platform-specific issues.
♻️ Optional refactor for platform-specific accuracy
- exe_name = f"{tool_name}-{version.info[0]}{suffix}" + # Mirror the platform-specific naming from is_installed + from clang_tools import install_os + exe_name = ( + f"{tool_name}" + + (f"-{version.info[0]}" if install_os != "windows" else "") + + suffix + )🤖 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 `@tests/test_install.py` around lines 135 - 151, The test test_is_installed_found builds exe_name with a version suffix regardless of platform, but is_installed uses platform-specific naming; update the test to mirror that logic: compute major = version.info[0] then if running on Windows (use platform.system() == "Windows" or the same platform check used in is_installed) set exe_name = f"{tool_name}{suffix}" else set exe_name = f"{tool_name}-{major}{suffix}", then create the fake_bin and monkeypatch shutil.which to return it; keep the subprocess.run mock and the assert against is_installed unchanged.
🤖 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 `@tests/test_main.py`:
- Around line 79-99: test_main_install currently performs real network
downloads; instead, monkeypatch the network/download path so the test is
hermetic: keep monkeypatch.chdir(tmp_path), then monkeypatch
clang_tools.util.urllib.request.urlopen (or stub
clang_tools.install.download_file and clang_tools.util.get_sha_checksum) to
return a fake response that writes a dummy binary file in the current working
directory with the expected name and checksum; call main() as before and assert
the installer moved/renamed the dummy file into tmp_path (bin_path exists).
Ensure you reference the functions/classes test_main_install,
clang_tools.util.urllib.request.urlopen, clang_tools.install.download_file,
clang_tools.util.get_sha_checksum, monkeypatch.chdir, and main when implementing
the mocks.
---
Nitpick comments:
In `@tests/test_install.py`:
- Around line 135-151: The test test_is_installed_found builds exe_name with a
version suffix regardless of platform, but is_installed uses platform-specific
naming; update the test to mirror that logic: compute major = version.info[0]
then if running on Windows (use platform.system() == "Windows" or the same
platform check used in is_installed) set exe_name = f"{tool_name}{suffix}" else
set exe_name = f"{tool_name}-{major}{suffix}", then create the fake_bin and
monkeypatch shutil.which to return it; keep the subprocess.run mock and the
assert against is_installed unchanged.
In `@tests/test_main.py`:
- Around line 102-117: The tests test_main_install_invalid_version and
test_main_no_args currently assert substrings which can drift; update their
assertions to check the full expected output (or use a regex) from main() so
they are robust: in test_main_install_invalid_version replace the "not a
semantic" substring assertion with a full string or re.match of "The version
specified is not a semantic specification" coming from main(), and in
test_main_no_args replace "Nothing to do" with the full message "Nothing to do
because `--install` and `--uninstall` was not specified" (or equivalent regex)
so the assertions exactly match main()'s printed messages. Ensure you reference
main() invocation in both tests (test_main_install_invalid_version,
test_main_no_args) and update the capsys assertions accordingly.
In `@tests/test_util.py`:
- Around line 141-148: The test test_download_file_with_progress_bar currently
performs a real network download; replace that with a deterministic mock of the
network call (patch the urlopen used by download_file) so the progress-bar
branch is exercised without hitting the network. In practice, monkeypatch the
module-level urlopen that download_file calls (or the urllib.request.urlopen
used by download_file) to return a small in-memory response object with a
working read(size) and getheader/getheaders/headers behaviors so the download
loop and progress calculation terminate correctly; keep using
clang_tools_binary_url to compute the URL but ignore it in the stubbed urlopen
and assert the returned file name as before.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b312aacc-d076-4b70-83bb-fe2706ea2d2d
📒 Files selected for processing (4)
tests/test_install.pytests/test_main.pytests/test_util.pytests/test_wheel.py
Comment on lines
+79
to
+99
| def test_main_install(monkeypatch: pytest.MonkeyPatch, tmp_path): | ||
| """Test main() with --install flag.""" | ||
| monkeypatch.chdir(tmp_path) | ||
| monkeypatch.setattr( | ||
| sys, | ||
| "argv", | ||
| [ | ||
| "clang-tools", | ||
| "--install", | ||
| "12", | ||
| "--tool", | ||
| "clang-format", | ||
| "--directory", | ||
| str(tmp_path), | ||
| "--no-progress-bar", | ||
| ], | ||
| ) | ||
| main() | ||
| # Binary should be installed | ||
| bin_path = tmp_path / f"clang-format-12{suffix}" | ||
| assert bin_path.exists() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for the downloader used by install_clang_tools and any existing mocks in the test suite.
rg -nP '\b(download_file|urlopen|requests\.|urlretrieve)\b' --type=py -C2
echo "----- existing mocks in tests -----"
rg -nP '\b(monkeypatch|mock|patch)\b' --type=py -g 'tests/**'Repository: cpp-linter/clang-tools-pip
Length of output: 10633
Avoid real network downloads in test_main_install
test_main_installruns the full--installflow without mocking theurllib.request.urlopencalls used byclang_tools.util.download_file/clang_tools.util.get_sha_checksum, so it will download the realclang-format-12artifact on every run; other tests only mockclang_tools.util.urllib.request.urlopenintests/test_util.py.monkeypatch.chdir(tmp_path)isn’t redundant with--directory str(tmp_path): the downloader writes its intermediate file relative to the current working directory, and the installer then moves it into the target directory—so keepchdir(or ensure your mock writes to the cwd location the installer expects).
Mock the download/install path in this test (e.g., monkeypatch clang_tools.util.urllib.request.urlopen or stub clang_tools.install.download_file + checksum handling) and assert the installed binary ends up in tmp_path.
🤖 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 `@tests/test_main.py` around lines 79 - 99, test_main_install currently
performs real network downloads; instead, monkeypatch the network/download path
so the test is hermetic: keep monkeypatch.chdir(tmp_path), then monkeypatch
clang_tools.util.urllib.request.urlopen (or stub
clang_tools.install.download_file and clang_tools.util.get_sha_checksum) to
return a fake response that writes a dummy binary file in the current working
directory with the expected name and checksum; call main() as before and assert
the installer moved/renamed the dummy file into tmp_path (bin_path exists).
Ensure you reference the functions/classes test_main_install,
clang_tools.util.urllib.request.urlopen, clang_tools.install.download_file,
clang_tools.util.get_sha_checksum, monkeypatch.chdir, and main when implementing
the mocks.
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
This PR adds 30 new test cases across all test modules, increasing line coverage from 87% → 98% (212 statements, only 4 hard-to-reach lines remain uncovered).
Changes
tests/test_util.py(+14 tests)no_progress_bar=FalseSystemExitfor unrecognized OStests/test_install.py(+11 tests)targetparameter$PATHtests/test_main.py(+4 tests)--uninstall,--install, invalid version, no-args help messagetests/test_wheel.py(+2 tests)--toolflagCoverage
All 129 tests pass.
Summary by CodeRabbit