[Django] #31055: Omits test_ prefix from database name when running subset of tests
Django
---------------------------------------------+------------------------
Reporter: Matthijs Kooijman | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 3.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
While debugging some test framework issues wrt mysql, I noticed a problem
where the test runner would try to access the test database without
prefixing `test_`, leading to an access denied error (because my
permissions are set up tightly).
What I suspect happens is that this subset of tests only uses the
`default` database, so only that one is set up by
`DisoveryRunner.setup_databases`. This is confirmed by using a debugger,
which shows
[[https://github.com/django/django/blob/845042b3d9faaefef8855c2bab48bd9532cd00ca/django/test/runner.py#L683|databases]]
only contains 'default'. Then, it runs the `check` management command,
which looks at `settings.DATABASES`, which still contains the settings for
`default` and `other`. This in turn causes a connection to the `other`
database to be made, but since the name of that database is not modified
by
[[https://github.com/django/django/blob/845042b3d9faaefef8855c2bab48bd9532cd00ca/django/db/backends/base/creation.py#L61|create_test_db]],
that still refers to the original name, and the connection fails.
To reproduce, I have a clean master
(c33eb6dcd0c211f8f02b2976fe3b3463f0a54498), with the following
`tests/test_mysql.py`:
{{{
DATABASES = {
'default': {
'ENGINE': 'django.db.backends.mysql',
'HOST': 'localhost',
'USER': 'test_django',
'PASSWORD': 'XXX',
# Django prepends test_ to this name...
'NAME': 'django_main',
},
'other': {
'ENGINE': 'django.db.backends.mysql',
'HOST': 'localhost',
'USER': 'test_django',
'PASSWORD': 'XXX',
# Django prepends test_ to this name...
'NAME': 'django_other',
}
}
SECRET_KEY = "django_tests_secret_key"
# Use a fast hasher to speed up tests.
PASSWORD_HASHERS = [
'django.contrib.auth.hashers.MD5PasswordHasher',
]
}}}
Then inside `tests`, I run:
./runtests.py --settings test_mysql --parallel 1 timezones
I think the `--parallel 1` is not strictly needed, but might make things
easier to debug. With the above, I get:
{{{
Creating test database for alias 'default'...
Destroying test database for alias 'default'...
Testing against Django installed in
'/home/matthijs/docs/src/upstream/django/django'
Traceback (most recent call last):
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/base/base.py",
line 220, in ensure_connection
self.connect()
File "/home/matthijs/docs/src/upstream/django/django/utils/asyncio.py",
line 24, in inner
return func(*args, **kwargs)
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/base/base.py",
line 197, in connect
self.connection = self.get_new_connection(conn_params)
File "/home/matthijs/docs/src/upstream/django/django/utils/asyncio.py",
line 24, in inner
return func(*args, **kwargs)
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/mysql/base.py",
line 233, in get_new_connection
return Database.connect(**conn_params)
File "/home/matthijs/docs/src/upstream/django/venv/lib/python3.7/site-
packages/MySQLdb/__init__.py", line 84, in Connect
return Connection(*args, **kwargs)
File "/home/matthijs/docs/src/upstream/django/venv/lib/python3.7/site-
packages/MySQLdb/connections.py", line 179, in __init__
super(Connection, self).__init__(*args, **kwargs2)
MySQLdb._exceptions.OperationalError: (1044, "Access denied for user
'test_django'@'localhost' to database 'django_other'")
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "./runtests.py", line 566, in <module>
options.start_at, options.start_after, options.pdb,
File "./runtests.py", line 308, in django_tests
extra_tests=extra_tests,
File "/home/matthijs/docs/src/upstream/django/django/test/runner.py",
line 687, in run_tests
self.run_checks()
File "/home/matthijs/docs/src/upstream/django/django/test/runner.py",
line 625, in run_checks
call_command('check', verbosity=self.verbosity)
File
"/home/matthijs/docs/src/upstream/django/django/core/management/__init__.py",
line 168, in call_command
return command.execute(*args, **defaults)
File
"/home/matthijs/docs/src/upstream/django/django/core/management/base.py",
line 369, in execute
output = self.handle(*args, **options)
File
"/home/matthijs/docs/src/upstream/django/django/core/management/commands/check.py",
line 64, in handle
fail_level=getattr(checks, options['fail_level']),
File
"/home/matthijs/docs/src/upstream/django/django/core/management/base.py",
line 395, in check
include_deployment_checks=include_deployment_checks,
File
"/home/matthijs/docs/src/upstream/django/django/core/management/base.py",
line 382, in _run_checks
return checks.run_checks(**kwargs)
File
"/home/matthijs/docs/src/upstream/django/django/core/checks/registry.py",
line 72, in run_checks
new_errors = check(app_configs=app_configs)
File
"/home/matthijs/docs/src/upstream/django/django/core/checks/model_checks.py",
line 34, in check_all_models
errors.extend(model.check(**kwargs))
File "/home/matthijs/docs/src/upstream/django/django/db/models/base.py",
line 1276, in check
*cls._check_constraints(),
File "/home/matthijs/docs/src/upstream/django/django/db/models/base.py",
line 1842, in _check_constraints
connection.features.supports_table_check_constraints or
File
"/home/matthijs/docs/src/upstream/django/django/utils/functional.py", line
48, in __get__
res = instance.__dict__[self.name] = self.func(instance)
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/mysql/features.py",
line 97, in supports_column_check_constraints
if self.connection.mysql_is_mariadb:
File
"/home/matthijs/docs/src/upstream/django/django/utils/functional.py", line
48, in __get__
res = instance.__dict__[self.name] = self.func(instance)
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/mysql/base.py",
line 364, in mysql_is_mariadb
return 'mariadb' in self.mysql_server_info.lower()
File
"/home/matthijs/docs/src/upstream/django/django/utils/functional.py", line
48, in __get__
res = instance.__dict__[self.name] = self.func(instance)
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/mysql/base.py",
line 351, in mysql_server_info
with self.temporary_connection() as cursor:
File "/usr/lib/python3.7/contextlib.py", line 112, in __enter__
return next(self.gen)
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/base/base.py",
line 604, in temporary_connection
with self.cursor() as cursor:
File "/home/matthijs/docs/src/upstream/django/django/utils/asyncio.py",
line 24, in inner
return func(*args, **kwargs)
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/base/base.py",
line 260, in cursor
return self._cursor()
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/base/base.py",
line 236, in _cursor
self.ensure_connection()
File "/home/matthijs/docs/src/upstream/django/django/utils/asyncio.py",
line 24, in inner
return func(*args, **kwargs)
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/base/base.py",
line 220, in ensure_connection
self.connect()
File "/home/matthijs/docs/src/upstream/django/django/db/utils.py", line
90, in __exit__
raise dj_exc_value.with_traceback(traceback) from exc_value
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/base/base.py",
line 220, in ensure_connection
self.connect()
File "/home/matthijs/docs/src/upstream/django/django/utils/asyncio.py",
line 24, in inner
return func(*args, **kwargs)
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/base/base.py",
line 197, in connect
self.connection = self.get_new_connection(conn_params)
File "/home/matthijs/docs/src/upstream/django/django/utils/asyncio.py",
line 24, in inner
return func(*args, **kwargs)
File
"/home/matthijs/docs/src/upstream/django/django/db/backends/mysql/base.py",
line 233, in get_new_connection
return Database.connect(**conn_params)
File "/home/matthijs/docs/src/upstream/django/venv/lib/python3.7/site-
packages/MySQLdb/__init__.py", line 84, in Connect
return Connection(*args, **kwargs)
File "/home/matthijs/docs/src/upstream/django/venv/lib/python3.7/site-
packages/MySQLdb/connections.py", line 179, in __init__
super(Connection, self).__init__(*args, **kwargs2)
django.db.utils.OperationalError: (1044, "Access denied for user
'test_django'@'localhost' to database 'django_other'")
}}}
I am not quite familiar with this code, and this is already a distraction
from a distraction from a distraction from the actual project I was
working on, so I'm going to leave this here for others to fix :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/31055>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Matthijs Kooijman | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Matthijs Kooijman):
* cc: Matthijs Kooijman (added)
* version: 3.0 => master
Old description:
New description:
SECRET_KEY = "django_tests_secret_key"
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:1>
Django
Reporter: Matthijs Kooijman | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Unreviewed => Accepted
Comment:
OK, wowser, yes. Good one.
Running all the tests we create both DBs:
{{{
$ ./runtests.py --parallel=1
Testing against Django installed in '.../django'
Creating test database for alias 'default'...
Creating test database for alias 'other'...
System check identified no issues (14 silenced).
}}}
vs
{{{
$ ./runtests.py --parallel=1 timezones
Testing against Django installed in '.../django'
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
}}}
How best we should disable the `other` alias in this circumstance is a
good question, but presumably this should work.
Thanks for the report.
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:2>
Django
Reporter: Matthijs Kooijman | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
This is likely related to #28478.
I guess `DiscoverRunner` should make sure only checks against the tested
databases are run when it calls `run_checks`
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:3>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* status: new => assigned
* owner: nobody => Simon Charette
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:4>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthijs Kooijman):
Simon, you set "has patch", but I cannot see any patch. Was that
unintentional?
As for the fix, I wonder if maybe the `connections` object should just be
trimmed to only contain the used databases, so all other code that
iterates the databases will only see the ones actually in use? I believe
there is code in multiple places now that has special casing for when a
subset of databases is in use to not use all of the `connections` but only
some, that could maybe also be simplified. On the other hand, this seems
like an obvious implementation for using a subset of databases, so I guess
there is a reason this implementation is not used right now?
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:5>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Matthijs, you can see links to Github PRs in the ticket description at the
top.
https://github.com/django/django/pull/12201
> As for the fix, I wonder if maybe the connections object should just be
trimmed to only contain the used databases, so all other code that
iterates the databases will only see the ones actually in use
You'd have to give it a shot to make sure but I'm afraid it might break
backward compatibility in subtle ways.
To me this issue is really similar to the ones discovered during #26351
where checks that require database accesses should be special cased.
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:6>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthijs Kooijman):
> Matthijs, you can see links to Github PRs in the ticket description at
the top.
Ah, I see. I quickly looked through those, but thought that if the PR link
was added as a property there, it should also show up in the activity log
below. But I suspect this PR link is added automatically based on the PR
description or something like that, so it does not show up in the log. Oh
well, found it now, thanks!
I had a quick look at the PR, but I do not think I have enough experience
with this code to add anything useful there (and it looks like others are
chipping in just fine). In any case, thanks for picking this up :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:7>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* needs_better_patch: 0 => 1
Comment:
Marking as ''"needs improvement"'' due to
[https://github.com/django/django/pull/12201#issuecomment-567217130
Simon's comment].
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:8>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* needs_better_patch: 1 => 0
Comment:
https://github.com/django/django/pull/12396
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:9>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:10>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0b83c8cc4db95812f1e15ca19d78614e94cf38dd" 0b83c8c]:
{{{
#!CommitTicketReference repository=""
revision="0b83c8cc4db95812f1e15ca19d78614e94cf38dd"
Refs #31055 -- Added --database option to the check management command.
This avoids enabling the ``database`` checks unless they are explicitly
requested and allows to disable on a per-alias basis which is required
when only creating a subset of the test databases.
This also removes unnecessary BaseCommand._run_checks() hook.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:11>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"430e796980799feb75901b9918245b6b0bff3f54" 430e796]:
{{{
#!CommitTicketReference repository=""
revision="430e796980799feb75901b9918245b6b0bff3f54"
Refs #31055 -- Made DiscoverRunner skip running system checks on unused
test databases.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:12>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Component: Testing framework | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"71756bdfed5029bd14dce8cb3c5629efc4be55ac" 71756bdf]:
{{{
#!CommitTicketReference repository=""
revision="71756bdfed5029bd14dce8cb3c5629efc4be55ac"
Fixed #31055 -- Made constraint checks support databases aware.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:13>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"708c534e0badd5659df2c66d6934a34b3cc79e5b" 708c534]:
{{{
#!CommitTicketReference repository=""
revision="708c534e0badd5659df2c66d6934a34b3cc79e5b"
Refs #31055 -- Fixed Model.check() call in
ConstraintsTests.test_check_constraints_required_db_features().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:14>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"5c8441a0b8787c14b45fb761550571baea48604e" 5c8441a]:
{{{
#!CommitTicketReference repository=""
revision="5c8441a0b8787c14b45fb761550571baea48604e"
Refs #31055 -- Made long column names checks support databases aware.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:15>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"e8d308892597e6abee5b94a77543a7ef7d34a655" e8d3088]:
{{{
#!CommitTicketReference repository=""
revision="e8d308892597e6abee5b94a77543a7ef7d34a655"
Refs #31055 -- Allowed database queries in
invalid_models_tests.test_models.FieldNamesTests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:16>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"c8d3cbdba809285ced3d9ba3cbb63bec422b8e48" c8d3cbdb]:
{{{
#!CommitTicketReference repository=""
revision="c8d3cbdba809285ced3d9ba3cbb63bec422b8e48"
Refs #31055 -- Doc'd 'databases' argument of check functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:17>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Testing framework | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"54684a3ec03725f605461f4460da550550e34a00" 54684a3]:
{{{
#!CommitTicketReference repository=""
revision="54684a3ec03725f605461f4460da550550e34a00"
[3.2.x] Refs #31055 -- Doc'd 'databases' argument of check functions.
Backport of c8d3cbdba809285ced3d9ba3cbb63bec422b8e48 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:18>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Testing framework | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
{{{#!CommitTicketReference repository=""
revision="1b539ef27e776bd6112e8f22e653feca5279c4fb"
Refs #31055 -- Augmented regression tests for database system checks.
We might want to change this in the future but it should be further
discussed first.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:19>
Django
Reporter: Matthijs Kooijman | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Testing framework | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
{{{#!CommitTicketReference repository=""
revision="96ee097bd68a93e0f360326271ebe78553fae15b"
Refs #31055 -- Adjusted passing of Field.check kwargs to
ArrayField.base_field.
This was missed when Field.check(databases) was introduced.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31055#comment:20>