test: add tests in test_requests - #2677
Conversation
| # (" = b ; ; = ; c = ; ", {"": "b", "c": ""}), | ||
| (" = b ; ; = ; c = ; ", {"": "b", "c": ""}), |
There was a problem hiding this comment.
Testing this, when the key and value both are empty, this pair should be ignored.
I don't know why this parameter was commented out before. However, to meet the condition where both the key and value are empty, this parameter with an empty chunk ("; ;") is what we need.
There was a problem hiding this comment.
This may have been on purpose? When was introduced?
There was a problem hiding this comment.
The test and cookie_parser were introduced in this PR.
It looks like these tests were inspired by tornado test suite ( relevant commit ).
And I didn't see any discussion related to this specific parameter. It was just commented out at the beginning.
My thought is that enabling the parameter shouldn't be a problem. cookie_parser does support this case.
There was a problem hiding this comment.
Just did more searching in Tornado's code and found this. They did have tested the case.
test_requests
| def test_request_lazy_load_property(test_client_factory: TestClientFactory) -> None: | ||
| async def app(scope: Scope, receive: Receive, send: Send) -> None: | ||
| request = Request(scope, receive) | ||
| assert not hasattr(request, "_url") | ||
| assert not hasattr(request, "_query_params") | ||
| assert not hasattr(request, "_json") | ||
| # trigger lazy loading | ||
| _, _, _ = request.url, request.query_params, await request.json() | ||
| assert hasattr(request, "_url") | ||
| assert hasattr(request, "_query_params") | ||
| assert hasattr(request, "_json") | ||
| data = { | ||
| "url": str(request.url), | ||
| "query_params": dict(request.query_params), | ||
| "json": await request.json(), | ||
| } | ||
| response = JSONResponse(data) | ||
| await response(scope, receive, send) | ||
|
|
||
| client = test_client_factory(app) | ||
| response = client.post("/42?foo=bar", json={"baz": "qux"}) | ||
| assert response.json() == { | ||
| "url": "http://testserver/42?foo=bar", | ||
| "query_params": {"foo": "bar"}, | ||
| "json": {"baz": "qux"}, | ||
| } |
There was a problem hiding this comment.
Which lines are this trying to cover?
There was a problem hiding this comment.
Or should we add pragma: no branch to these lines too?
There was a problem hiding this comment.
The thing is that this test is doing too much, maybe we can divide it a bit? Or maybe just the no branch... I don't want us to try to satisfy the coverage just because we want to satisfy it, but actually create meaningful tests (naming wise as well).
There was a problem hiding this comment.
Or maybe just the
no branch
No problem, let's go with this.
|
Thanks @Orenoid ! 🙏 |
Summary
Add tests for some of the uncovered branches in
starlette.requests, related to this issueChecklist