Code

Opened 5 months ago

Closed 2 months ago

Last modified 2 months ago

#21518 closed Bug (fixed)

override_settings(ROOT_URLCONF) doesn't clear resolver cache

Reported by: gcc Owned by: gcc
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.

Attachments (0)

Change History (10)

comment:1 Changed 5 months ago by gcc

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Bug

comment:2 Changed 5 months ago by aaugustin

  • Easy pickings unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

The idea is sane, see comment on the PR.

comment:3 Changed 5 months ago by gcc

  • Patch needs improvement unset

Modified the PR branch as requested by @aaugustin.

comment:4 Changed 4 months ago by claudep

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 4 months ago by gcc

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 4 months ago by aaugustin

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 4 months ago by mjtamlyn

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

comment:8 Changed 4 months ago by claudep

The plan looks fine for me.

comment:9 Changed 2 months ago by Tim Graham <timograham@…>

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

In 65131911dba08dcc1451d71ae4d5101724d722f6:

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

Thanks Aymeric Augustin and Simon Charette for reviews.

comment:10 Changed 2 months ago by timo

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

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.