Turn scope["client"] to None on TestClient#2377
Conversation
3dca547 to
55c8602
Compare
| def test_client(test_client_factory): | ||
| async def app(scope, receive, send): | ||
| client = scope.get("client") | ||
| assert client is not None | ||
| host, port = client | ||
| response = JSONResponse({"host": host, "port": port}) | ||
| await response(scope, receive, send) | ||
|
|
||
| client = test_client_factory(app) | ||
| response = client.get("/") | ||
| assert response.json() == {"host": "testclient", "port": 50000} |
There was a problem hiding this comment.
I didn't see this test before... Can we have a background of why it exists?
There was a problem hiding this comment.
It was added here: #1462
I think it was not intentional, there’s another test in test_requests.py which is fine, but not needed for testclient.
5b4d284 to
f44203c
Compare
|
Ok. I actually don't have any opinion here. If some other member approves it, I'm fine. 👍 |
|
Do you mind if are explicit on the |
scope["client"] to None on TestClient
Since PR Kludex/starlette#2377, we can no longer expect scope["client"] to return a tuple.
Since PR Kludex/starlette#2377, we can no longer expect scope["client"] to return a tuple.
Seems that client is optional according to the ASGI spec. https://asgi.readthedocs.io/en/latest/specs/www.html With Starlette 0.35 the TestClient connection scope is None for "client". Kludex/starlette#2377 Signed-off-by: moson <moson@archlinux.org>
This was removed in Kludex/starlette#2377
The 'client' scope was removed in Kludex/starlette#2377, which means we can't read the client IP from it in unit tests
The 'client' scope was removed in Kludex/starlette#2377, which means we can't read the client IP from it in unit tests
The 'client' scope was removed in Kludex/starlette#2377, which means we can't read the client IP from it in unit tests
|
Can I override |
Faced the same problem |
|
This broke my tests, noticed when upgrading fastapi to address a security advisory :( |
|
I'm facing the same problem w/ broken tests. These fail because I am accessing It appears that, for the actual application, All that said, while initially this seems like a bug in Starlette, the ASGI spec states:
Although this is a frustrating breaking change, it seems the onus is on us (users of Starlette) to adhere to the spec. |
|
As others have commented above, could we please revert this? Although it does not help any of us that the ASGI spec is not clear when it can/should be "missing". |
|
Since |
|
Not sure what is the point of testing |
|
Is there something fundamental I (and others) am/are missing here? |
|
Because it is not a normal client, it is a test client which means at this stage where scope is being set up, no connection is started yet and it's hard-coding the host and port ('testclient', 5000) and is odd because anyway it's an invalid value and the test should mock the Request.client rather than relying on a wrong value. But anyway this is the reason behind it and based on the comments if maintainers want to change this in Starlette:
|
|
Apologies if I was blunt before and thank you for explaining. From my side, I would expect a codebase that works in production to work similarly when a TestClient is thrown at it.
We didn't rely on the actual value, just that it was set. We log the host for auditing reasons. This change has forced me to use |
I think this is a good idea. Setting the client to |
|
My workaround for now is using |
This reverts commit 483849a.
Can you please expand on why a test should mock these aspects of the fixture? It is my impression that that the client, the "server"/application, request, and response are all components that |
This reverts commit 483849a.
Summary
Related to: #2295
I suggest to not include the client scope, instead of always setting it to None. According to the ASGI spec this has the same effect.
Checklist