Repository navigation
test(ingestion): cover URL fallback in get_first_response - #5148
benhammondmusic merged 2 commits into
Conversation
get_first_response is documented to retry the next URL when one fails: for an HTTP error, and, with validate_json=True, for a non-JSON body. Neither path was exercised. MockResponse.raise_for_status was a no-op, so the requests.HTTPError branch never ran, and every test passed a single-URL list, so the loop never advanced past the first entry. Give MockResponse an optional status_code (default 200) so raise_for_status can raise a real requests.HTTPError, and add two cases that pass two URLs: a 503 on the first, and a non-JSON body on the first under validate_json. Both assert that the second response is returned and that requests.get was called twice. Tests only; no production change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe test suite extends Changesget_first_response retry and fallback tests
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR adds focused coverage for URL fallback behavior without changing production code; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
benhammondmusic
left a comment
benhammondmusic
left a comment
There was a problem hiding this comment.
Awesome; thanks!
…erage # Conflicts: # python/tests/ingestion/test_url_file_to_gcs.py
|
Thanks for merging main and updating the assertions — One optional nit, not worth blocking on. Both fallback tests feed responses through by_url = {"https://badurl.com": error_response, "https://goodurl.com": ok_response}
with patch("requests.get", side_effect=lambda url, **kw: by_url[url]) as mock_get, ...
self.assertEqual(
[c.args[0] for c in mock_get.call_args_list],
["https://badurl.com"] * 3 + ["https://goodurl.com"],
)Verified locally: with that form, removing the fallback fails exactly the two fallback tests, and removing the same-URL retry also fails the HTTP fallback test and the two added in #5153. No need to hold this PR; I can send it as a follow-up if useful. |
Description and Motivation
Follow-up to #5142, at @benhammondmusic's request: port the missing URL-fallback coverage onto the merged #5143 code.
get_first_responsepromises to retry the next URL when one fails — for an HTTP error, and, withvalidate_json=True, for a non-JSON body. Neither promise was exercised:MockResponse.raise_for_status()was a no-op, so therequests.HTTPErrorbranch never ran.Tests only; no production change. (
get_first_responsealready catchesValueErrorrather than a bareException, so there was nothing to tighten there.)MockResponsean optionalstatus_code(default200) soraise_for_status()can raise a realrequests.HTTPError.testGetFirstResponse_HttpError_FallsBackToNextUrl— 503 on the first URL, valid response on the second.testGetFirstResponse_JsonValidation_FallsBackToNextUrl— HTML body on the first URL, valid JSON on the second, undervalidate_json=True.Both assert that the second response is returned and that
requests.getwas called twice.Has this been tested? How?
Against
mainat1761bfcd:pytest python/tests/ingestion/test_url_file_to_gcs.py— 10 passed (8 pre-existing + 2 new).black --check(line-length 120) andpylint --rcfile=pyproject.toml— both clean.Mutation-checked so the new cases are load-bearing rather than decorative:
get_first_responseexcept requests.HTTPErrortestGetFirstResponse_HttpError_FallsBackToNextUrlfailsexcept ValueErrortestGetFirstResponse_JsonValidation_FallsBackToNextUrlfails, alongside the two pre-existing JSON testsThe second row is the point of the new JSON case: the pre-existing tests only prove a single-URL list returns
None, not that the loop continues to the next URL.status_codedefaults to200, soraise_for_status()stays a no-op for all eight pre-existing tests.Types of Changes