Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#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 Simon Charette, 7 years ago

I suggest we expose a _needed_databases attribute instead which returns set(connections) if self.multi_db else {DEFAULT_DB_ALIAS} to optimize the case where only the default database is needed as well.

comment:2 by Adam Johnson, 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 Simon Charette, 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.

Last edited 7 years ago by Simon Charette (previous) (diff)

comment:4 by Simon Charette, 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 Adam Johnson, 6 years ago

Cc: Adam Johnson added

comment:6 by Adam Johnson, 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:7 by Simon Charette, 6 years ago

Has patch: set
Patch needs improvement: set

comment:8 by Simon Charette, 6 years ago

Patch needs improvement: unset

comment:9 by Simon Charette, 6 years ago

Patch needs improvement: set

comment:10 by Simon Charette, 6 years ago

Patch needs improvement: unset

comment:11 by Tim Graham <timograham@…>, 6 years ago

In 8c775391:

Refs #28478 -- Deprecated TestCase's allow_database_queries and multi_db in favor of databases.

comment:12 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: newclosed

In 41e73de3:

Fixed #28478 -- Make DiscoverRunner skip creating unused test databases.

SimpleTestCase.databases makes it possible to determine the set of
databases required to run the discovered tests.

comment:13 by Tim Graham, 6 years ago

Resolution: fixed
Status: closednew

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 Tim Graham, 6 years ago

Has patch: unset
Severity: NormalRelease blocker

The problem also affects other test apps when run in isolation on PostgreSQL.

comment:15 by Simon Charette, 6 years ago

Cc: Simon Charette added

I'll look into it today.

comment:16 by Simon Charette, 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 Tim Graham, 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:18 by Tim Graham <timograham@…>, 6 years ago

In a02a6fd5:

Refs #28478 -- Prevented connection creation in model_indexes tests.

Entering a SchemaEditor instance creates a connection but it isn't needed
for this test.

comment:19 by Tim Graham <timograham@…>, 6 years ago

In a96b9019:

Refs #28478 -- Prevented timezone assignment for unusable PostgreSQL connections.

comment:20 by Tim Graham <timograham@…>, 6 years ago

In f5b63508:

Refs #28478 -- Prevented connection attempts against disallowed databases in tests.

Mocking connect as well as cursor methods makes sure an appropriate error
message is surfaced when running a subset of test attempting to access a
a disallowed database.

comment:21 by Tim Graham <timograham@…>, 6 years ago

In b181aba7:

Refs #28478 -- Prevented database feature based skipping on tests disallowing queries.

Database features may require a connection to be established to determine
whether or not they are enabled.

comment:22 by Tim Graham, 6 years ago

Resolution: fixed
Status: newclosed

comment:23 by Carlton Gibson <carlton.gibson@…>, 6 years ago

In 7071f8f2:

Fixed #30193, Refs #28478 -- Avoided PostgreSQL connection health checks on initialization.

This addressed a regression introduced by a96b9019320ed8236659ee520a7a017c1bafbc6f as identified by Ran Benita.

comment:24 by Carlton Gibson <carlton.gibson@…>, 6 years ago

In 7f25344c:

[2.2.x] Fixed #30193, Refs #28478 -- Avoided PostgreSQL connection health checks on initialization.

This addressed a regression introduced by a96b9019320ed8236659ee520a7a017c1bafbc6f as identified by Ran Benita.
Backport of 7071f8f2729295b0da77b6c651966dc32c71f1ab from master

comment:25 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In b61ea567:

Refs #28478 -- Removed support for TestCase's allow_database_queries and multi_db per deprecation timeline.

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