Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#19031 closed New feature (fixed)

Add a warning that @override_settings may not work with DATABASES

Reported by: Jonas H. Owned by: Joeri Bekker
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Joeri Bekker, timograham@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

I'm aware of the fact that SQLite's :memory: mode doesn't work with threads, so I wanted to override TEST_NAME using override_settings to make SQLite use a filesystem DB for a single test:

from django.test import TransactionTestCase
from django.db import models
from django.test.utils import override_settings
from threading import Thread

class OverrideDATABASESTest(TransactionTestCase):
    @override_settings(DATABASES={'default': {'BACKEND': 'django.db.backends.sqlite3', 'TEST_NAME': 'test-db'}})
    def test_override_DATABASES(self):
        t = Thread(target=SomeModel.objects.get)
        t.start()
        t.join()

This doesn't work with threads, it fails with a DatabaseError: no such table: app_modelname exception in the thread. The test works if I set TEST_NAME in my settings.py though, so this seems like an issue with override_settings.

Change History (18)

comment:1 by Anssi Kääriäinen, 11 years ago

Resolution: wontfix
Status: newclosed

I don't think we should claim that you can switch the used database in-flight and expect everything to work. My guess is that the problem here is no syncdb ran for the DB. You could try if things work if you run syncdb manually in setUp().

Wontfixing this, as ensuring that the database is set up correctly, and is also torn down correctly seems hard to do.

comment:2 by Jonas H., 11 years ago

In that case, it might be useful to have some sort of warning or exception if someone tries to override the DATABASE setting. Or a note in the docs -- along with all the other settings that shouldn't/can't be overriden (if any exist).

comment:3 by Claude Paroz, 11 years ago

Component: Testing frameworkDocumentation
Easy pickings: set
Resolution: wontfix
Status: closedreopened
Triage Stage: UnreviewedAccepted

I do agree with Anssi that overriding DATABASE is too much for override_settings. However, I also agree that this could be documented.

comment:4 by Jan Bednařík, 11 years ago

I don't think that overriding DATABASE settings is too much. It depends on what do you expect from it. I can imagine proper use cases for it.

In the docs is described that the test database is created before running tests. And every time you run tests you see in the output: Creating test database... -> runing tests -> Destoying test database... This should be enough to understand that overriding DATABASE will not change test database in the middle of running tests.

Overriding settings is always tricky. Situation described in this ticket is just one case out of many others. I think this particular situation does not need any special documentation.

jonash: Right solution for your problem is skippping the test for SQLite's :memory: database.

comment:5 by Jonas H., 11 years ago

It's not since then I either need to run the tests twice (with :memory: and a real database) or my test is not run at all.

comment:6 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:7 by Joeri Bekker, 11 years ago

Owner: changed from nobody to Joeri Bekker
Status: newassigned

Will write some documentation for this, and also add a warning when using override_settings with these complex settings.

Will also pull in #20075 (cache settings) regarding the above.

comment:8 by Joeri Bekker, 11 years ago

Cc: Joeri Bekker added
Has patch: set

Just added a pull request: https://github.com/django/django/pull/1095

So, override_settings provides a way to change settings in your Django settings. However, some settings are only accessed during the initialization of your project. For example, the DATABASES and CACHES settings are read and cached by Django and Django internals only use these cached objects (and not read your settings file over and over again).

Still, overriding the DATABASE setting with override_settings does change the DATABASE setting when accessed via django.conf.settings. This override however has no effect in practice. I call this unexpected behaviour because you probably expected that your database backend actually changed.

In addition to documenting these settings that show unexpected behaviour, I also added a UserWarning when people try to change these settings. I specifically not raise an exception because you might want to test something not related to what Django does internally (although you probably do).

Note that this also introduces the interesting fact that the Django test suite shows this warning. This actually related to another ticket #20075 where this problem is described.

comment:9 by Aymeric Augustin, 11 years ago

The setting_changed signal is designed to catch such changes and clear whatever caches may exist in Django. I would prefer to raise the warning in a setting_changed listener, for consistency.

comment:10 by Joeri Bekker, 11 years ago

Sounds good. However, this will trigger the warning twice since this signal is also given when leaving the context. Unless there is a way to check if its change/change back signal.

comment:11 by Aymeric Augustin, 11 years ago

As discussed together it would make sense to add a boolean argument to the setting_changed signal to tell entering the block from exiting it.

comment:12 by Joeri Bekker, 11 years ago

Changed pull request as per our discussion.

comment:13 by Tim Graham, 11 years ago

Component: DocumentationTesting framework

Updating component since the patch is more than just documentation.

comment:14 by Tim Graham, 11 years ago

Cc: timograham@… added
Patch needs improvement: set
Summary: Using @override_settings doesn't override DATABASES in threads +SQLiteAdd a warning that @override_settings may not work with DATABASES and CACHES
Type: BugNew feature

Left some comments on the pull request.

comment:15 by Tim Graham, 11 years ago

Was looking into editing the pull request with my comments and noticed this causes the Django test suite to throw a warning because django/contrib/sessions/tests.py calls @override_settings with CACHES. Not sure how we should handle this, since it appears CACHES can apparently be overridden successfully, at least in some cases?

comment:16 by Tim Graham, 11 years ago

Patch needs improvement: unset
Summary: Add a warning that @override_settings may not work with DATABASES and CACHESAdd a warning that @override_settings may not work with DATABASES
Version: 1.4master

On further investigating of #20075, CACHES can be overridden but there are some caveats. I've updated the proposed documentation change with these details and removed CACHES from generating a warning. New PR.

comment:17 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 66f3d57b79eee0381c29ee4c76582d6b182bfad9:

Fixed #19031 -- Added a warning when using override_settings with 'DATABASES'

comment:18 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

In eabc3b6c8dd67e9ff49da9f2f41cc653898cd0a1:

Set stacklevel for the override_settings warning.

Refs #19031.

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