Go back to using a file-backed SQLite database for tests

Using a memory database causes tests/test_build to intermittently
fail, because using the same pysqlite3 connection object from multiple
threads - as was done so that the threads shared the same memory database
- is not, in the end, thread safe. One thread will stomp on the transaction
state of other threads, resulting in errors from starting a new transaction
when another is already in progress, or trying to commit a transaction
that is not in progress.

To avoid a significant speed penalty, the session-scope fixture sets up
a database in the pytest temporary directory, which will typically be on
tmpfs. Time to complete all tests:

 memory backend:               38 seconds
 file on tmpfs:                40 seconds
 file on nvme ssd with btrfs: 137 seconds

MBSSQLAlchemy, which attempted to make the memory backend work, is removed.

Session hooks are installed on the Session class rather than on the
scoped_session instance - this works better when we're changing from
one database to another at test setup time.

module_build_service/__init__.py

@@ -24,7 +24,6 @@ import pkg_resources
 
 from flask import Flask, has_app_context, url_for
 from flask_sqlalchemy import SQLAlchemy
-from sqlalchemy.pool import StaticPool
 
 from module_build_service.common.config import config_section
 from module_build_service.web.proxy import ReverseProxy
@@ -40,27 +39,7 @@ app.wsgi_app = ReverseProxy(app.wsgi_app)
 app.config.from_object(config_section)
 
 
-class MBSSQLAlchemy(SQLAlchemy):
-    """
-    Inherits from SQLAlchemy and if SQLite in-memory database is used,
-    sets the driver options so multiple threads can share the same database.
-
-    This is used *only* during tests to make them faster.
-    """
-
-    def apply_driver_hacks(self, app, info, options):
-        if info.drivername == "sqlite" and info.database in (None, "", ":memory:"):
-            options["poolclass"] = StaticPool
-            options["connect_args"] = {"check_same_thread": False}
-            try:
-                del options["pool_size"]
-            except KeyError:
-                pass
-
-        super(MBSSQLAlchemy, self).apply_driver_hacks(app, info, options)
-
-
-db = MBSSQLAlchemy(app)
+db = SQLAlchemy(app)
 
 
 def get_url_for(*args, **kwargs):

module_build_service/common/config.py

@@ -41,6 +41,8 @@ class BaseConfiguration(object):
 
 class TestConfiguration(BaseConfiguration):
     LOG_LEVEL = "debug"
+    # This is just for initialization - we override this with a database in the
+    # pytest temporary directory
     SQLALCHEMY_DATABASE_URI = os.environ.get("DATABASE_URI", "sqlite:///:memory:")
     DEBUG = True
     MESSAGING = "in_memory"

module_build_service/scheduler/db_session.py

@@ -4,7 +4,7 @@ from __future__ import absolute_import
 
 import sqlalchemy.event
 from sqlalchemy.pool import NullPool
-from sqlalchemy.orm import scoped_session, sessionmaker
+from sqlalchemy.orm import scoped_session, sessionmaker, Session
 
 from module_build_service.common.config import conf
 from module_build_service.common.models import (
@@ -24,8 +24,8 @@ def _setup_event_listeners(db_session):
     )
 
     for event, handler in event_hooks:
-        if not sqlalchemy.event.contains(db_session, event, handler):
-            sqlalchemy.event.listen(db_session, event, handler)
+        if not sqlalchemy.event.contains(Session, event, handler):
+            sqlalchemy.event.listen(Session, event, handler)
 
     # initialize DB event listeners from the monitor module
     from module_build_service.common.monitor import db_hook_event_listeners

tests/__init__.py

@@ -13,6 +13,7 @@ import yaml
 from traceback import extract_stack
 
 import koji
+import flask_sqlalchemy
 from mock import patch
 from six import string_types
 
@@ -132,7 +133,7 @@ def truncate_tables():
     db_session.commit()
 
 
-def clean_database(add_platform_module=True, add_default_arches=True):
+def clean_database(database_uri=None, add_platform_module=True, add_default_arches=True):
     """Initialize the test database
 
     This function is responsible for dropping all the data in the database and
@@ -142,6 +143,19 @@ def clean_database(add_platform_module=True, add_default_arches=True):
     Flask-SQLAlchemy.
     """
 
+    if database_uri is not None:
+        # When starting tests, we switch from the memory backend to a
+        # database in the pytest temporary directory, requiring some
+        # hacks
+        db.session.remove()
+        db.session = None
+
+        # clear any cached engine connections
+        flask_sqlalchemy.get_state(module_build_service.app).connnectors = {}
+
+        module_build_service.app.config['SQLALCHEMY_DATABASE_URI'] = database_uri
+        db.session = db.create_scoped_session()
+
     # Helpful for writing tests if any changes were made using the database
     # session but the test didn't commit or rollback.
     #

tests/conftest.py

@@ -281,6 +281,17 @@ def provide_test_client(request):
 
 
 @pytest.fixture(autouse=True, scope="session")
-def create_database(request):
+def create_database(request, tmpdir_factory):
     """Drop and recreate all tables"""
-    clean_database(add_platform_module=False, add_default_arches=False)
+
+    # pysqlite + the SQLite memory backend doesn't handle multiple threads usefully:
+    # either each thread gets its own backend, or the threads stomp on each others
+    # transaction state. So, before running tests, replace the memory backend with
+    # a file backend - if this ends up on tmpfs, its almost as fast as the memory
+    # backend.
+    database_uri = None
+    if module_build_service.app.config["SQLALCHEMY_DATABASE_URI"] == 'sqlite:///:memory:':
+        database_path = str(tmpdir_factory.getbasetemp() / "test.db")
+        database_uri = 'sqlite:///' + database_path
+
+    clean_database(database_uri=database_uri, add_platform_module=False, add_default_arches=False)