-
Notifications
You must be signed in to change notification settings - Fork 3
Run conformance tests with granian, gunicorn, uvicorn too #33
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
Conversation
conformance/test/test_server.py
Outdated
| "--skip", | ||
| "**/bidi-stream/full-duplex/**", | ||
| ] | ||
| # Servers often run out of file descriptors on macOS due to low default ulimit. |
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.
Previously with hypercorn, it would fail due to ulimit issue in a default macos shell without --parallel=1. For some reason with granian it would have issues even with the parallelism. But I realized it's not difficult to automatically increase ulimit like this, so I did so and removed the parallel limit.
|
|
||
| _skipped_tests_async = [ | ||
| "--skip", | ||
| # There seems to be a hypercorn bug with HTTP/1 and request termination. |
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.
This seems to only have been happening with the low ulimit
820b1c4 to
2176f4c
Compare
23251a3 to
be6d91f
Compare
|
While I could work through some flakiness locally in docker, looks like there is still some issue will check it out |
f3ba171 to
be0d36f
Compare
|
Got 3 runs in a row to succeed so hopefully this is fine now but let's keep an eye out for flakiness. I will also see if I can understand why granian would have issues with concurrency since in other testing I've found it to work pretty well. |
| ): | ||
| # Granian seems to have a bug that it prints out 0 rather than the resolved port, | ||
| # so we need to determine it ourselves. If we see race conditions because of it, | ||
| # we can set max-servers=1 in the runner. |
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.
want to add a ref: https://github.com/emmett-framework/granian/issues/711 to this comment so we can revisit when fixed?
conformance/test/server.py
Outdated
| env=_server_env(request), | ||
| ) | ||
| stdout = proc.stdout | ||
| assert stdout is not None # noqa: S101 |
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.
should we just disable the ruff rule that checks for assert usage in conformance/ or conformance/test?
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
7b1c7d4 to
3f54d06
Compare
Signed-off-by: Anuraag Agrawal <[email protected]>
ed148af to
4da1ff5
Compare
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
7ddb53c to
887de92
Compare
|
Sorry for the notification spam - I had good luck yesterday but very much not so today. In the end
It means I can't find a good solution for http/2 on python yet. Will keep on trying |
In preparation for writing a doc on deployment, I wanted to make sure the common app servers are all tested. This adds granian, gunicorn, and uvicorn.
While previously hypercorn was started directly, I switched to just using subprocess for all the servers. It was already pretty tedious to do programmatic start for hypercorn, and some servers like granian it's not practical since they need to control the main thread. Instead of having many different ways of starting a server by code, we get to instead have a consistent pattern with just different CLI flags so overall it seems like a win.