Swap mock_http_response to MockHTTPResponse#22935
Swap mock_http_response to MockHTTPResponse#22935mwdd146980 merged 8 commits intomwdd146980/httpx-migration-basefrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files🚀 New features to boost your workflow:
|
|
✨ Fix all issues with BitsAI or with Cursor
|
21e6af8 to
c1ac2c5
Compare
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
cb98cdb to
c4fad19
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4fad19d17
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Claude Code Review
Issues
Bug fix in sonatype_nexus test (good, but worth calling out)
The old code had:
json_data=status_metrics_response_data.update({"gauges": ...}),dict.update() returns None, so the mock was sending an empty response body. The PR fixes this correctly by building the dict before passing it. This is a genuine bug fix — worth mentioning in the PR description.
_CaseInsensitiveDict is incomplete
The dict only overrides __setitem__, __getitem__, __contains__, and get(). Missing methods that could silently behave incorrectly:
__delitem__—del headers['Content-Type']won't find the lowercase-stored keypop()— same issueupdate()— won't normalize keys from the incoming dictsetdefault()— won't normalize the key
For test mocking this is likely sufficient today, but if any production code path calls .pop() or del on response headers, tests would silently use the wrong key. Consider either:
- Adding the missing methods, or
- Using
requests.structures.CaseInsensitiveDictorhttpx.Headersdirectly (one will be a dependency anyway), or - At minimum, adding a comment noting the intentional subset
mock_http_response_per_endpoint still patches requests.Session.get
method: str = 'requests.Session.get', # default targetIf the goal is to decouple from requests, this should either be updated here or tracked for a follow-up.
Minor observations
- The
_CaseInsensitiveDict.__setitem__and__getitem__call.lower()unconditionally, which would raiseAttributeErroron non-string keys.__contains__handles this with anisinstancecheck but the others don't. Inconsistent, though HTTP headers are always strings in practice. _CaseInsensitiveDictstores keys lowercased (unlikerequests.structures.CaseInsensitiveDictwhich preserves original case for iteration). Fine for testing but worth a docstring note.- PR description says "~102 test files" but only 13 are in the diff. The central fixture swap covers the rest implicitly — might be worth clarifying to avoid reviewer confusion.
461f963 to
a11d677
Compare
01603f0 to
6e9c663
Compare
a11d677 to
8e65a6f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a934fdb4b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
358ac6f to
2d32227
Compare
| response = MockHTTPResponse(json_data={'key': 'value'}, headers=headers) | ||
|
|
||
| assert response.headers['content-type'] == 'application/json' | ||
| assert response.headers['Content-Type'] == 'application/json' |
There was a problem hiding this comment.
| assert response.headers['Content-Type'] == 'application/json' | |
| assert response.headers['content-type'] == 'application/json' |
I don't see a reason to touch this test
| default_response: MockResponse | None = None, | ||
| method: str = 'requests.Session.get', | ||
| default_response: MockHTTPResponse | None = None, | ||
| method: str = 'requests.Session.get', # TODO(httpx-migration): update default when backend changes |
There was a problem hiding this comment.
In case we can't get rid of this, could we store in a global constant so we would only need to change it in one place?
fd558c5 to
08e03be
Compare
217fc16 to
9f4a6c4
Compare
Co-authored-by: Kyle-Neale <37895372+Kyle-Neale@users.noreply.github.com>
Replace the requests-coupled MockResponse with the library-agnostic MockHTTPResponse in the mock_response fixture, decoupling ~102 test files from the requests library without touching individual test code. - Swap mock_response fixture to yield MockHTTPResponse - Fix mock_http_response_per_endpoint fallback to use fixture parameter - Remove sonatype_nexus local fixture override (now redundant) - Update match strings in 9 integrations from requests.exceptions.HTTPError to HTTPStatusError (our own shared exception class) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Document that MockHTTPResponse has a different parameter order than MockResponse (all callers use keyword args, so not a compatibility concern). Fix test_successful_metrics_collection which was passing vacuously: dict.update() returns None so json_data was always None, the check's json decode failed silently, no metrics were submitted, and assert_all_metrics_covered() trivially passed. Provide complete mock data for both API calls and add individual metric assertions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Step3b will make MockHTTPResponse the backing for all mock_http_response fixture users (~102 test files). Production code accesses headers with original-case keys (e.g. response.headers['Content-Type']), which would KeyError or silently return wrong values with the plain lowered-key dict. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add missing dict methods (__delitem__, pop, update, setdefault) to _CaseInsensitiveDict so tests won't silently break if production code uses them on response headers - Make isinstance guards consistent across all methods - Replace redundant `assert in` checks with mixed-case .get() per reviewer suggestion; add tests for new dict methods - Add TODO(httpx-migration) comment on per-endpoint fixture default Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Make raw.read delegate to BytesIO stream so json.load(r.raw) works (fixes kubelet_base test regression). Add url attribute with default '' (fixes sonatype_nexus test_timeout_error accessing response.url). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…dead args - Fix _CaseInsensitiveDict.update() to lowercase keys from iterable-of-pairs - Add test_mock_response_url() for MockHTTPResponse url parameter - Remove dead URL positional args from 7 sonatype_nexus test calls Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Revert unnecessary Content-Type → content-type case change in test - Extract _DEFAULT_MOCK_METHOD constant for requests.Session.get default Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5e594eb to
889474c
Compare
9f4a6c4 to
457ad34
Compare
8938e5e
into
mwdd146980/httpx-migration-base

Motivation
Part of the httpx migration: decouple the shared
mock_http_responsetest fixture from therequestslibrary so individual integration tests don't need to change when we swap HTTP backends.Approach
mock_http_responsefixture fromMockResponse(requests-backed) toMockHTTPResponse(library-agnostic) in the central pytest plugin — this implicitly covers ~102 test files (228 calls) without touching individual test code_CaseInsensitiveDicttoMockHTTPResponse.headers— previously unnecessary when only used in controlled new tests, now required because production code (e.g.prometheus/mixins.py) accessesresponse.headers['Content-Type']dict.update()returnsNone, sojson_data=data.update({...})was passing an empty response body — now builds the dict before passing itSee plan for more details.
agint-review follow-ups
Addressed nits from the agint-review findings:
_CaseInsensitiveDict.update()to handle iterable-of-pairs (aligns with httpx'sHeadersinterface)test_mock_response_url()forMockHTTPResponse.urlcoverageVerification
ddev test -fs datadog_checks_base— cleanddev test datadog_checks_base— 1280 passedddev test sonatype_nexus— 13 passedReview checklist (to be filled by reviewers)
qa/skip-qalabel if the PR doesn't need to be tested during QA.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged