Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace HTTP client on TestClient from requests to httpx #1376

Merged
merged 25 commits into from Sep 6, 2022

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Dec 17, 2021

Missing:

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

@Kludex Kludex force-pushed the httpx-based-testclient branch 5 times, most recently from 4cb3f1a to 9348fb2 Compare December 17, 2021 11:50
@Kludex Kludex marked this pull request as draft January 7, 2022 19:03
@Kludex Kludex marked this pull request as ready for review January 7, 2022 19:06
@Kludex
Copy link
Sponsor Member Author

Kludex commented Jan 7, 2022

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
Copy link
Sponsor Member Author

Kludex commented Jan 7, 2022

Test failing until encode/httpx#1936 is merged.

@tomchristie
Copy link
Member

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
Copy link
Sponsor Member Author

Kludex commented Jan 10, 2022

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 encode/httpx#1936, or giving me orientation e.g. I can skip the test that involves that issue.

@tomchristie
Copy link
Member

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.

setup.py Outdated
Comment on lines 51 to 52
# "httpx>=0.20.0"
"httpx @ git+https://github.com/encode/httpx.git@master",
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until next httpx release. This is a blocker.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could httpx cut a release? This is going to cause issues for anyone that depends on httpx right? I realize you can probably just not install the "full" extra, but this still breaks things for those that are already doing it / it's a bit unergonomic and confusing.

@Kludex
Copy link
Sponsor Member Author

Kludex commented Jan 23, 2022

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 Kludex requested a review from tiangolo January 23, 2022 16:29
@Kludex
Copy link
Sponsor Member Author

Kludex commented Jan 23, 2022

@tomchristie friendly follow-up ping

@tiangolo
Copy link
Sponsor Member

Awesome! 🚀

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... 😅 .

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. 😬

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. 😅 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. 😅

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. 😎

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. 😬

@Kludex
Copy link
Sponsor Member Author

Kludex commented Jan 23, 2022

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.

@br3ndonland
Copy link
Member

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.

def __enter__(self) -> "TestClient":

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

@tomchristie
Copy link
Member

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.

Copy link
Sponsor Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! I'm all in for this. 🚀


I just tested it locally in FastAPI, here's the PR with the changes needed after this goes in (and I upgrade the Starlette version pinned in FastAPI): tiangolo/fastapi#5350

There are indeed some changes, but I think they are all sensible, and codebases that need to upgrade would have cleaner code after that.

I would think that the main changes I would expect would be:

  • Changing data to content where needed
  • Changing the style of form data from a list of tuples to a dict, which I think is much more cleaner and understandable
  • Changing inline cookies with cookies at client creation

I wouldn't expect many to need to change follow_redirects because I don't think many would even use it.

I also don't expect many to need to send body payloads with GET, DELETE or others, just in corner cases, and they can achieve it as Tom described above (and as done in that FastAPI PR).


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?! 😱 😉


I have a couple of minor comments. But this looks great to me. Awesome job @Kludex! 🙇 🍰 ☕

Also, thank you @tomchristie for having me in mind! 🙇 ☕

README.md Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@Kludex Kludex merged commit 6765502 into encode:master Sep 6, 2022
@Kludex Kludex deleted the httpx-based-testclient branch September 6, 2022 05:43
@michaeloliverx
Copy link
Member

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?! 😱 😉

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

@Kludex
Copy link
Sponsor Member Author

Kludex commented Sep 6, 2022

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
Copy link
Member

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
Copy link
Sponsor Member

tiangolo commented Sep 6, 2022

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? 😱 😉

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

I was just putting the "inception" idea seed. 😅

@Kludex
Copy link
Sponsor Member Author

Kludex commented Sep 7, 2022

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.

  1. may be tricky or even not needed, because people usually have a TestClient fixture.
  2. Not sure if something needs to be done there? 🤔

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

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

@gyKa
Copy link

gyKa commented Nov 14, 2022

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

diegoquintanav added a commit to diegoquintanav/datadis that referenced this pull request Jan 9, 2023
fix/upgrade dependencies

upgrades python version to 3.10.5
upgrades dependencies to their latest versions

this is needed because of pinned httpx version is outdated and TestClient breaks starlette and FastAPI

references

encode/starlette#1376
tiangolo/fastapi#5471
tiangolo/fastapi#5749
vilmibm pushed a commit to internetarchive/fatcat-scholar that referenced this pull request Feb 16, 2024
vilmibm pushed a commit to internetarchive/fatcat-scholar that referenced this pull request Feb 16, 2024
seemingly, this test hits a fallback path for resolving access urls and
that fallback requires an API client; this issue may have started when
starlette (ASGI) framework switched from requests to httpx.

> 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.

-- encode/starlette#1376 (comment)

possible mitigations: upgrade fastapi, as per notes in notes in
tiangolo/fastapi#5350
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up Refinement to existing functionality feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTPX-based TestClient?
7 participants