[Django] #33359: Don't close database connections after every TestCase
Django
------------------------------------------------+------------------------
Reporter: Ran Benita | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/33359>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Ran Benita | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Florian Apolloner):
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33359#comment:1>
Django
Reporter: Ran Benita | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam Johnson):
* cc: Adam Johnson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33359#comment:2>
Django
Reporter: Ran Benita | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* resolution: => needsinfo
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33359#comment:3>