[Django] #28478: Make DiscoverRunner skip creating a test database if not needed
Django
------------------------------------------------+------------------------
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/28478>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:1>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:2>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
I wonder if we could unify `SimpleTestCase.allow_database_queries`,
`TransactionTestCase.mutli_db` and this feature under a single
`SimpleTestCase.databases` feature.
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 accessed
to all connection aliases not defined in the test's `databases`.
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:3>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Here's a
[https://github.com/django/django/compare/master...charettes:ticket-28478
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:4>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam (Chainz) Johnson):
* cc: Adam (Chainz) Johnson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:5>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:6>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/10175 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:7>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:8>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:9>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:10>
Django
Reporter: Tim Graham | Owner: nobody
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"41e73de39df31c4b13d65462bfeedde6924226d8" 41e73de3]:
{{{
#!CommitTicketReference repository=""
revision="41e73de39df31c4b13d65462bfeedde6924226d8"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:12>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"8c775391b78b2a4a2b57c5e89ed4888f36aada4b" 8c775391]:
{{{
#!CommitTicketReference repository=""
revision="8c775391b78b2a4a2b57c5e89ed4888f36aada4b"
Refs #28478 -- Deprecated TestCase's allow_database_queries and multi_db
in favor of databases.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:11>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: closed => new
* resolution: fixed =>
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:13>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 1 => 0
* severity: Normal => Release blocker
Comment:
The problem also affects other test apps when run in isolation on
PostgreSQL.
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:14>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* cc: Simon Charette (added)
Comment:
I'll look into it today.
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:15>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:16>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:17>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a02a6fd5805f9f0e613b9951249555876b8c4041" a02a6fd5]:
{{{
#!CommitTicketReference repository=""
revision="a02a6fd5805f9f0e613b9951249555876b8c4041"
Refs #28478 -- Prevented connection creation in model_indexes tests.
Entering a SchemaEditor instance creates a connection but it isn't needed
for this test.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:18>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f5b635086a07c9df5897e69685ebdc3a04049ec6" f5b63508]:
{{{
#!CommitTicketReference repository=""
revision="f5b635086a07c9df5897e69685ebdc3a04049ec6"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:20>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a96b9019320ed8236659ee520a7a017c1bafbc6f" a96b9019]:
{{{
#!CommitTicketReference repository=""
revision="a96b9019320ed8236659ee520a7a017c1bafbc6f"
Refs #28478 -- Prevented timezone assignment for unusable PostgreSQL
connections.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:19>
Django
Reporter: Tim Graham | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"b181aba7dd24c73ec9923c39e35393b0487a5f47" b181aba7]:
{{{
#!CommitTicketReference repository=""
revision="b181aba7dd24c73ec9923c39e35393b0487a5f47"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:21>
Django
Reporter: Tim Graham | Owner: nobody
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:22>
Django
Reporter: Tim Graham | Owner: nobody
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson <carlton.gibson@…>):
In [changeset:"7071f8f2729295b0da77b6c651966dc32c71f1ab" 7071f8f2]:
{{{
#!CommitTicketReference repository=""
revision="7071f8f2729295b0da77b6c651966dc32c71f1ab"
Fixed #30193, Refs #28478 -- Avoided PostgreSQL connection health checks
on initialization.
This addressed a regression introduced by
a96b9019320ed8236659ee520a7a017c1bafbc6f as identified by Ran Benita.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:23>
Django
Reporter: Tim Graham | Owner: nobody
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson <carlton.gibson@…>):
In [changeset:"7f25344c2e69454bb69ec6100a1bc0af6cee6d70" 7f25344c]:
{{{
#!CommitTicketReference repository=""
revision="7f25344c2e69454bb69ec6100a1bc0af6cee6d70"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:24>
Django
Reporter: Tim Graham | Owner: nobody
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"b61ea56789a5825bd2961a335cb82f65e09f1614" b61ea567]:
{{{
#!CommitTicketReference repository=""
revision="b61ea56789a5825bd2961a335cb82f65e09f1614"
Refs #28478 -- Removed support for TestCase's allow_database_queries and
multi_db per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28478#comment:25>