-
-
Notifications
You must be signed in to change notification settings - Fork 782
update async extension and add tests #635
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
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
📝 Docs preview for commit af84d75 at: https://5fc91ef2.sqlmodel.pages.dev |
This comment was marked as resolved.
This comment was marked as resolved.
|
📝 Docs preview for commit fb60073 at: https://f6ad04c2.sqlmodel.pages.dev |
|
Fix conflicts |
|
📝 Docs preview for commit 3e9bfd8 at: https://c7b5a1e8.sqlmodel.pages.dev |
|
This pull request has a merge conflict that needs to be resolved. |
YuriiMotov
left a comment
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.
@PookieBuns, thanks for your interest and efforts!
This looks a bit raw and contains changes that are not necessary (testcontainers).
Would you like to continue working on this?
|
|
||
| # create_async_engine by default already has future set to be true. | ||
| # Porting this over to sqlmodel to make it easier to use. | ||
| def create_async_engine(*args: Any, **kwargs: Any) -> AsyncEngine: |
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 way we don't have autocompletion and type hints for parameters
| testcontainers = "^3.7.1" | ||
| psycopg2-binary = "^2.9.7" | ||
| asyncpg = "^0.28.0" |
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.
It may be good idea in general to introduce testcontainers and test SQLModel with real Postgres, but for the reasons of this PR I think we can just use aiosqlite driver and in-memory DB
| assert hero_create == hero_query | ||
|
|
||
|
|
||
| def test_async_create(postgres_container_url: str) -> None: |
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 need to use @pytest.mark.anyio to run async tests: https://anyio.readthedocs.io/en/stable/testing.html
| # SQLModel.metadata is a singleton and the Hero Class has already been defined. | ||
| # If I flush the metadata during this test, it will cause test_enum to fail | ||
| # because in that file, the model isn't defined within a function. For now, the | ||
| # workaround is to set extend_existing to True. In the future, test setup and | ||
| # teardown should be refactored to avoid this issue. | ||
| __table_args__ = {"extend_existing": True} |
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.
We can clear DB before each test, that's common approach.
Or, use in-memory SQLite DB with aiosqlite driver
|
I will re-open this over the weekend. It's been a while 😂 |
from sqlmodel.ext.asyncio import AsyncSessionwhich obeys the import format of sqlalchemy Add documentation about how to use the async tools (session, etc) #626 (comment)Doesn't really address the documentation part of #626 but adds a few QOL features and sets up an easy testing framework for async features