fix(bootstrapper): log specific error types in wheel cache lookup#1029
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR narrows a broad Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_bootstrapper.py`:
- Around line 304-318: The test test_resolver_exception_returns_none should also
assert that ResolverException triggers the "did not find wheel" INFO log path:
when calling Bootstrapper._download_wheel_from_cache with
fromager.resolver.resolve patched to raise ResolverException, capture logs
(e.g., using caplog) and assert an INFO-level record contains the phrase "did
not find wheel" (or the exact message emitted by _download_wheel_from_cache) in
addition to asserting the return value is (None, None); update the test to
reference ResolverException and bt._download_wheel_from_cache so the log
assertion focuses on that code path.
- Around line 396-406: The test test_no_cache_url_returns_none does not assert
that the resolver is not invoked; update it to mock or spy on
fromager.resolver.resolve and assert it was not called when
Bootstrapper.cache_wheel_server_url is empty; specifically instantiate
bootstrapper.Bootstrapper, set cache_wheel_server_url = "", call
_download_wheel_from_cache with the Requirement/Version, and then assert
fromager.resolver.resolve (or the injected resolver used by Bootstrapper) had
zero invocations to validate the early-return guard.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c95b0344-a5aa-404c-ab53-0466adcc8d05
📒 Files selected for processing (2)
src/fromager/bootstrapper.pytests/test_bootstrapper.py
2199620 to
3d0214e
Compare
Replace bare `except Exception` in `_download_wheel_from_cache()` with three handlers so logs distinguish why a cache lookup failed: - ResolverException → INFO (wheel not found) - RequestException → WARNING (network error) - Exception → WARNING (unexpected error, e.g. bad wheel file, I/O) All three still return (None, None) to preserve existing behavior. Closes: python-wheel-build#1025 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
3d0214e to
fd5be04
Compare
| tmp_context: WorkContext, | ||
| ) -> bootstrapper.Bootstrapper: | ||
| bt = bootstrapper.Bootstrapper(tmp_context) | ||
| bt.cache_wheel_server_url = "https://cache.example.com/simple" |
There was a problem hiding this comment.
example.com is a real domain. Tests should use a RFC 6761 domain
Replace bare
except Exceptionin_download_wheel_from_cache()with three handlers so logs distinguish why a cache lookup failed:All three still return (None, None) to preserve existing behavior.
Closes: #1025