Code

Opened 21 months ago

Closed 21 months ago

Last modified 21 months ago

#19167 closed Uncategorized (fixed)

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

Reported by: EvilDMP 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

https://docs.djangoproject.com/en/dev/topics/testing/#the-test-database 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.

Attachments (0)

Change History (15)

comment:1 follow-up: Changed 21 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 21 months ago by EvilDMP

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 21 months ago by akaariai

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 21 months ago by EvilDMP

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 21 months ago by aaugustin

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 21 months ago by aaugustin (previous) (diff)

comment:6 Changed 21 months ago by akaariai

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 21 months ago by EvilDMP

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 21 months ago by ptone

  • Component changed from Uncategorized to Documentation
  • Triage Stage changed from Unreviewed to Accepted

A suggested location for the doc improvements would be:

https://docs.djangoproject.com/en/dev/topics/testing/#the-test-database

comment:9 Changed 21 months ago by akaariai

Here is a proof-of-concept for disabling queries during import time: https://github.com/akaariai/django/commit/1b99fcee4c0b3782d3136744cfaaccfea1d535e0 - 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 21 months ago by EvilDMP

https://github.com/evildmp/django/commit/f0b5d1b91698452a468fd6a6af63c5b7a7d95405 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 21 months ago by EvilDMP

I've made a pull request for the documentation: https://github.com/django/django/pull/463

comment:12 Changed 21 months ago by EvilDMP

Please ignore the previous pull request, here's another attempt: https://github.com/django/django/pull/486

comment:13 Changed 21 months ago by timo

  • Has patch set

comment:14 Changed 21 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 07361d1fd6b4531e422e2593c91b47bc6bf88993:

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

Thanks Daniele Procida for the patch.

comment:15 Changed 21 months 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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.