fix(bootstrapper): log specific error types in wheel cache lookup#1029
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 33 seconds. ⌛ 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR replaces 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. 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 |
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
3d0214e to
fd5be04
Compare
|
This pull request has merge conflicts that must be resolved before it can be merged. |
fd5be04 to
e6784f8
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>
e6784f8 to
960587a
Compare
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