TestClient.request does not honor timeout

[falkben]

Checklist

• The bug is reproducible against the latest release and/or master.
• There are no similar issues or pull requests to fix it yet.

Describe the bug

timeout parameter exposed in testclient.request has no effect. Looking through the code, it seems as though we use the same interface as requests package but only a subset of those features are supported by testclient

To reproduce

import time

from starlette.applications import Starlette
from starlette.responses import JSONResponse
from starlette.testclient import TestClient

mock_service = Starlette()


@mock_service.route("/slow_response")
def slow_response(request):
    time.sleep(0.01)
    return JSONResponse({"mock": "slow example"})


def test_timeout():
    """ no timeout exception raised -- this test passes when it should fail """
    client = TestClient(mock_service, raise_server_exceptions=True)
    response = client.get("/slow_response", timeout=0.001)
    assert response.json() == {"mock": "slow example"}

Expected behavior

A TimeoutException should be raised if the timeout specified in request is shorter than the time the request takes.

Actual behavior

No timeout exceptions are raised when setting timeout.

Additional context

In CI/CD context, testing our own endpoints which, during integration testing, may timeout, is very helpful. If this is unsupported, it might be nice to have a list of differences between the interface of requests/testclient (this might include #1102).

I did look a bit how the requests package implemented their timeout exceptions, and to be completely honest I got a bit lost going down the rabbit hole of requests -> urllib3 -> http.client. Since we're using a different adapter for the testclient, it might be some work to reimplement all the way down the timeout exceptions in the same way.

I did notice that the async_asgi_testclient works with a timeout. Of course, it requires an async endpoint. But their implementation might still be useful?

Found that httpx had a similar issue with timeout having no effect when mounting an app with its client.

[tomchristie]

Closing as per #1109

timeouts == flaky tests

[tomchristie]

Okay - that comment is a bit brief. But see #1109. Frameworks such as Flask and Django have always had this constraint. It's something that we can live with as well.

[logi]

I have a case where a "timeout" would be useful in testing how the server handles disconnecting clients. So a "timeout" where the client times out immediately after sending the request would be useful and should not be flaky since no actual timing is involved.

[Kludex]

I have a case where a "timeout" would be useful in testing how the server handles disconnecting clients. So a "timeout" where the client times out immediately after sending the request would be useful and should not be flaky since no actual timing is involved.

This may help: 25a52fe

[falkben]

I was just looking at this again last week. I never could figure out how to get around the BlockingPortal. We run our tests in a synchronous context (not async as you linked to above) but that example @Kludex looks quite helpful.

Anyways, I sort of came to the conclusion that we should remove our individual test timeouts from our service unit tests as they were a sort of anti-pattern. Many of our service unit tests also serve as integration tests, but we probably need to separate those out better.