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
Use updated headers from server state #1706
Conversation
For a first pass can we just describe the change in behaviour against this issue, with a manual demonstration. |
Application: async def app(scope, receive, send):
await send(
{
"type": "http.response.start",
"status": 200,
"headers": [
[b"content-type", b"text/plain"],
],
}
)
await send(
{
"type": "http.response.body",
"body": b"Hello, world!",
}
) Server command (set uvicorn main:app --reload --timeout-keep-alive 100 Client: from time import sleep
from httpx import Client
with Client() as client:
response = client.get("http://localhost:8000")
first_date_header = response.headers["date"]
sleep(3)
response = client.get("http://localhost:8000")
second_date_header = response.headers["date"]
assert first_date_header != second_date_header, "Date headers should be different" You can test the code with and without this PR. 🙏 |
Pragmatically I'd be inclined to just accept this pull request. |
as far as I'm ocncerned I'm not inclined in accepting it without a test. This is a reather simple test, just sending 2 queries waiting 1s in between, there's no magic at all, so the test fails smells really bad, the fact the sleep is not respected in the CI looks bad as well, if the logs displayed are correct of course, the fact noone here can understand why it fails is probably the worse. anyways, the fix looks good, you can merge it without a test, but this will resurface one way or the other because it's highly illogical it's failing at the moment. |
We know why it fails, and why this is the right fix... The Lines 224 to 250 in a94781d
This PR changes where we include the It's a simple fix. It's just awkward to write a test for is all. |
I'm not debating the fix, which is good, I was talking about the test that fails. |
The problem on the test is that comparing time with time.time can lead to this lack of precision. I can mock patch the datetime format on the date header, and also mock patch time.time to use loop.time, and then making sure that is bigger than one second and less than two... But I guess things become too complex? |
@tomchristie @euri10 Can I have an approval here to proceed with the release? 🙏 In any case, I'll try to increase the test coverage to 100% until the end of the year (I'm not sure if I can cover this scenario anyway). |
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.
Absolutely, yup. 👍
Does anyone know a way to test this feature? ;)
Testing with
asyncio.sleep
is not reliable.Related PRs
This was already worked, and reverted twice. I'm sorry for that.
This PR is a blocker for the next release.