Replace HTTP client on TestClient from `requests` to `httpx`

[Kludex]

Missing:

• [x] Remove asgiref.
• [x] Use the previous import typing
• [x] Fix skipped tests
• [x] Remove docs references to requests.

[Kludex]

How do I send the headers in httpx the same way as requests?

See https://docs.python-requests.org/en/latest/user/quickstart/#post-a-multipart-encoded-file

[Kludex]

Test failing until https://github.com/encode/httpx/pull/1936/ is merged.

[tomchristie]

Great bit of work, thanks!

We'll want some careful decision making before we hit go on this. Particularly around what FastAPI users will expect.

Something that we could choose to do here would be switch from requests to httpx in the implementation but make sure that we're not introducing API changes. (Eg. continue to support allow_redirects, at the TestClient level, but perhaps soft-deprecate it.) Not sure.

[Kludex]

About the deprecation, anything else besides the item specified in the description of this PR and the allow_redirects that you have in mind? Because I've run the test suite of both FastAPI and Starlette using this source (on another repository), and only what was specified in the description above was a "problem", and also the fact that cookies parameter don't work the same way on the packages involved.

As a reminder, this PR cannot be concluded without taking a decision on https://github.com/encode/httpx/pull/1936, or giving me orientation e.g. I can skip the test that involves that issue.

[tomchristie]

As a reminder, this PR cannot be concluded without taking a decision on encode/httpx#1936, or giving me orientation e.g. I can skip the test that involves that issue.

Indeed, so for now let's hold on that.

About the deprecation, anything else besides the item specified in the description of this PR and the allow_redirects that you have in mind? Because I've run the test suite of both FastAPI and Starlette using this source (on another repository), and only what was specified in the description above was a "problem", and also the fact that cookies parameter don't work the same way on the packages involved.

Well essentially it'd be interesting to consider two options...

• This PR as-is.
• This PR, but also with backwards compatibility, and without changes to the test suite.

A useful thing to consider here is that any changes to the footprint of our test suite here is a good indicator of what changes our users would also need to make to their test suites. If we're hitting them with a big set of required changes in order to upgrade, then that's not fantastic. (Although there might or might not be good enough reasons why we're prefer to do that.)

I'm mostly saying all this in order to flag up that I think we'll want @tiangolo's perspective, and that we will need to carefully look at the cost of API changes, and how we can mitigate that.

[Kludex]

As a reminder, this PR cannot be concluded without taking a decision on encode/httpx#1936, or giving me orientation e.g. I can skip the test that involves that issue.

Indeed, so for now let's hold on that.

About the deprecation, anything else besides the item specified in the description of this PR and the allow_redirects that you have in mind? Because I've run the test suite of both FastAPI and Starlette using this source (on another repository), and only what was specified in the description above was a "problem", and also the fact that cookies parameter don't work the same way on the packages involved.

Well essentially it'd be interesting to consider two options...

• This PR as-is.
• This PR, but also with backwards compatibility, and without changes to the test suite.

A useful thing to consider here is that any changes to the footprint of our test suite here is a good indicator of what changes our users would also need to make to their test suites. If we're hitting them with a big set of required changes in order to upgrade, then that's not fantastic. (Although there might or might not be good enough reasons why we're prefer to do that.)

I'm mostly saying all this in order to flag up that I think we'll want @tiangolo's perspective, and that we will need to carefully look at the cost of API changes, and how we can mitigate that.

• User perspective: we're only breaking their test suite, not their application (assuming they don't use testclient module on their application).
• Starlett's perspective: we're breaking our API, as testclient.TestClient is public.

We usually follow a "try to not add breaking changes" approach, even if we are on "zero version". Considering this, we should make it backwards compatible.

That being said... I'm going to list the set of breaking changes, so it can help us to make the best decision here:

• The methods ("delete", "get", "head", "options") doesn't accept the content, data, json and files parameters.
• allow_redirects is now follow_redirects.
• cookies parameter now is deprecated under the method calls (it should be used on the client instantiation).
• content_type send defaults to "text/plain" when sending file instead of empty string.
• data is now content.

[Kludex]

@tomchristie friendly follow-up ping

[tiangolo]

Awesome! :rocket:

Thanks a lot @Kludex for the PR and for the short summary of breaking changes, that's super helpful.

Indeed, breaking changes are not great in general... :sweat_smile: .

Nevertheless, the intention has been to move from Requests to HTTPX from the beginning, I would like to do as much as possible to support this.

Some comments about the changes:

allow_redirects ✅

I don't expect a lot of people to have a lot of allow_redirects=False, so I wouldn't expect that migration to be too painful for users.

Body in GET and others ⁉

Starlette and FastAPI actually allow receiving content from GET requests, even if that's not auto documented in OpenAPI, and it's weird in general, it's still supported. I guess mainly because ElasticSearch does it, and to keep compatibility with it. There's a small mention about it in the info box here: https://fastapi.tiangolo.com/tutorial/body/

If the test client doesn't support it, it would mean that users can't test their weird apps. Which are weird, but they should be able to test them.

No Cookies ⁉

I think not being able to pass directly cookie values could be cumbersome, as I would expect some tests to send a cookie value directly from tests, to test authentication flows, independent from setting and retrieving the value. I've seen that and I think I've used that myself. :grimacing:

Default content_type ❓

I'm not sure if this could break anything in FastAPI, but I think the behavior should probably be whatever is the same default behavior from browsers. Unless there's some specification that says that everything should be by default text/plain.

But I would go with whatever browsers do. And if it breaks FastAPI, I should fix it there in FastAPI. :sweat_smile: Because I would expect that the great majority of real-life/production clients would be browsers.

Data to Content ❓

This is one of those breaking changes that I guess could be a bit cumbersome but might be worth just going with it... like ripping a band-aid. :sweat_smile:

Just because this is a design decision in HTTPX, it's quite visible, and we'll want to educate people to use it, instead of having both.

On top, content is a much better name for what it is than data. :sunglasses:

Maybe, if it's not too complex, we could have both with a deprecation warning for data for a while, what do you think?

---

Off topic note: Thanks for pinging me @tomchristie and @Kludex!

And thanks @Kludex for pinging me on another channel! I'm swamped in GitHub notifications and it's hard to get to the important stuff like this. :grimacing:

[Kludex]

Thanks for your input @tiangolo ! :)

I'll wait for @tomchristie 's feedback, so we can decide how to continue here in order to have this PR merged.

[tomchristie]

First thoughts here...

How difficult would it be for us to add compat for the places where the API is changed?

It'd be fantastic if we were able to get this in, without requiring users to update their test suites. We'll be causing an awful lot of work otherwise.

• Is it feasible for us to add compat for data=..., allow_redirects=..., .reason/.reason_phrase for now?
• The content type change I don't think I've enough info on. Is there a PR in httpx or something that adds that? Where's the behaviour coming from?
• We do support body in GET requests, it's just that we force users to use client.request("GET", ..) and make it deliberately awkward, so that it's less easy for folks to go down that route. I think that's fine as a change so long as it's called out.
• The cookies thing is a bit fiddly perhaps. Not sure. Just going to park that right now.

[Kludex]

Is it feasible for us to add compat for data=..., allow_redirects=..., .reason/.reason_phrase for now?

I can do that. I'm not sure what to do with the test suite, tho. I can add those back, and give a DeprecationWarning. Should I make the test suite match the non-deprecate parameters or the deprecated ones?

We do support body in GET requests, it's just that we force users to use client.request("GET", ..) and make it deliberately awkward, so that it's less easy for folks to go down that route. I think that's fine as a change so long as it's called out.

Sorry, I don't understand the end. What is fine? Should I make it compatible?

How difficult would it be for us to add compat for the places where the API is changed?

Easy. :+1:

[tomchristie]

I can do that. I'm not sure what to do with the test suite, tho. I can add those back, and give a DeprecationWarning. Should I make the test suite match the non-deprecate parameters or the deprecated ones?

I think a good plan is probably for this PR to aim to change our test suite as little as possible. (So, eg. leave them as data=...) If we do that then we know we're also not going to be causing pain for other users. We can introduce a later follow-up PR that does switch them over.

Sorry, I don't understand the end. What is fine? Should I make it compatible?

I don't think we need to make "body for GET requests" compatible, no. It's a good change to enforce it being with .request(). I'm probably okay with us forcing that one.

[Kludex]

Added allow_redirects compat on the last commit (I don't know what happened with the commit message). JFYK

[br3ndonland]

Thanks for taking this on @Kludex. I know there have been a lot of "requests" to remove Requests :zany_face:.

Are there any concerns about client state? I'm just thinking re: encode/httpx#1800 and HTTPX 0.19.

[Kludex]

Thanks for taking this on @Kludex. I know there have been a lot of "requests" to remove Requests 🤪.

Are there any concerns about client state? I'm just thinking re: encode/httpx#1800 and HTTPX 0.19.

I didn't get the question 🤔

[br3ndonland]

I didn't get the question 🤔

I asked because the Starlette TestClient is overriding the __enter__ method on the HTTPX Client, but not enforcing opened/closed state in the same way.

https://github.com/encode/starlette/blob/3050e9e3517eeb6bb061cbd7f2a58f61cd5edab8/starlette/testclient.py#L711

I don't have a specific problem with it in this context, just wanted to raise the question for discussion.

[tomchristie]

So... the largest impact of this change will be for FastAPI devs. They're the largest install base, they're using the test client, and the change will introduce breaking changes into their test suites and require extra work for their teams.

I guess we might(?) want to have a couple of shims in place in order to minimise the API changes...

• We could have a shim to continue allowing data=....
• We could have a shim to allow content in GET etc... requests.
• The content type change is probably absolutely fine as it is.
• I'm not sure we can easily shim the cookie change.

We could also consider assessing how much negative impact this PR would have by making a pull request to the FastAPI project that uses this branch for the TestClient, and determining how much work is required to change the test suite there.

Anyways, I don't know really - not super-keen on giving teams extra busywork, but yes it would be nicer to have switched to httpx. Perhaps @tiangolo ought to have the final say on if/when we're okay with merging this.

[michaeloliverx]

Nice job @Kludex

Also, for almost all of the points, I think except for the cookies in the client, the needed refactor could be automated with a LibCST codemod... but who would have the expertise to end up building that?! :scream: :wink:

This seems like a useful route to take and is quite common in the JavaScript ecosystem.

[Kludex]

Nice job @Kludex

Thanks!

Also, for almost all of the points, I think except for the cookies in the client, the needed refactor could be automated with a LibCST codemod... but who would have the expertise to end up building that?! 😱 😉

This seems like a useful route to take and is quite common in the JavaScript ecosystem.

If no one does it before me, I'll do it before the next release. 👍

[michaeloliverx]

Nice job @Kludex

Thanks!

Also, for almost all of the points, I think except for the cookies in the client, the needed refactor could be automated with a LibCST codemod... but who would have the expertise to end up building that?! 😱 😉

This seems like a useful route to take and is quite common in the JavaScript ecosystem.

If no one does it before me, I'll do it before the next release. 👍

I'd be interested in a blog post on AST transforms 😉

[tiangolo]

Also, for almost all of the points, I think except for the cookies in the client, the needed refactor could be automated with a LibCST codemod... but who would have the expertise to end up building that? :scream: :wink:

Hehe, I was actually joking with the "who would have the expertise", I knew @Kludex was the - right - person. :sunglasses:

I was just putting the "inception" idea seed. :sweat_smile:

[Kludex]

From the changes:

1. The methods ("delete", "get", "head", "options") doesn't accept the content, data, json and files parameters.
2. allow_redirects is now follow_redirects.
3. cookies parameter now is deprecated under the method calls (it should be used on the client instantiation).
4. content_type send defaults to "text/plain" when sending file instead of empty string.
5. data is now content.

I've implemented 1, 2, and 5. I need to add more tests, and fix a scenario, but in progress.

3. may be tricky or even not needed, because people usually have a TestClient fixture.
4. Not sure if something needs to be done there? :thinking:

Code in https://github.com/Kludex/bump-testclient.

EDIT: I thought of a way to do 3. :thinking:

[gyKa]

Are there any technical reasons why starlette is starting to use httpx instead of requests?