-
Notifications
You must be signed in to change notification settings - Fork 22
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 FastAPI test client for testing #322
Conversation
username = default_user() | ||
token = ( | ||
client.post( | ||
"/token", | ||
data={ | ||
"username": username, | ||
"password": os.environ.get( | ||
"AI_PROXY_DEMO_AUTHENTICATION_PASSWORD", username | ||
"RAGNA_DEMO_AUTHENTICATION_PASSWORD", username |
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 was a driveby since I was looking at this function. After that I grep
ed for the wrong string and found one more usage that I also fixed.
|
||
|
||
@pytest.mark.parametrize("database", ["memory", "sqlite"]) |
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.
Removing the memory test here for two reasons:
- remove memory option for state db #320 will remove it from our special cases. Meaning, this is no longer part of our core API, but rather a special case.
- For some reason, in-memory databases don't play nice with the
TestClient
(Documented testing approach does not work for SQLite in-memory instances fastapi/fastapi#3906). The proposed solution, i.e. usingpoolclass=StaticPool
does indeed work, but will cause the streaming test to crash with RunTimeError: got Future <Future pending> attached to a different loop when using custom loop in sync fixtures when upgrading from 0.14.2 to 0.15.0 encode/starlette#1315. I figured it wasn't worth the effort to dive any deeper than that.
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.
LGTM! Makes sense.
Per title. Documentation is here. This has at least three upsides: