Skip to content
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

feat: Add flag to clean up postgres databases. #204

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DanCardin
Copy link
Contributor

@DanCardin DanCardin commented Mar 7, 2024

Fixes #202.

  • Adds cleanup_databases=False argument. Default False to retain original behavior
  • Cleans up individual test databases during the natural fixture teardown phase, fairly straightforward
  • Cleaning up the template databases is a lot more involved and a majority of the refactor being done by this PR
    • Internally we now create 1 new (randomly named) session fixture per create_postgres_fixture call
    • that gets hackily injected into the calling code's global scope under a uniqified name
    • we then use the resultant fixture to provide us a scope from which we can perform tear-down logic to delete the template databases at exit.

It probably makes sense to follow this up with a pmr --cleanup or the like kind of CLI extension, that more directly just deletes all pmr_* named databases. If there's a hard test failure, you enter a debugger and exit uncleanly, you ctrl+c enough, or various other means: it's still fairly straightforward to end up with left over databases.

1 unresolved issue:

  • pytest-asyncio has all sorts of weird stuff going on in both prior versions that people claim works, and the new versions are just broken apparently. In either case, I wasn't able to find a way that seemed to work reliably in async code (at least, using pytest-asyncio). Only affecting template database cleanup.

    So unless/until there's a different plugin, or they resolve their stuff and we start requiring a much more recent minimum version bound for it after they do, template database cleanup will mean that async code wont share templates (and thus will be slower). Could be worse, but isn't ideal.

@josiah-lunit
Copy link

For issues regarding pytest-asyncio (which I agree is not in a good state now), I find using nest_asyncio resolved some of the issues I had with different scoped fixtures.

@DanCardin
Copy link
Contributor Author

A potentially good callout, although it'd mean it would mandate use of it for downstream users, not just our tests.

The current test failures seem to not be related at least...which makes sense, since i'm bypassing the new fixture for async code.

They're also not a failure I'm experiencing locally...

@DanCardin DanCardin force-pushed the dc/database-cleanup branch 4 times, most recently from 80fea86 to 8f4f321 Compare April 15, 2024 12:54
@DanCardin DanCardin force-pushed the dc/database-cleanup branch 5 times, most recently from a8c9dbe to fffe2ff Compare April 15, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create_postgres_fixture doesn't clean up after running tests.
2 participants