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

Improve robustness of tests with sqlalchemy #17

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

Conversation

nphilipp
Copy link
Contributor

@nphilipp nphilipp commented Dec 14, 2019

This PR fixes some issues with quickstarted projects:

  • model.init_model() could be called multiple times through loading the app (functional tests) or tests.models.setup_db() but wouldn't dispose of the previous session
  • mixed use of transaction and DBSession APIs to manage transactions
  • websetup assumes that transactions are in the ACTIVE state but this doesn't hold true if teardown_db() was called before

This would cause functional tests to fail setting up the test application if setup_db() was called in a previous test, which can easily be tested by renaming the tests.functional to e.g. tests.zzz_functional in a quickstarted project and then running the test suite.

NB: Having slept a night over it, a different way to address this would be to have exactly one test app, and one call to setup_db() prior to all tests but I'm not sure how feasible this is.

Different levels of tests (e.g. app/functional, db model) can cause
init_model() to be called a couple of times. Dispose of the current
session (if any) so subsequent tests can start with a clean slate.
Mixed use of transaction and SQLAlchemy session APIs for handling
transactions in tests can cause consecutive tests that in some way
access the database to fail. The websetup package uses the transaction
API, so use the same in the model test harness.
This is to deal with potentially aborted transactions in tests.
@amol-
Copy link
Member

amol- commented Dec 23, 2019

DBSession.rollback() was used because in theory you can have a project with SQLalchemy but without the transaction manager. I'm not sure if that's still working as expected as I have never done that, but it was in theory an option.

I'll try to reproduce locally the behaviour to have a better understanding of what's going on. I'm fairly ok with the changes, but I want to understand what causes them to be needed before merging them.

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.

2 participants