Opened 18 months ago

Closed 9 days 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: master
Severity: Release blocker Keywords:
Cc: Adam (Chainz) 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 (22)

comment:1 Changed 18 months ago by Simon Charette

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 Changed 18 months ago by Adam (Chainz) Johnson

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 Changed 18 months ago by Simon Charette

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 18 months ago by Simon Charette (previous) (diff)

comment:4 Changed 18 months ago by Simon Charette

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 Changed 7 months ago by Adam (Chainz) Johnson

Cc: Adam (Chainz) Johnson added

comment:6 Changed 7 months ago by Adam (Chainz) Johnson

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 Changed 7 months ago by Simon Charette

Has patch: set
Patch needs improvement: set

comment:8 Changed 2 months ago by Simon Charette

Patch needs improvement: unset

comment:9 Changed 2 months ago by Simon Charette

Patch needs improvement: set

comment:10 Changed 2 months ago by Simon Charette

Patch needs improvement: unset

comment:11 Changed 12 days ago by Tim Graham <timograham@…>

In 8c775391:

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

comment:12 Changed 12 days ago by Tim Graham <timograham@…>

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 Changed 11 days ago by Tim Graham

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 Changed 11 days ago by Tim Graham

Has patch: unset
Severity: NormalRelease blocker

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

comment:15 Changed 11 days ago by Simon Charette

Cc: Simon Charette added

I'll look into it today.

comment:16 Changed 10 days ago by Simon Charette

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 Changed 10 days ago by Tim Graham

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 Changed 10 days ago by Tim Graham <timograham@…>

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 Changed 9 days ago by Tim Graham <timograham@…>

In a96b9019:

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

comment:20 Changed 9 days ago by Tim Graham <timograham@…>

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 Changed 9 days ago by Tim Graham <timograham@…>

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 Changed 9 days ago by Tim Graham

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top