fix: print stdout and stderr when there is error #142
Conversation
2bndy5 force-pushed the fix-141 branch 3 times, most recently from c04607c to edf3d32 Compare 
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## main #142 +/- ## ========================================== + Coverage 94.44% 95.41% +0.96% ========================================== Files 4 4 Lines 108 109 +1 ========================================== + Hits 102 104 +2 + Misses 6 5 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| I had to get rid of the Instead, I just checked the exit code and return |
CodSpeed Performance ReportMerging #142 will degrade performances by 43.16%Comparing Summary
Benchmarks breakdown
Footnotes
|
| The performance will decrease because this wasn't capturing the stdout or stderr streams previously. |
| make sense |
WalkthroughSelects Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ 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 |
![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
🧹 Nitpick comments (2)
tests/test_util.py (2)
146-156: Add assertions to verify error logging.The test patches
LOGbut doesn't verify thatLOG.errorwas called with the decoded stdout/stderr. Since logging the output on failure is a key objective of this PR, the test should validate this behavior.Apply this diff:
with ( patch("subprocess.run", side_effect=patched_run), - patch("cpp_linter_hooks.util.LOG"), + patch("cpp_linter_hooks.util.LOG") as mock_log, ): result = _install_tool("clang-format", "20.1.7") assert result is None + + # Verify error logging + assert mock_log.error.call_count == 3 + mock_log.error.assert_any_call("pip failed to install %s %s", "clang-format", "20.1.7") + mock_log.error.assert_any_call("Installation failed") + mock_log.error.assert_any_call("Error")
162-164: Make the subprocess.run mock explicit.The test patches
subprocess.runwithout specifying a return value, which implicitly returns aMagicMock. For clarity and maintainability, explicitly return aCompletedProcesswithreturncode=0.Apply this diff:
- with patch("subprocess.run"), patch("shutil.which", return_value=None): + def patched_run(*args, **kwargs): + return subprocess.CompletedProcess(args, returncode=0) + + with ( + patch("subprocess.run", side_effect=patched_run), + patch("shutil.which", return_value=None), + ): result = _install_tool("clang-format", "20.1.7") assert result is None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp_linter_hooks/util.py(2 hunks)tests/test_util.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_util.py (1)
cpp_linter_hooks/util.py (1)
_install_tool(62-74)
🪛 Ruff (0.14.3)
cpp_linter_hooks/util.py
64-64: subprocess call: check for execution of untrusted input
(S603)
tests/test_util.py
126-126: Unused function argument: kwargs
(ARG001)
146-146: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (2)
cpp_linter_hooks/util.py (1)
8-11: LGTM: Clean version-guarded import.The version-guarded import for tomllib/tomli is well-structured and follows best practices for supporting both Python 3.11+ (stdlib tomllib) and earlier versions (tomli package).
tests/test_util.py (1)
126-139: LGTM: Test correctly updated for subprocess.run.The test properly mocks
subprocess.runwith aCompletedProcessand verifies thatcapture_output=Trueis passed, aligning with the implementation changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_util.py (1)
126-127: Optional: Remove unusedkwargsparameter.The
kwargsparameter is unused and flagged by Ruff (ARG001). Consider using*argsonly if no keyword arguments are expected, or remove both if neither is used.Apply this diff:
- def patched_run(*args, **kwargs): + def patched_run(*args): return subprocess.CompletedProcess(args, returncode=0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp_linter_hooks/util.py(2 hunks)tests/test_util.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_util.py (1)
cpp_linter_hooks/util.py (1)
_install_tool(62-75)
🪛 Ruff (0.14.3)
tests/test_util.py
126-126: Unused function argument: kwargs
(ARG001)
147-147: Unused function argument: kwargs
(ARG001)
cpp_linter_hooks/util.py
64-64: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (2)
cpp_linter_hooks/util.py (2)
8-11: LGTM! Clean version-specific import.Using
sys.version_infofor the conditional import is more explicit and idiomatic than try/except.
62-75: LGTM! Implementation correctly captures and logs output on failure.The use of
text=Trueautomatically handles encoding, eliminatingUnicodeDecodeErrorrisks. The error logging on non-zero return codes addresses the original issue effectively.Note: The static analysis S603 warning about untrusted subprocess input is a false positive—
sys.executableis a trusted system path.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_util.py (1)
126-140: LGTM! Test correctly updated for subprocess.run.The test properly mocks
subprocess.runto return aCompletedProcesswithreturncode=0and verifies the call includescapture_output=Trueandtext=True, matching the actual implementation.The unused
kwargsparameter (flagged by Ruff ARG001) could optionally be removed or prefixed with_to suppress the warning, but accepting**kwargsis a common pattern in test mocks for flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp_linter_hooks/util.py(2 hunks)tests/test_util.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_util.py (1)
cpp_linter_hooks/util.py (1)
_install_tool(62-75)
🪛 Ruff (0.14.3)
tests/test_util.py
126-126: Unused function argument: kwargs
(ARG001)
147-147: Unused function argument: kwargs
(ARG001)
164-164: Unused function argument: kwargs
(ARG001)
cpp_linter_hooks/util.py
64-64: subprocess call: check for execution of untrusted input
(S603)
🔇 Additional comments (4)
cpp_linter_hooks/util.py (2)
8-11: LGTM! Proper version-gated import.The version check correctly uses
tomllibfrom the standard library on Python 3.11+ and falls back to thetomlipackage for older versions. This is the idiomatic approach for handling this transition.
62-75: LGTM! Implementation correctly captures and logs pip output.The refactored implementation properly addresses the PR objectives:
- Uses
subprocess.runwithcapture_output=Trueto capture stdout/stderr- Uses
text=Trueto automatically handle string decoding (avoiding encoding issues)- Logs both stdout and stderr when pip fails (returncode != 0)
- Returns
Noneon failure or the tool path on successThe static analysis hint (S603) about untrusted input is a false positive here—
toolandversionare controlled parameters resolved from the codebase's version lists and pyproject.toml, not arbitrary user input.tests/test_util.py (2)
147-150: LGTM! Test correctly simulates pip failure with string output.The mock now correctly returns
stdoutandstderras strings (not bytes), which matches the behavior ofsubprocess.runwithtext=True. This addresses the previous review comment about data type mismatch.
164-172: LGTM! Test now correctly simulates successful pip install.The mock properly returns a
CompletedProcesswithreturncode=0, ensuring the code follows the success path whereresult.returncode == 0evaluates toTrue. This addresses the previous review comment where the mock wasn't simulating success correctly. Theshutil.whichreturningNoneproperly tests the edge case where installation succeeds but the tool isn't found in PATH.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cpp_linter_hooks/util.py (1)
62-74: Consider logging when the tool isn't found in PATH despite successful installation.The implementation correctly captures and logs pip output on failure. However, if pip succeeds but
shutil.which(tool)returnsNone(Line 70), this edge case goes unlogged. This could happen if pip's install location isn't in PATH or the executable name differs from the package name.Apply this diff to add diagnostic logging:
if result.returncode == 0: - return shutil.which(tool) + tool_path = shutil.which(tool) + if tool_path is None: + LOG.warning("pip install succeeded but %s not found in PATH", tool) + return tool_path LOG.error("pip failed to install %s %s", tool, version)
Static analysis warning S603 is a false positive.
Ruff flagged the subprocess call as potentially executing untrusted input. However,
toolis constrained to"clang-format"or"clang-tidy"inresolve_install(Line 77-90), andversionis validated againstCLANG_FORMAT_VERSIONS/CLANG_TIDY_VERSIONSor defaults through_resolve_version. The inputs are effectively whitelisted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp_linter_hooks/util.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.3)
cpp_linter_hooks/util.py
64-64: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test (3.10)
- GitHub Check: test (3.9)
- GitHub Check: test (3.14)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: Run benchmarks
🔇 Additional comments (1)
cpp_linter_hooks/util.py (1)
8-11: LGTM! Cleaner version-based import.Using
sys.version_info >= (3, 11)is the idiomatic approach for version-specific imports and is more explicit than try/except for this use case.
Co-authored-by: Brendan <2bndy5@gmail.com>
adjust tests for `subprocess.run()`
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cpp_linter_hooks/util.py (1)
8-11: Unrelated but correct refactor.This change from try/except to an explicit version check is functionally equivalent and slightly clearer. However, it's unrelated to the PR's stated objective (fixing pip output logging on errors).
tests/test_util.py (1)
126-127: Silence the linter warnings for unusedkwargs.The
**kwargsparameter inpatched_runis necessary becausesubprocess.runis invoked with keyword arguments (capture_output=True,text=True). Although the parameter is not used within the function body, removing it would cause the mock to fail when called.To silence Ruff's ARG001 warnings, prefix the parameter with an underscore:
-def patched_run(*args, **kwargs): +def patched_run(*args, **_kwargs):Apply this change to all three
patched_rundefinitions (lines 126, 147, and 164).Also applies to: 147-150, 164-165
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp_linter_hooks/util.py(2 hunks)tests/test_util.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_util.py (1)
cpp_linter_hooks/util.py (1)
_install_tool(62-74)
🪛 Ruff (0.14.3)
tests/test_util.py
126-126: Unused function argument: kwargs
(ARG001)
147-147: Unused function argument: kwargs
(ARG001)
164-164: Unused function argument: kwargs
(ARG001)
cpp_linter_hooks/util.py
64-64: subprocess call: check for execution of untrusted input
(S603)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run benchmarks
🔇 Additional comments (2)
cpp_linter_hooks/util.py (1)
62-74: Implementation correctly addresses the PR objective.The function now captures stdout and stderr using
subprocess.runwithcapture_output=Trueandtext=True, logging both streams on failure. This successfully resolves issue #141.Regarding the static analysis hint (S603): the warning about untrusted input is a false positive. The command executes
sys.executable(the current Python interpreter) with internally validated arguments—toolandversionare resolved and validated by_resolve_versionbefore reaching this function.tests/test_util.py (1)
126-172: Test coverage appropriately validates the new behavior.The three test cases correctly cover success, failure, and edge-case scenarios (installation succeeds but tool not found in PATH). The mocks properly simulate
subprocess.runwithcapture_output=Trueandtext=True, and stdout/stderr are correctly provided as strings.
2bndy5 left a comment
2bndy5 left a comment There was a problem hiding this comment.
I have nothing left to add here
closes #141
Summary by CodeRabbit
Refactor
Tests