ThreadTests fails due to double test_ prefix caused by TestDbCreationTests.

[Matthijs Kooijman]

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

See ​https://github.com/django/django/blob/cebd41e41603c3ca77c5b29d6cd20c1bff43827f/django/db/backends/base/creation.py#L33

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

See ​https://github.com/django/django/blob/5e00bd1f7717149573df9607b848297a520881d3/tests/backends/base/test_creation.py#L58

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.

See ​https://github.com/django/django/blob/5e00bd1f7717149573df9607b848297a520881d3/tests/backends/tests.py#L634

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.

[Mariusz Felisiak]

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.

[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.

[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.

[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.

[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.

[Mariusz Felisiak]

​PR

[GitHub <noreply@…>]

In c159bace:

Refs #31117 -- Isolated backends.sqlite.test_creation.TestDbSignatureTests.

[Matthijs Kooijman]

I completely rewrote the patch, as suggested by Felix, and it should be ready for review now.

[Mariusz Felisiak <felisiak.mariusz@…>]

In f34be52:

Refs #31117 -- Moved get_connection_copy() test hook to a module level.

[Mariusz Felisiak <felisiak.mariusz@…>]

In b64b1b2e:

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.databasesdefault, 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.databasesdefaultNAME 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().

[Mariusz Felisiak <felisiak.mariusz@…>]

In 2a2ea4ee:

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.