fix(fixtures): add clean up teardown code to pytest fixture async_sqla_connection #266
Conversation
conste11ations
changed the title
| return create_async_engine(async_sqlalchemy_url) | ||
| engine = create_async_engine(async_sqlalchemy_url) | ||
| yield engine | ||
| engine.dispose() |
There was a problem hiding this comment.
Should this be awaited? IIUC it's an async function
| return create_async_engine(async_sqlalchemy_url) | ||
| engine = create_async_engine(async_sqlalchemy_url) | ||
| yield engine | ||
| engine.dispose() |
There was a problem hiding this comment.
On your notion page your context is anasync fixture`.
The engine.dispose() will just block on this sync fixture context or behind the scene a future/promise is run and will be completed at some point in time ?
There was a problem hiding this comment.
I'm open to ideas on how to add a test case that can validate fixture teardown code that usually happens outside of a test case - otherwise there is the Notion doc's findings. I can also demonstrate that this change + removing the short-term workaround on the other PR = solves the connection leak.
Can we add a test that replicates what was happening when you encountered the issue and confirm that this indeed fixes the issue?
|
Hi @thibremy-dialogue @arththebird, I'm parking this temporarily for now: As it turns out, @arththebird just putting down notes here - we had a chat and we agreed that, in place of a test, I will link this PR branch as a dependency to |
Problem
This issue came up on a test (with many test cases) in this PR. cc @anthony-bennett
Running the full tests were producing the following errors, as the connections gradually were exceeding the postgres instance limits (default of 100).
I created a page in Notion to document the short-term fix we put on our end and to support the use of
Engine.dispose()in relevant fixtures that support our test suites.Solution
Add teardown commands to the fixture that disposes of the created engine. Perhaps it is another question to ask why we have to create the engine so many times but I still think adding this teardown code is best practice.
Supporting documentation from sqlalchemy
Related
Validation
I'm open to ideas on how to add a test case that can validate fixture teardown code that usually happens outside of a test case - otherwise there is the Notion doc's findings. I can also demonstrate that this change + removing the short-term workaround on the other PR = solves the connection leak.