Closing AsyncClient in all tests (#871)

[cdeler]

As we discussed in #1197 (comment) , it was decided to move the test refactoring out from #1197

In this PR I added closing to all unclosed AsyncClient in the tests

[lovelydinosaur]

Great!

At the moment you're using async with in some cases here, and .aclose() in others.

I'm wondering if we should instead just always be using async with?
Maybe it doesn't really mater either way? What do other @encode/maintainers think?

Might be a good signal to users digging through our test cases, about the proper usage?

[cdeler]

@tomchristie
Some tests, for example

httpx/tests/client/test_auth.py

Lines 313 to 326 in cfda19e

	@pytest.mark.asyncio
	asyncdeftest_auth_property() ->None:
	client=AsyncClient(transport=AsyncMockTransport())
	assertclient.authisNone
	client.auth= ("tomchristie", "password123") # type: ignore
	assertisinstance(client.auth, BasicAuth)
	url="https://example.org/"
	response=awaitclient.get(url)
	assertresponse.status_code==200
	assertresponse.json() == {"auth": "Basic dG9tY2hyaXN0aWU6cGFzc3dvcmQxMjM="}
	awaitclient.aclose()

are long. In such cases I was adding await client.aclose() to prevent adding new indentation level (but I can change all test to make them use identical AsyncClient closing method, e.g. async with)

PS moveover, these tests are not about closing. They might be provided with AsyncClient using fixture, but it would make test suite more complicated

[florimondmanca]

I'm wondering if we should instead just always be using async with?

In such cases I was adding await client.aclose() to prevent adding new indentation level (but I can change all test to make them use identical AsyncClient closing method, e.g. async with)

Yeah, imo, we should probably stick to async with everywhere, even if it increases the diff size here a bit (we can ease review by enabling "ignore whitespace" in the GitHub review UI). At least we shouldn't be using .aclose() without a try/finally so we don't hint at wrong usage, and so by that point we need extra indentation anyway. :-)

[lovelydinosaur]

Yup, fair call. I've also noticed that there seem to be a bunch of places in the test cases where we're using AsyncClient, but that I think we should probably just be using Client instead.

For example https://github.com/encode/httpx/blob/master/tests/client/test_headers.py where it looks to me like it'd be a whole bunch neater if it was just testing against a plain client.

Wondering if we should do the following in order?

• Tackle any cleanup in test cases, and always prefer just using httpx.Client unless there's a good reason.
• Make sure we're using context management in any remaining places where we are still using httpx.AsyncClient.
• Update Issue warning on unclosed AsyncClient. #1197 against the master branch at that point.

(Also @cdeler thanks for being so patient, there's been a whole bunch of extra stuff we've had to work through in order to approach this as carefully and throughly as this.)

[lovelydinosaur]

See #1222.

[cdeler]

I'm to work on this ticket in 2 days

[lovelydinosaur]

Sure thing - don't put yourself under pressure, or feel that you need to let us know when you will or won't be working on stuff. Thanks again for all your work on this. 😄

[cdeler]

@tomchristie

I did what you asked:

1. for now all AsyncClient usage is gone by async with block (except thetest_async_proxy_close, where it's done in finally block)
2. I changed all AsyncClient references to httpx.AsyncClient

[lovelydinosaur]

Great! It looks like there's still some work we could be doing on preferring the use of httpx.Client over httpx.AsyncClient wherever possible in the tests, but we can probably just treat that separately to the rest of this now.

I'm happy enough for this to go in whenever. Good work. 👍