Context
I contribute to pytest-django, which provides Django integration to the pytest test framework. One of the features missing in pytest-django is an equivalent of setUpTestData() - a way to share DB setup between tests, mostly as a time-saving optimization.
Django’s setUpTestData works only at the scope of a single test class. pytest has a more elaborate fixture system and the expectation is that the feature will work with any scope (class, module, package, entire test session). For those familiar with pytest, my current idea is this:
@pytest.fixture(scope='module')
def my_item(django_test_data):
return Item.objects.create()
I have this implemented as a proof of concept. The django_test_data bit causes the fixture to wrap its corresponding scope with transaction.atomic(), as setUpTestData does.
Problem
In addition to “native” pytest-django tests, pytest-django also supports Django’s unittest tests, which can be intermingled freely. Django’s TestCase closes all database connections during teardown[0]. Now suppose the my_item fixture is active in some test module, and the module also contains a Django TestCase. The connection closing(s) terminates the fixture’s transaction(s) prematurely when the class is torn down, causing all subsequent tests in the module to fail.
Question
Why does Django’s TestCase close the connections? Naively, it just seems to add overhead, since connections are still usable after rollback. As a test, I ran Django’s test suite using sqlite with these lines removed, and all of the tests pass.
Ran
[0] django/testcases.py at 4.0rc1 · django/django · GitHub
Note: this was originally an email, but apparently you need a google account to post there - eek.
Hi @bluetech
I don’t know why Django does that. It could certainly slow down tests unnecessarily.
Running git blame I found this commit changing the lines for closing the connections: Fixed #15838 -- Promoted assertFieldOutput to a general test utility.… · django/django@2664fa1 · GitHub
This change only renamed the variable names. But it shows us the comment that existed before the loop at that time:
# Some DB cursors include SQL statements as part of cursor
# creation. If you have a test that does rollback, the effect
# of these statements is lost, which can effect the operation
# of tests (e.g., losing a timezone setting causing objects to
# be created with the wrong time).
# To make sure this doesn't happen, get a clean connection at the
# start of every test.
That’s a clue - it’s sometimes needed to clean up connection-related variables.
Following git pickaxe (git log -S) back, I must have missed some merge commits, because the next commit mentioning connection.close() is: Fixed #8138 -- Changed django.test.TestCase to rollback tests (when t… · django/django@344f16e · GitHub
This seems to be the first addition of connection.close(), the refactor 13 years ago to use transactions for test case rollback. It was not commented at this point.
Yes I tried some tests too, no problems. I would guess the connection closing behaviour is only useful in particular situations (certain backends or when using certain DB features). It’s possible we could make it optional in Django, which would help pytest-django?
Thanks a lot for looking at this @adamchainz.
That’s a clue - it’s sometimes needed to clean up connection-related variables.
Hmm, I see. Actually, looking at the file some more, I see that the comment is still there, although on a different close call - one which happens per-test, enabled always in TransactionalTestCase but only enabled for backends which don’t support transactions on TestCase.
It’s curious though - I’ve heard of running SQL statements on connection setup but not on cursor setup. And even so, how would a rollback affect it - the cursor lifetime should either be contained in a transaction lifetime, or contain it, and in either case the rollback wouldn’t undo any cursor setup SQL statements.
Yes I tried some tests too, no problems. I would guess the connection closing behaviour is only useful in particular situations (certain backends or when using certain DB features). It’s possible we could make it optional in Django, which would help pytest-django?
Yes, that would help.
Another way to approach it is to only do the closing if not transaction.in_atomic_block. Though that would be specifically for pytest-django, and always false in Django itself, which I know is (rightly) not something Django likes to do. So I think it would be good to check if the connection closes can be removed or restricted to specific cases, which would benefit all Django tests.
Maybe I should create a Django PR which removes these lines and see what, if anything, breaks? Either outcome would provide more info at least.
You can do that, and ask the fellows to help you run it against Oracle too.
I submitted a PR for testing: https://github.com/django/django/pull/15156. All of the tests pass (without Oracle).