Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#21518 closed Bug (fixed)

override_settings(ROOT_URLCONF) doesn't clear resolver cache

Reported by: Chris Wilson Owned by: Chris Wilson
Component: Core (URLs) Version: 1.6
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

If override_settings is used to set different URL mappings on different tests, by setting ROOT_URLCONF to different values, it doesn't work properly because the resolver cache is never cleared.

You might argue that this behaviour is unnecessary or broken, but Django-CMS for example recommends that you do:

Your apps need testing, but in your live site they aren’t in urls.py as they are attached to a CMS page. So if you want to be able to use reverse() in your tests, or test templates that use the url template tag, you need to hook up your app to a special test version of urls.py and tell your tests to use that... in your tests you can plug this in with the override_settings() decorator.

A simple test case that fails:

class FirstUrls:
    urlpatterns = patterns('', url(r'first/$', fake_view, name='first'))

class SecondUrls:
    urlpatterns = patterns('', url(r'second/$', fake_view, name='second'))

class OverrideSettingsTests(TestCase):
    """
    If neither override_settings nor a settings_changed receiver clears the
    URL cache between tests, then one of these two test methods will fail.
    """

    @override_settings(ROOT_URLCONF=FirstUrls)
    def test_first(self):
        reverse('first')

    @override_settings(ROOT_URLCONF=SecondUrls)
    def test_second(self):
        reverse('second')

In order for this to work, I think the best approach is to add another listener for the settings_changed signal in django/test/signals.py, which detects when the ROOT_URLCONF setting is changed and clears the LRU caches in django.core.urlresolvers.

Change History (10)

comment:1 Changed 3 years ago by Chris Wilson

Type: UncategorizedBug

comment:2 Changed 3 years ago by Aymeric Augustin

Easy pickings: unset
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

The idea is sane, see comment on the PR.

comment:3 Changed 3 years ago by Chris Wilson

Patch needs improvement: unset

Modified the PR branch as requested by @aaugustin.

comment:4 Changed 3 years ago by Claude Paroz

The patch looks good, however the preferred method to override the URLConf is https://docs.djangoproject.com/en/dev/topics/testing/overview/#urlconf-configuration
Yes, that means creating a different TestCase class for each different URLConf to be tested, but it's probably not very common to have to test different URLConfs.

comment:5 Changed 3 years ago by Chris Wilson

In that case, can we make it throw an error if someone tries to override ROOT_URLCONF and point them to the right part of the documentation?

Anyway, I can see cases where it might be useful, for example if you want to isolate your tests from the rest of the environment by defining only the routes that they should depend on. Or changing routes within a single test. Having to create a subclass for each test feels a bit heavyweight.

I could also propose a different test case, that modifies ROOT_URLCONF at runtime. Django-CMS does this when you attach a Django application (with its own routes) to a CMS page. If Django is going to cache things (which is undocumented) then I think it ought to Do The Right Thing, realise when its cache is invalid and update it.

comment:6 Changed 3 years ago by Aymeric Augustin

Clearly we'd better provide just "one way to do it" in the long term. We could consider the "urls" attribute as an old and specific way to override a particular setting. That would give us a reason deprecate it in favor of the standard settings override mechanisms, @override_settings(...) and with self.settings(...):.

Claude, can we simply fix this ticket and start a new one to discuss the more general problem and long term solutions?

comment:7 Changed 3 years ago by Marc Tamlyn

If we can move towards making more settings overridable with override_settings I'd consider that a good thing.

comment:8 Changed 3 years ago by Claude Paroz

The plan looks fine for me.

comment:9 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 65131911dba08dcc1451d71ae4d5101724d722f6:

Fixed #21518 -- Made override_settings(ROOT_URLCONF) clear the resolver cache.

Thanks Aymeric Augustin and Simon Charette for reviews.

comment:10 Changed 3 years ago by Tim Graham

I created a follow-up ticket: "#21977 - Deprecate SimpleTestCase.urls in favor of override_settings"

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