Opened 2 years ago

Closed 2 years ago

#33359 closed Cleanup/optimization (needsinfo)

Don't close database connections after every TestCase

Reported by: Ran Benita Owned by: nobody
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Adam Johnson Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Django's TestCase closes all DB connections at tearDownClass: https://github.com/django/django/blob/4.0/django/test/testcases.py#L1235-L1236

This makes it impossible to wrap a TestCase inside a larger DB transaction. This is something that an external project, pytest-django, would like to do. See here for more discussion: https://forum.djangoproject.com/t/why-does-django-close-db-connections-between-test-classes/10782

Besides the pytest-django angle (which Django itself should not really consider), DB connection setup can be somewhat heavy and so needlessly closing them adds overhead to the test run.

We've tried to figure out why it was added. It seems to involve databases which perform SQL queries during *cursor* setup but it does not make much sense to me (see Django forum discussion).

We've run the Django CI with these lines removed (including Oracle) and all of the tests pass.

PR: https://github.com/django/django/pull/15156

Change History (3)

comment:1 by Florian Apolloner, 2 years ago

DB connection setup can be somewhat heavy and so needlessly closing them adds overhead to the test run.

Agreed, this can be "heavy" -- do you mind running with sqlite & postgres locally and report timings with and without your patch?

What I am a bit worried though is that the state of the connection is essentially undefined after running a test, so I wonder if we should add a reset method or similar on the connection to restore it to it's initial state; for instance see docs about server_reset_query in pgbouncer:

The query is supposed to clean any changes made to the database session so that the next client gets the connection in a well-defined state. The default is DISCARD ALL which cleans everything, but that leaves the next client no pre-cached state. It can be made lighter, e.g. DEALLOCATE ALL to just drop prepared statements, if the application does not break when some state is kept around.

We might have to rerun connection initialization queries after that though.

comment:2 by Adam Johnson, 2 years ago

Cc: Adam Johnson added

comment:3 by Mariusz Felisiak, 2 years ago

Resolution: needsinfo
Status: newclosed

This change crashes many tests for non-in-memory database on SQLite, e.g.:

$ ./runtests.py backends queries test_utils
...
  File "/django/django/db/backends/sqlite3/base.py", line 268, in create_cursor
    return self.connection.cursor(factory=SQLiteCursorWrapper)
django.db.utils.ProgrammingError: Cannot operate on a closed database.

It's tempting to remove these two lines, but more research is needed.

Note: See TracTickets for help on using tickets.
Back to Top