Opened 3 years ago

Closed 23 months ago

Last modified 17 months ago

#19031 closed New feature (fixed)

Add a warning that @override_settings may not work with DATABASES

Reported by: jonash Owned by: joeri
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: joeri, 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 Changed 3 years ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to closed

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 Changed 3 years ago by jonash

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 Changed 3 years ago by claudep

  • Component changed from Testing framework to Documentation
  • Easy pickings set
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 3 years ago by Architekt

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 Changed 3 years ago by jonash

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 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:7 Changed 2 years ago by joeri

  • Owner changed from nobody to joeri
  • Status changed from new to assigned

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 Changed 2 years ago by joeri

  • Cc joeri 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 Changed 2 years ago by aaugustin

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 Changed 2 years ago by joeri

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 Changed 2 years ago by aaugustin

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 Changed 2 years ago by joeri

Changed pull request as per our discussion.

comment:13 Changed 2 years ago by timo

  • Component changed from Documentation to Testing framework

Updating component since the patch is more than just documentation.

comment:14 Changed 2 years ago by timo

  • Cc timograham@… added
  • Patch needs improvement set
  • Summary changed from Using @override_settings doesn't override DATABASES in threads +SQLite to Add a warning that @override_settings may not work with DATABASES and CACHES
  • Type changed from Bug to New feature

Left some comments on the pull request.

comment:15 Changed 2 years ago by timo

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 Changed 23 months ago by timo

  • Patch needs improvement unset
  • Summary changed from Add a warning that @override_settings may not work with DATABASES and CACHES to Add a warning that @override_settings may not work with DATABASES
  • Version changed from 1.4 to master

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 Changed 23 months ago by Tim Graham <timograham@…>

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

In 66f3d57b79eee0381c29ee4c76582d6b182bfad9:

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

comment:18 Changed 17 months ago by Aymeric Augustin <aymeric.augustin@…>

In eabc3b6c8dd67e9ff49da9f2f41cc653898cd0a1:

Set stacklevel for the override_settings warning.

Refs #19031.

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