add basic timeout support for testclient

[falkben]

Closes #1108

Note, this does not implement the separate connection and read timeouts that requests supports (will raise a ValueError if user supplies a tuple. Besides the added complexity of dealing with that, it also didn't make sense to me that there would be a connection timeout when testing a local mounted app.

Not sure if something is needed in the docs about timeout, but maybe I could if desired? https://github.com/encode/starlette/blob/ben%2Ftestclient_timeout/docs/testclient.md

[falkben]

Wondering if there's anything more to do for this? Thanks.

[JayH5]

Hey 👋 this seems like a reasonable change, but I have a couple of questions/points:

1. Can you explain more about why you want this functionality? I see in TestClient.request does not honor timeout #1108 you mention timing out in CI/CD. Can you explain what you're trying to solve exactly? Is it to prevent tests from hanging?
2. I think the behaviour should be that a timeout error is always raised if one occurs, regardless of the value of the raise_server_exceptions flag. The flag is for server errors, and this timeout is a client error.

[falkben]

Hey 👋 this seems like a reasonable change, but I have a couple of questions/points:

1. Can you explain more about why you want this functionality? I see in #1108 you mention timing out in CI/CD. Can you explain what you're trying to solve exactly? Is it to prevent tests from hanging?

2. I think the behaviour should be that a timeout error is _always_ raised if one occurs, regardless of the value of the `raise_server_exceptions` flag. The flag is for _server_ errors, and this timeout is a _client_ error.

1. Yes, to prevent tests from hanging. Better to fail them if they take too long.
2. I see your point, but I think the line between client/server here is not so clear cut. The client isn't timing out (since the app is mounted directly into the client, it cannot time out). It's the server that's timing out. But, perhaps it's something that should be cleaned up somehow, since it is a bit confusing.

[Kludex]

Hi @falkben! 👋

Would you mind rebasing it?

[falkben]

I started this process in a diff branch but unclear where to put something like fail_after from anyio. I get an error message about not being in an async context where I have the fail_after currently, which I thought I would be inside the portal_factory context. Do you have any suggestions?

https://github.com/encode/starlette/compare/master...falkben:ben/testclient_timeout_rebase?expand=1

My approval was given before being part of encode. The solution is not anyio compliant, so we need to check how to proceed here.

[tomchristie]

I'm not convinced we necessarily want timeout behaviour in the test client, but either way around it's probably not great timing given #1376 - might make sense to consider this only after that's in?

[Kludex]

I'm more inclined towards #1446, which is exclusive considering this one (as the parameter name is the same).

We can consider this PR with another parameter, after #1376, tho.

[tomchristie]

I'm going to put on my maintainer-pushback hat here and say "the TestClient doesn't support timeouts".
At least for now, and possibly for the long term.

Existing WSGI frameworks haven't generally supported this, and needing them almost certainly points to flaky tests.

I'd be up for docs pull requests around this if anyone's motivated to do so.