-
Notifications
You must be signed in to change notification settings - Fork 91
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
integration test update for Esmerald #1087
integration test update for Esmerald #1087
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1087 +/- ##
==========================================
- Coverage 91.91% 89.22% -2.69%
==========================================
Files 116 116
Lines 8591 8591
==========================================
- Hits 7896 7665 -231
- Misses 695 926 +231
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -12,10 +12,10 @@ | |||
SERVERS = ["uvicorn", "Hypercorn", "granian"] | |||
ROUTER_DEPENDENCIES = { | |||
"starlette": ["starlette"], | |||
"fastapi": ["fastapi>=0.112.1"], |
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.
I think we version pinned this because of a version mismatch with Starlette - presumably this is fixed now.
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.
Yes, I think it's fixed and I had no problems in testing.
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.
Cool, thanks
@@ -40,6 +40,7 @@ async def receive(): | |||
"method": "GET", | |||
"query_string": b"", | |||
"headers": [], | |||
"_body": b"null", |
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.
I looked into this a bit, and you're right - _body
seems to be the only thing different in the scopes.
I looked at the Esmerald codebase, and it looks like _body
is just an Esmerald thing, and not a generic ASGI thing, so even though it works, I can't really explain why this works.
I had an idea - we could use the httpx test client instead:
import asyncio
import importlib
import sys
import typing as t
from httpx import AsyncClient
async def dummy_server(app: t.Union[str, t.Callable] = "app:app"):
"""
A very simplistic ASGI server. It's used to run the generated ASGI
applications in unit tests.
:param app:
Either an ASGI app, or a string representing the path to an ASGI app.
For example, ``module_1.app:app`` which would import an ASGI app called
``app`` from ``module_1.app``.
"""
print("Running dummy server ...")
if isinstance(app, str):
path, app_name = app.rsplit(":")
module = importlib.import_module(path)
app = getattr(module, app_name)
async with AsyncClient(app=app) as client:
response = await client.get('http://localhost:8000')
if response.status_code != 200:
sys.exit("The app isn't callable!")
if __name__ == "__main__":
asyncio.run(dummy_server())
Our existing dummy server must be too simplistic, and isn't doing something.
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 is great. I tried that and it works great. I just need to add httpx
to test-requirements.txt
and replace
async def dummy_server(app: t.Union[str, t.Callable] = "app:app"):
with
async def dummy_server(app: t.Union[t.Any, t.Callable] = "app:app"):
to keep mypy happy.
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.
Oh yeah, I see the mypy error now.
You can also do this:
app = t.cast(t.Callable, getattr(module, app_name))
Instead of setting the arg type to Any
.
@sinisaos Looks great, thanks 👍 |
@dantownsend Thank you for the great suggestion on how to implement a |
No problem! |
Related to #1078