Skip to content

Commit

Permalink
Add test helpers for dynamic database-based tests
Browse files Browse the repository at this point in the history
See #2115 for full details.

The problem this refactoring aims to solve is, in short, as quoted from
@mikerkelly in #2115:

> The [DatabaseOperationForbidden] errors occur because there's a mismatch
> between what [these test cases](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py)
> that import databases dynamically are
> trying to do and how both the Django and `pytest-django` test case
> frameworks work. In a multi-database context, the test frameworks want
> to declare which databases are available to a test case at
> test-case-definition-time, which then triggers those databases being
> created, the test case wrapped in a transaction referencing those
> databases, and the databases flushed in clean up. Since 5.1 it's
> enforced that test cases only open connections to such databases, to
> avoid accidental access. But the [coding system release import tests](https://github.com/opensafely-core/opencodelists/blob/main/coding_systems/bnf/tests/test_import_data.py)
> do want to access a database that they do not want to exist at the start
> of the test. That's in conflict. So maybe we shouldn't be using test cases
> this way at all?

Mike proposed several ideas to fix this that we looked at, and we found that:
amending the test case attribute that defines the allowed databases
seems reasonable, and, further, this was the only solution that we had working.

It was decided that refactoring the existing tests was the solution for
now, rather than rethinking them or the code under test entirely. This
facilitates the upgrade to Django 5.1 without blocking us further. We
may have to revisit this code and these tests in future, with a view to
a more comprehensive overhaul.

This single class facilitates getting most of the tests working by just
moving them into a test class that uses this DynamicDatabaseTestCase
class. Those tests have a generalisable pattern for accessing the
databases. The BNF and base coding system tests are slightly more
complicated, and the issues will be described in those respective
subsequent commits.

Co-authored-by: Mike Kelly <mike.kelly@thedatalab.org>
  • Loading branch information
StevenMaude and mikerkelly committed Feb 5, 2025
1 parent a8675bb commit f249db5
Showing 1 changed file with 84 additions and 0 deletions.
84 changes: 84 additions & 0 deletions coding_systems/base/tests/helpers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import pytest
from django.test import TestCase


class DynamicDatabaseStateNotRecordedError(Exception):
pass


class DynamicDatabaseTestCase(TestCase):
"""A special TestCase subclass to facilitate use of
a dynamically created database.
db_alias: name of the database that needs to be accessed, str (required)
coding_system: name of the coding system, for generating a temporary
database directory, str or None
import_data_path: name of the path to import data used, str or None
"""

@property
def db_alias(self):
# The db_alias that will be added temporarily to the DB.
raise NotImplementedError(
"This test class requires a database alias to be set."
)

coding_system = None
import_data_path = None

@pytest.fixture
def _get_tmp_path(self, coding_systems_tmp_path):
"""Runs the fixture that creates a temporary path."""
self.coding_systems_tmp_path = coding_systems_tmp_path
self.expected_db_path = (
self.coding_systems_tmp_path
/ f"{self.coding_system}"
/ f"{self.db_alias}.sqlite3"
)

@pytest.fixture(autouse=True)
def _configure_tmp_path(self, request):
if self.coding_system is not None:
request.getfixturevalue("_get_tmp_path")

def add_to_databases(self, *args):
"""Add database names to the test instance's allowed databases.
On first run, the originally allowed databases are recorded,
for later restoration with restore_original_databases."""
try:
getattr(self, "original_databases")
except AttributeError:
# Record the original state, so we can later restore it.
self.original_databases = type(self).databases

type(self).databases |= frozenset({*args})

def restore_original_databases(self):
try:
getattr(self, "original_databases")
except AttributeError:
# The list of original databases was never set.
# This should never happen for a test based on this class.
# We record the state when first adding to the databases.
raise DynamicDatabaseStateNotRecordedError
else:
# Remove the dynamic database from the test class, as Django doesn't
# know how to roll back when the transaction wrapping the test case ends.
type(self).databases = self.original_databases

def setUp(self):
super().setUp()

# Mutate *class* state, this attribute determines to which databases
# SimpleTestCase.ensure_connection_patch_method will allow connections.
# We can't patch this directly in the test case as the class is
# constructed dynamically. No need to reset as each test case execution
# gets a new dynamic class.
self.add_to_databases(self, self.db_alias)

# Not necessary to remove the DB as the temp dir is scoped by test case.

def tearDown(self):
super().tearDown()
self.restore_original_databases()

0 comments on commit f249db5

Please sign in to comment.