Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#20290 closed Bug (fixed)

override_settings cannot be nested

Reported by: Oliver Beattie Owned by: nobody
Component: Testing framework Version: 1.5
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

Due to to the way override_settings stores the underlying settings object, they can't properly be nested. They grab the value seen at the time they were instantiated, which is not necessarily the same time as when they are used (especially when using class [or function] decorators).

I have made a test-case and patch for this (GitHub pull request)

Change History (9)

comment:2 by Claude Paroz, 11 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Simon Meers, 11 years ago

Thank you for the report and patch, obeattie.

Can you confirm (preferably via an additional test) whether your patch also fixes this issue for instances where the @override_settings decorator is used to decorate a method within a class which is also decorated? i.e.

    @override_settings(foo=1, bar=2)
    class Test(TestCase):
        def test_one(self):
            # assert that setup was correctly performed using foo=1, bar=2

        @override_settings(bar=3)
        def test_two(self):
            # assert that setup was correctly performed using foo=1, bar=3

I ran into this whilst writing tests for #19670, in which STATICFILES_STORAGE was overridden at the class level, and the method-level decorator would produce correct values in settings.STATICFILES_STORAGE but not upstream enough to actually affect the staticfiles_storage object -- see draft snippet from django.test.staticfiles_tests.tests:

@override_settings(**dict(TEST_SETTINGS,
    STATICFILES_STORAGE='django.contrib.staticfiles.storage.CachedStaticFilesStorage',
    DEBUG=False,
))
class TestCollectionCachedStorage(BaseCollectionTestCase,
        BaseStaticFilesTestCase, TestCase):

     # ...

     @override_settings(
         STATICFILES_STORAGE=(
             'staticfiles_tests.storage.MultiCachedStaticFilesStorage'
         )
     )
     def test_multi_extension_patterns(self):
         """ #19670 """
         self.assertEqual(
             settings.STATICFILES_STORAGE,
             'staticfiles_tests.storage.MultiCachedStaticFilesStorage'
         )
         self.assertEqual(
             storage.staticfiles_storage._wrapped.__class__.__name__,
             'MultiCachedStaticFilesStorage'
         )
AssertionError: 'CachedStaticFilesStorage' != u'MultiCachedStaticFilesStorage'

Perhaps I'm asking too much of the decorator, but this feels like it should work to me.

comment:4 by Oliver Beattie, 11 years ago

It appears there are already tests that check this. I have beefed them up slightly from what they were to check that things are applying in the correct precedence order for sure, but they weren't insufficient before.

comment:5 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 552a90b44456f24de228a6c7cd916c040787cf22:

Fixed #20290 -- Allow override_settings to be nested

Refactored override_settings to store the underlying settings._wrapped
value seen at runtime, not instantiation time.

comment:6 by Claude Paroz, 11 years ago

@DrMeers: your issue is related to module objects (caches) not being flushed when your setting is overriden. You probably need to add one more setting_changed signal (see for example https://github.com/django/django/blob/master/django/contrib/auth/hashers.py#L25).

comment:7 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 02619227a9271047c7d4b62a6aec744ca17f7270:

[1.5.x] Fixed #20636 -- Stopped stuffing values in the settings.

In Django < 1.6, override_settings restores the settings module that was
active when the override_settings call was executed, not when it was
run. This can make a difference when override_settings is applied to a
class, since it's executed when the module is imported, not when the
test case is run.

In addition, if the settings module for tests is stored alongside the
tests themselves, importing the settings module can trigger an import
of the tests. Since the settings module isn't fully imported yet,
class-level override_settings statements may store a reference to an
incorrect settings module. Eventually this will result in a crash during
test teardown because the settings module restored by override_settings
won't the one that was active during test setup.

While Django should prevent this situation in the future by failing
loudly in such dubious import sequences, that change won't be backported
to 1.5 and 1.4. However, these versions received the "allowed hosts"
patch and they're prone to "AttributeError: 'Settings' object has no
attribute '_original_allowed_hosts'". To mitigate this regression, this
commits stuffs _original_allowed_hosts on a random module instead of the
settings module.

This problem shouldn't occur in Django 1.6, see #20290, but this patch
will be forward-ported for extra safety.

Also tweaked backup variable names for consistency.

comment:8 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In e3b6fed320f6e67b7e5dc28bab610f342a81f7d7:

[1.4.x] Fixed #20636 -- Stopped stuffing values in the settings.

In Django < 1.6, override_settings restores the settings module that was
active when the override_settings call was executed, not when it was
run. This can make a difference when override_settings is applied to a
class, since it's executed when the module is imported, not when the
test case is run.

In addition, if the settings module for tests is stored alongside the
tests themselves, importing the settings module can trigger an import
of the tests. Since the settings module isn't fully imported yet,
class-level override_settings statements may store a reference to an
incorrect settings module. Eventually this will result in a crash during
test teardown because the settings module restored by override_settings
won't the one that was active during test setup.

While Django should prevent this situation in the future by failing
loudly in such dubious import sequences, that change won't be backported
to 1.5 and 1.4. However, these versions received the "allowed hosts"
patch and they're prone to "AttributeError: 'Settings' object has no
attribute '_original_allowed_hosts'". To mitigate this regression, this
commits stuffs _original_allowed_hosts on a random module instead of the
settings module.

This problem shouldn't occur in Django 1.6, see #20290, but this patch
will be forward-ported for extra safety.

Also tweaked backup variable names for consistency.

Backport of 0261922 from stable/1.5.x.

Conflicts:

django/test/utils.py

comment:9 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 5a6f12182ef14883df6534dc3465059b78f54a1c:

Fixed #20636 -- Stopped stuffing values in the settings.

In Django < 1.6, override_settings restores the settings module that was
active when the override_settings call was executed, not when it was
run. This can make a difference when override_settings is applied to a
class, since it's executed when the module is imported, not when the
test case is run.

In addition, if the settings module for tests is stored alongside the
tests themselves, importing the settings module can trigger an import
of the tests. Since the settings module isn't fully imported yet,
class-level override_settings statements may store a reference to an
incorrect settings module. Eventually this will result in a crash during
test teardown because the settings module restored by override_settings
won't the one that was active during test setup.

While Django should prevent this situation in the future by failing
loudly in such dubious import sequences, that change won't be backported
to 1.5 and 1.4. However, these versions received the "allowed hosts"
patch and they're prone to "AttributeError: 'Settings' object has no
attribute '_original_allowed_hosts'". To mitigate this regression, this
commits stuffs _original_allowed_hosts on a random module instead of the
settings module.

This problem shouldn't occur in Django 1.6, see #20290, but this patch
will be forward-ported for extra safety.

Also tweaked backup variable names for consistency.

Forward port of 0261922 from stable/1.5.x.

Conflicts:

django/test/utils.py

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