[Django] #31117: ThreadTests fails due to double test_ prefix caused by TestDbCreationTests
Django
---------------------------------------------+------------------------
Reporter: Matthijs Kooijman | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
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 working on testcases for #26552 and #31051 I ran into some problems
with
testing against a local MySQL and Postgres server. I initially thought my
testcases broke things, but it turns out I can see the same broken
behaviour on
a clean master. Hence this ticket.
In short, what happens is that
`backends.base.test_creation.TestDbCreationTests` runs `create_test_db` to
verify migration is (not) run depending on the `MIGRATE` settings.
However,
this has two problems:
- This runs `create_test_db` on an already initialized database, leading
to
double initialization (in particular, it adds a *second* `test_` prefix
to the
database name, which produces an unexpected database name.
- `create_test_db` has side effects that are not properly cleaned up.
Some of them
are reverted by restoring `settings_dict` (see below), but not all of
them
(more on this below).
=== Possible solution
A solution to both issues could be to use a separate database, that is not
normally used by other tests and thus not initialized by the test runner.
This
test can then freely call `create_test_db` to initialize it (possible even
actually creating the database), and then call `destroy_test_db` to clean
up
everthing again.
I have alredy been working on adding an extra database like this, since
this
was also a possible solution to problems I had with
https://github.com/django/django/pull/12166
=== To reproduce
On my system, the partial reverting leads to an exception in a later test
due
to a fairly obscure interaction between various parts (details below).
To reproduce this exception, I've used the commands below. Note that the
`multiple_databases.tests` are not related to this bug directly, but used
to
work around #31055.
{{{
$ ./runtests.py --settings test_postgresql multiple_database.tests
backends.base.test_creation.TestDbCreationTests backends.tests.ThreadTests
--parallel 1
File "/home/matthijs/docs/src/upstream/django/venv/lib/python3.7/site-
packages/psycopg2/__init__.py", line 126, in connect
conn = _connect(dsn, connection_factory=connection_factory, **kwasync)
psycopg2.OperationalError: FATAL: database "test_test_django_main" does
not exist
$ ./runtests.py --settings test_mysql multiple_database.tests
backends.base.test_creation.TestDbCreationTests backends.tests.ThreadTests
--parallel 1
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 'test_test_django_main'")
}}}
Interestingly enough, this happens for me locally, but not with the
automatic
testing on Jenkins. I originally suspected that this was because my MySQL
permissions were set up strictly (only allowing access to
`test_django_*`), but
for postgresql, the error was not about permissions. Also, when I relaxed
the
MySQL permissions, the error only changed to `(1049, "Unknown database
'test_test_django_main'")`.
Is the configuration for the Jenkins tests published somewhere? Maybe that
would offer a clue about this difference.
=== Analysis
I dug a little deeper to figure out why this exception occurs exactly.
This is what happens:
First, `create_test_db` prepends `test_` to the database name, and
configures this in two places:
{{{
settings.DATABASES[self.connection.alias]["NAME"] = test_database_name
self.connection.settings_dict["NAME"] = test_database_name
}}}
Since the test runner has previously called `create_test_db`, the database
name now has a `test_test_` prefix.
At this time, self.connection.settings_dict is the same dict as
`django.db.connections.databases`, so that one is also changed.
At the end of the test, `connection.settings_dict` is restored to a copy
made before the test. This replaces the entire dict instead of modifying
the
dict, so `django.db.connections.databases` is *not* restored and still has
the `test_test_` prefix.
{{{
connection.settings_dict = saved_settings
}}}
Finally, `backends.tests.ThreadTests` creates a new connection object
by calling `connection.cursor()` from within another thread. This new
connection object is initialized using
`django.db.connections.databases['default']`, which has the `test_test_`
prefix.
=== Next steps
Since I was already working on allowing a third database, I'll see if I
can fix
this issue as well, probably as a separate pullrequest. Any thoughts on
whether
this is the right approach are welcome, as well as thoughts about why this
does
not occur on Jenkins.
--
Ticket URL: <https://code.djangoproject.com/ticket/31117>
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: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Unreviewed => Accepted
* component: Uncategorized => Testing framework
Comment:
> Is the configuration for the Jenkins tests published somewhere? Maybe
that would offer a clue about this difference.
No, but it doesn't contain anything unusual. Jenkins runs the entire test
suite without a `parallel` flag, that's why it works. For example
{{{
> ./runtests.py multiple_database.tests backends.base backends.tests
}}}
works without any issues.
--
Ticket URL: <https://code.djangoproject.com/ticket/31117#comment:1>
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 Matthijs Kooijman):
`./runtests.py multiple_database.tests backends.base backends.tests`
works without any issues.
Oh, interesting. Parallelization might indeed be related (I can imagine
that when parallelization causes the two problematic tests to be ran in
different threads and/or different order, this might not break).
However, something else seems to be going on (as well maybe). The command
you gave indeed works, but when I add `--parallel 1` (which would ensure
the problematic tests are ran in the problematic order), it still works:
{{{
./runtests.py multiple_database.tests backends.base backends.tests
--parallel 1
}}}
Maybe there is some tests that, when ran in between, prevents this problem
from occuring? OTOH, when I run the *entire* test suite, with
parallelization, IIRC the problem *also* occurs.
Regardless, I believe my analysis and proposed solution still hold. I just
tried the fix and it seems to work, so pullrequest coming up.
--
Ticket URL: <https://code.djangoproject.com/ticket/31117#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 Matthijs Kooijman):
Oh, never mind the last part of my previous comment. Maybe I failed to
stress that this only fails with MySQL and Postgres, while SQLite is
unaffected (because it uses no NAME for the in-memory database used by
default). So running with mysql and `--parallel 1` does indeed fail:
{{{
./runtests.py multiple_database.tests backends.base backends.tests
--parallel 1
}}}
But removing `-parallel makes things work again, so there is likely some
ordering difference there.
--
Ticket URL: <https://code.djangoproject.com/ticket/31117#comment:3>
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 Matthijs Kooijman):
Turns out the problem with restoration of settings also exists in
`backends.sqlite.test_creation`. This was not previously a problem because
`backends.base.test_creation.TestDbCreationTests` would sever the
reference between `connection.settings_dict` and
`connections.databases['default']`, but with that fixed, this problem is
not exposed in the sqlite tests. I've added a fix for this in the PR as
well.
No, but it doesn't contain anything unusual. Jenkins runs the entire
test suite without a parallel flag, that's why it works. For example
This does not seem true after all. Further investigation (using some dummy
commits to trigger Jenkins builds with extra debug output) shows that
Jenkins puts `DJANGO_TEST_PROCESSES=1` in the environment, which limits
the build to a single process, so that cannot be the culprit here. I've
been doing some experiments in https://github.com/django/django/pull/12248
to figure out why Jenkins does not have this problem, but I'm having some
problems getting the right debug output from Jenkins. I'll update here
when I figure out something definitive.
--
Ticket URL: <https://code.djangoproject.com/ticket/31117#comment:4>
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 Matthijs Kooijman):
I highly suspect that the reason Jenkins does not show this problem, is
because it's database config specifies `['TEST']['NAME']` rather than just
`['NAME']`. While the former has `test_` prefixed, the latter is used as-
is, so re-running the database does not actually change the database name,
so this problem does not surface. I have not been able to verify this
completely yet (since the PR I was using for testing was closed), but I'm
pretty confident this was the case.
In any case, the PR can now again run the testsuite completely
succesfully. Only things open are:
- How to prevent concurrency issues accessing this new database when
multiple testcases use it? Normally, this is handled by making the
testrunner make clones of the database, but I can't see how that would
work here. Any suggestions?
- What name to use for this new database? 'unused' does not seem too
great.
- The Jenkins database configuration needs to be changed to configure
this new database. Is there anything I can do for that?
- https://github.com/django/django/pull/12201 should be merged first.
--
Ticket URL: <https://code.djangoproject.com/ticket/31117#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
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):
* status: new => assigned
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* owner: nobody => Matthijs Kooijman
Comment:
[https://github.com/django/django/pull/12247 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/31117#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
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
Comment (by GitHub <noreply@…>):
In [changeset:"c159bacebaa256a7d171d00a3eb14c4ef8357f7c" c159bace]:
{{{
#!CommitTicketReference repository=""
revision="c159bacebaa256a7d171d00a3eb14c4ef8357f7c"
Refs #31117 -- Isolated
backends.sqlite.test_creation.TestDbSignatureTests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31117#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
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 Matthijs Kooijman):
* needs_better_patch: 1 => 0
Comment:
I completely rewrote the patch, as suggested by Felix, and it should be
ready for review now.
--
Ticket URL: <https://code.djangoproject.com/ticket/31117#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
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 Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"f34be5294d8bd9530079525fb56e661816a63e20" f34be52]:
{{{
#!CommitTicketReference repository=""
revision="f34be5294d8bd9530079525fb56e661816a63e20"
Refs #31117 -- Moved get_connection_copy() test hook to a module level.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31117#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
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 Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"b64b1b2e1a17da3fd702cde4bfc2b6e0689b4348" b64b1b2e]:
{{{
#!CommitTicketReference repository=""
revision="b64b1b2e1a17da3fd702cde4bfc2b6e0689b4348"
Fixed #31117 -- Isolated backends.base.test_creation.TestDbCreationTests.
Previously, this test could modify global state by changing
connection.settings_dict. This dict is a reference to the same dict as
django.db.connections.databases['default'], which is thus also changed.
The cleanup of this test would replace connection.settings_dic` with a
saved copy, which would leave the dict itself modified.
Additionally, create_test_db() would also modify these same dicts, as
well as settings.databases['default']['NAME'] by adding a "test_"
prefix, which is what can cause problems later.
This patch:
- makes a complete copy of the connection and work on that, to improve
isolation.
- calls destroy_test_db() to let that code clean up anything done by
create_test_db().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31117#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Matthijs Kooijman | Owner: Matthijs
| Kooijman
Type: Bug | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"2a2ea4ee18fdcf2c95bf6435bc63b74623e3085b" 2a2ea4ee]:
{{{
#!CommitTicketReference repository=""
revision="2a2ea4ee18fdcf2c95bf6435bc63b74623e3085b"
Refs #31117 -- Made various tests properly handle unexpected databases
aliases.
- Used selected "databases" instead of django.db.connections.
- Made routers in tests.migrations skip migrations on unexpected
databases.
- Added DiscoverRunnerGetDatabasesTests.assertSkippedDatabases() hook
which properly asserts messages about skipped databases.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31117#comment:11>