Test suite refactor using async server context manager instead of background thread #917
Conversation
|
ok this is almost ready but so far i like i a lot, minus 143 lines, exceptions are bubbling now. this feels way cleaner overall few points to note before it's in a mergeable state:
|
|
pretty happy with it now ! |
requirements.txt
Outdated
| httpx | ||
| pytest-asyncio | ||
| async_generator; python_version < '3.7' |
There was a problem hiding this comment.
Should we pin some of these, perhaps HTTPX and pytest-asyncio to the latest minor versions?
tests/conftest.py
Outdated
| if sys.version_info >= (3, 7): | ||
| from contextlib import asynccontextmanager | ||
| else: | ||
| from async_generator import asynccontextmanager |
There was a problem hiding this comment.
From experience, a great approach for this kind of thing is to have a dedicated "compatibility" module.
Should we move this to a new tests/compat.py?
Also I think we could just try/except:
| if sys.version_info >= (3, 7): | |
| from contextlib import asynccontextmanager | |
| else: | |
| from async_generator import asynccontextmanager | |
| try: | |
| from contextlib import asynccontextmanager | |
| except ImportError: # pragma: no cover | |
| from async_generator import asynccontextmanager |
tests/conftest.py
Outdated
| async def run_server(config: Config, sockets=None): | ||
| server = Server(config=config) | ||
| cancel_handle = asyncio.ensure_future(server.serve(sockets=sockets)) | ||
| await asyncio.sleep(0.1) | ||
| try: | ||
| yield server | ||
| finally: | ||
| await server.shutdown() | ||
| cancel_handle.cancel() |
There was a problem hiding this comment.
I think we already have a utils.py module. What about move this there? If not, maybe we can consider it. Importing from conftest feels a bit strange given this is a special pytest module — things only we use are probably better in a separate module.
There was a problem hiding this comment.
yes you're more than correct, I moved everything (import and run-server) in a new utils.py
| thread.join() | ||
|
|
||
|
|
||
| def test_run_with_shutdown(): |
There was a problem hiding this comment.
Could you clarify why we can do without this test now?
There was a problem hiding this comment.
ok maybe it's me misunderstanding what the test does but..
the way i get it is that it's equivalent to the new async with run-server with the slight exception that a socket sock was explicitly binded, and it just checks the fact no exception is raised in that case.
Now if the context manager was raising an exception all our tests would fail. hence why I didnt bother.
honestly at first I tried to run it "the new way" and couldnt find a way that was really testing something.
uvicorn/server.py
Outdated
| if threading.current_thread() is not threading.main_thread(): | ||
| # Signals can only be listened to from the main thread. | ||
| return |
There was a problem hiding this comment.
We added this via #871, and I don't think we should be dropping it. Sure, maybe we don't really care about it in our own test suite anymore, but people who are running Uvicorn within threads certainly still do. Right?
There was a problem hiding this comment.
Oo, I never think about that, I reverted
| with no_ssl_verification(): | ||
| response = requests.get("https://127.0.0.1:8000") | ||
| async with run_server(config): | ||
| async with httpx.AsyncClient(verify=False) as client: |
There was a problem hiding this comment.
Note for the future: maybe Requests didn't support it, but now with HTTPX I think we could pass verify=<ca_cert>, rather than disabling SSL verification.
There was a problem hiding this comment.
I wanted to mimic the no verification, maybe another PR can use the certificates fixtures !
WIP, but this looks promising, at least exceptions are bubbling up in ws tests !