Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#19167 closed Uncategorized (fixed)

When running tests, Django accesses the database referred to in settings

Reported by: Daniele Procida Owned by: nobody
Component: Documentation Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description says:

Tests that require a database (namely, model tests) will not use your "real" (production) database. Separate, blank databases are created for the tests.

There seem to be circumstances in which Django will access the real database that is referred to in settings.

Say I have a model called Entity.

I find I can't directly access the real database's Entity objects in tests, and in most of the application code, as expected, Django doesn't find anything in Entity.objects.all(). This is what the documentation tells me to expect.

However, in other parts of the code, Entity.objects.all() does return the Entity objects from the real database, which surprised me. The real database is accessed before Django gets to "Creating test database for alias 'default'..."

If the real database is removed, then things behave as I would expect.

If it is to be expected that Django code when running tests will sometimes access the real database, then the documents need to explain when or how this will happen - but I don't think it should happen.

Change History (15)

comment:1 Changed 6 years ago by Aymeric Augustin

If the real database is accessed before creating the test database, it means that your code runs SQL queries (most likely via ORM callls) at compilation time. Don't do that.

comment:2 in reply to:  1 Changed 6 years ago by Daniele Procida

So for example trying to access objects from the database in a module (and not inside a class or function)? Because that's what in fact I do...

comment:3 Changed 6 years ago by Anssi Kääriäinen

Depends on when the module is imported. Inside class has the same problem. Most likely this would result in accessing the production DB. As aaugustin above said the fix is: don't do this.

There would be certain point in ensuring no queries are even possible through django.db.connections until the test database is set up. The problem here is that to set up the test databases we have to run some queries (CREATE DATABASE for example). We could likely do this by making a local copy of the connections dict first thing in the test runner, then clearing the global connections dictionary. When the test databases are set up, we repopulate the global connections dictionary again.

The question is if we actually want this, and if so, what will break.

comment:4 Changed 6 years ago by Daniele Procida

Maybe I should find a better way to do what I am trying to do, then, that avoids this (if it is even possible, I don't know).

However, I wasn't aware that accessing the DB when modules are imported is a bad idea in general, or that it could interfere with tests.

Is it worth making a note of this on the test documentation, or anywhere else, if it's not already made clear that it's something one should not do?

comment:5 Changed 6 years ago by Aymeric Augustin

The easiest way to work around this problem is cached lazy loading:

_things = None
def get_things():
    if _things is None:
        _things = Things.objects.all()
    return _things

# instead of
# things = Things.objects.all()

I presume that syncdb fails on your current code, because it will attempt to load Entity objects before the corresponding table is created in the database. This is the most obvious reason why I said "don't do that".

Right now I don't know how Django could forbid this. It's a fairly common pitfall.

Last edited 6 years ago by Aymeric Augustin (previous) (diff)

comment:6 Changed 6 years ago by Anssi Kääriäinen

I am not sure if this is documented or not. If not a docs note would be welcome (likely recommending something like what was done in comment:5).

I am pretty sure we could prevent this in tests using the idea in comment:3. Assuming users actually write tests, then import time queries would be spotted...

Another option is to have prevent_normal_queries() and enable_normal_queries() available. These could be called in various places, for example app-loading, test setup, syncdb etc. Then we would still need some way to bypass this prevention. If prevent_normal_queries just appends __hidden__ to each alias in connections, then we could still run queries by .using('__hidden__' + alias) when needed.

I am not at all sure this is worth all the trouble... And, I am pretty sure some user code would break.

comment:7 Changed 6 years ago by Daniele Procida

I will write a couple of notes for the documentation then, warning that real database items may leak into tests if the code accesses the database at compilation time.

I have not seen advice not to do that in the documentation previously, but I am not even sure where it might say such a thing or what terms it would use.

I'll also add aaugustin's workaround in comment:5.

Thanks for your feedback.

comment:8 Changed 6 years ago by Preston Holmes

Component: UncategorizedDocumentation
Triage Stage: UnreviewedAccepted

A suggested location for the doc improvements would be:

comment:9 Changed 6 years ago by Anssi Kääriäinen

Here is a proof-of-concept for disabling queries during import time: - I quickly tested this on different databases using a test project which does import time "SomeModel.objects.get(pk=1)" query. Seemed to work. And, Django's test suite didn't complain on sqlite either.

IMO we should do the disabling of queries. This will only hurt users who do import time queries when running tests. They will benefit from fixing their code. If this change isn't possible for them, they can just do _enable_connections().

The implementation only ensures that the main thread can't run queries (django.db.connections is thread-local). I wonder if there is any point in making sure all threads are prevented from running queries...

I don't see any easy way to test this.

comment:10 Changed 6 years ago by Daniele Procida for the documentation; once I work out how to make a pull request containing this and not two other files that I came from I don't-know-where, I will do that.

comment:11 Changed 6 years ago by Daniele Procida

I've made a pull request for the documentation:

comment:12 Changed 6 years ago by Daniele Procida

Please ignore the previous pull request, here's another attempt:

comment:13 Changed 6 years ago by Tim Graham

Has patch: set

comment:14 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 07361d1fd6b4531e422e2593c91b47bc6bf88993:

Fixed #19167 - Added a warning regarding module-level database queries

Thanks Daniele Procida for the patch.

comment:15 Changed 6 years ago by Tim Graham <timograham@…>

In 90af863410cae9d043f5378a7cfdab92a696b562:

[1.5.X] Fixed #19167 - Added a warning regarding module-level database queries

Thanks Daniele Procida for the patch.

Backport of 07361d1fd6 from master

Note: See TracTickets for help on using tickets.
Back to Top