#28478 closed Cleanup/optimization (fixed)
Make DiscoverRunner skip creating a test database if not needed
Reported by: | Tim Graham | Owned by: | nobody |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | Adam Johnson, Simon Charette | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Inspired by https://gist.github.com/zenweasel/7495758 and https://www.caktusgroup.com/blog/2013/10/02/skipping-test-db-creation/, DiscoverRunner
could skip creating the test database if no TransactionTestCase
or its subclasses are used.
I'd suggest adding an attribute on the test classes such as _needs_database
rather than using isinstance(test, TransactionTestCase)
in those patches. This would allow compatibility in case Django users have other classes using the database that don't inherit from Django's classes (though I'm not aware of anyone doing this).
Change History (25)
comment:1 by , 7 years ago
comment:2 by , 7 years ago
N.B. pytest-django does this already, it makes the test databases pytest fixtures and attaches them to TransactionTestCase etc., if you run a set of tests without such classes then the fixture is never invoked.
comment:3 by , 7 years ago
I wonder if we could unify SimpleTestCase.allow_database_queries
, TransactionTestCase.mutli_db
and this feature under a single SimpleTestCase.databases
attribute.
The databases
attribute could be set to None
on SimpleTestCase
and to {DEFAULT_DB_ALIAS}
on TransactionTestCase
with a deprecation shim that turns multi_db = True
into set(connections)
. The discover runner could simply create the union of all tests's databases
and the allow_database_queries
functionality could be reused to block accesses to all connection aliases not defined in the test's databases
.
comment:4 by , 7 years ago
Here's a POC of what I had in mind. The patch is still missing documentation, appropriate warnings and probably doesn't correctly deal with mirroring setups but it correctly only creates the required databases while still respecting allow_database_queries
and multi_db
.
comment:5 by , 6 years ago
Cc: | added |
---|
comment:6 by , 6 years ago
The PR looks like a good start... do you have time to continue working on it? I'll happily help with review, since I was bitten by multi_db in #29513.
comment:8 by , 6 years ago
Patch needs improvement: | unset |
---|
comment:9 by , 6 years ago
Patch needs improvement: | set |
---|
comment:10 by , 6 years ago
Patch needs improvement: | unset |
---|
comment:13 by , 6 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
A failure appeared after 41e73de39df31c4b13d65462bfeedde6924226d8:
./tests/runtests.py --settings=test_postgres postgres_tests Testing against Django installed in '/home/tim/code/django/django' with up to 3 processes PostgreSQL 90515 Creating test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… System check identified no issues (0 silenced). ... ====================================================================== ERROR: test_datetime_prepare_value (postgres_tests.test_ranges.TestFormField) ---------------------------------------------------------------------- Traceback (most recent call last): File "/opt/python3.7.0/lib/python3.7/unittest/case.py", line 59, in testPartExecutor yield File "/opt/python3.7.0/lib/python3.7/unittest/case.py", line 615, in run testMethod() File "/home/tim/code/django/django/test/utils.py", line 370, in inner with self as context: File "/home/tim/code/django/django/test/utils.py", line 338, in __enter__ return self.enable() File "/home/tim/code/django/django/test/utils.py", line 419, in enable self.disable() File "/home/tim/code/django/django/test/utils.py", line 437, in disable raise exc File "/home/tim/code/django/django/test/utils.py", line 415, in enable setting=key, value=new_value, enter=True, File "/home/tim/code/django/django/dispatch/dispatcher.py", line 175, in send for receiver in self._live_receivers(sender) File "/home/tim/code/django/django/dispatch/dispatcher.py", line 175, in <listcomp> for receiver in self._live_receivers(sender) File "/home/tim/code/django/django/test/signals.py", line 74, in update_connections_time_zone conn.ensure_timezone() File "/home/tim/code/django/django/db/backends/postgresql/base.py", line 198, in ensure_timezone self.ensure_connection() File "/home/tim/code/django/django/db/backends/base/base.py", line 216, in ensure_connection self.connect() File "/home/tim/code/django/django/db/utils.py", line 89, in __exit__ raise dj_exc_value.with_traceback(traceback) from exc_value File "/home/tim/code/django/django/db/backends/base/base.py", line 216, in ensure_connection self.connect() File "/home/tim/code/django/django/db/backends/base/base.py", line 194, in connect self.connection = self.get_new_connection(conn_params) File "/home/tim/code/django/django/db/backends/postgresql/base.py", line 178, in get_new_connection connection = Database.connect(**conn_params) File "/home/tim/.virtualenvs/django37/lib/python3.7/site-packages/psycopg2/__init__.py", line 130, in connect conn = _connect(dsn, connection_factory=connection_factory, **kwasync) django.db.utils.OperationalError: FATAL: database "djangotests2_2" does not exist
Add --parallel=1
and there's no problem.
comment:14 by , 6 years ago
Has patch: | unset |
---|---|
Severity: | Normal → Release blocker |
The problem also affects other test apps when run in isolation on PostgreSQL.
comment:16 by , 6 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
Some tests were failing because the patch wasn't mocking all connection interaction attempts. If test database creation is skipped then any attempts to connect to it will necessarily fail.
The current patch prevents that from happening by mocking the connect
method but it unveiled a few SimpleTestCase
that happened to silently interact with the database.
The only part missing at this point is to decide whether or not we want to make skipUnlessDBFeature
and friends usage on SimpleTestCase
without databases
override error out or go through a deprecation period where it automatically adds DEFAULT_CONNECTION_ALIAS
to databases and raise a warning instead. Given the use case is quite nice I think error'ing out should be fine but given we had some instances of this pattern in the test suite I thought it was still worth discussing.
comment:17 by , 6 years ago
I don't expect user projects use skipUnlessDBFeature
much (it would be more like libraries that test against different databases) so I don't a deprecation adds much benefit.
comment:22 by , 6 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I suggest we expose a
_needed_databases
attribute instead which returnsset(connections) if self.multi_db else {DEFAULT_DB_ALIAS}
to optimize the case where only the default database is needed as well.