Code

Opened 15 months ago

Closed 13 months ago

Last modified 13 months ago

#20290 closed Bug (fixed)

override_settings cannot be nested

Reported by: obeattie 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)

Attachments (0)

Change History (9)

comment:1 Changed 15 months ago by obeattie

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

comment:2 Changed 15 months ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 14 months ago by DrMeers

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 Changed 14 months ago by obeattie

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 Changed 13 months ago by Claude Paroz <claude@…>

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

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 Changed 13 months ago by claudep

@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 Changed 13 months ago by Aymeric Augustin <aymeric.augustin@…>

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 Changed 13 months ago by Aymeric Augustin <aymeric.augustin@…>

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 Changed 13 months ago by Aymeric Augustin <aymeric.augustin@…>

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

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.