-
-
Notifications
You must be signed in to change notification settings - Fork 930
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add tests in test_requests
#2677
Changes from 4 commits
0ed4529
6de3190
adeb680
7c5047c
1fb0ff2
f3e2813
4258aa8
5011ad8
040f1b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,34 @@ async def app(scope: Scope, receive: Receive, send: Send) -> None: | |
assert response.json() == {"method": "GET", "url": "https://example.org:123/"} | ||
|
||
|
||
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"}, | ||
} | ||
|
||
|
||
def test_request_query_params(test_client_factory: TestClientFactory) -> None: | ||
async def app(scope: Scope, receive: Receive, send: Send) -> None: | ||
request = Request(scope, receive) | ||
|
@@ -78,6 +106,17 @@ def test_request_client(scope: Scope, expected_client: Address | None) -> None: | |
assert client == expected_client | ||
|
||
|
||
@pytest.mark.anyio | ||
async def test_request_close() -> None: | ||
request = Request({"type": "http", "method": "GET", "path": "/foo/"}) | ||
assert request._form is None | ||
# When request.`_form` is None, close() should do nothing. | ||
# To verify this behavior, we could just call request.close() here. | ||
# If request.`_form`.close is called, | ||
# an Exception will be raised and the test will fail. | ||
await request.close() | ||
Orenoid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
def test_request_body(test_client_factory: TestClientFactory) -> None: | ||
async def app(scope: Scope, receive: Receive, send: Send) -> None: | ||
request = Request(scope, receive) | ||
|
@@ -425,7 +464,7 @@ async def app(scope: Scope, receive: Receive, send: Send) -> None: | |
# Browsers don't send extra whitespace or semicolons in Cookie headers, | ||
# but cookie_parser() should parse whitespace the same way | ||
# document.cookie parses whitespace. | ||
# (" = b ; ; = ; c = ; ", {"": "b", "c": ""}), | ||
(" = b ; ; = ; c = ; ", {"": "b", "c": ""}), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may have been on purpose? When was introduced? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No idea. The test and My thought is that enabling the parameter shouldn't be a problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just did more searching in Tornado's code and found this. They did have tested the case. |
||
], | ||
) | ||
def test_cookies_invalid( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which lines are this trying to cover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/encode/starlette/blob/master/starlette/requests.py#L96
https://github.com/encode/starlette/blob/master/starlette/requests.py#L125
https://github.com/encode/starlette/blob/master/starlette/requests.py#L240
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should we add
pragma: no branch
to these lines too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, let's go with this.