[Django] #21518: override_settings(ROOT_URLCONF) doesn't clear resolver cache
Django
-------------------------------+-----------------
Reporter: gcc | Owner: gcc
Type: Uncategorized | Status: new
Component: Core (URLs) | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------+-----------------
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 [http://docs.django-
cms.org/en/2.4.2/extending_cms/testing.html 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:
{{{#!python
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21518>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: gcc | Owner: gcc
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by gcc):
* needs_better_patch: => 0
* type: Uncategorized => Bug
* needs_tests: => 0
* needs_docs: => 0
Comment:
Pull request is at: https://github.com/django/django/pull/2001
--
Ticket URL: <https://code.djangoproject.com/ticket/21518#comment:1>
Django
Reporter: gcc | Owner: gcc
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Changes (by aaugustin):
* needs_better_patch: 0 => 1
* easy: 1 => 0
* stage: Unreviewed => Accepted
Comment:
The idea is sane, see comment on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/21518#comment:2>
Django
Reporter: gcc | Owner: gcc
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
* needs_better_patch: 1 => 0
Comment:
Modified the PR branch as requested by @aaugustin.
--
Ticket URL: <https://code.djangoproject.com/ticket/21518#comment:3>
Django
Reporter: gcc | Owner: gcc
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21518#comment:4>
Django
Reporter: gcc | Owner: gcc
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21518#comment:5>
Django
Reporter: gcc | Owner: gcc
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/21518#comment:6>
Django
Reporter: gcc | Owner: gcc
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by mjtamlyn):
If we can move towards making more settings overridable with
`override_settings` I'd consider that a good thing.
--
Ticket URL: <https://code.djangoproject.com/ticket/21518#comment:7>
Django
Reporter: gcc | Owner: gcc
Type: Bug | Status: new
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by claudep):
The plan looks fine for me.
--
Ticket URL: <https://code.djangoproject.com/ticket/21518#comment:8>
Django
Reporter: gcc | Owner: gcc
Type: Bug | Status: closed
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"65131911dba08dcc1451d71ae4d5101724d722f6"]:
{{{
#!CommitTicketReference repository=""
revision="65131911dba08dcc1451d71ae4d5101724d722f6"
Fixed #21518 -- Made override_settings(ROOT_URLCONF) clear the resolver
cache.
Thanks Aymeric Augustin and Simon Charette for reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21518#comment:9>
Django
Reporter: gcc | Owner: gcc
Type: Bug | Status: closed
Component: Core (URLs) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+------------------------------------
Comment (by timo):
I created a follow-up ticket: "#21977 - Deprecate SimpleTestCase.urls in
favor of override_settings"
--
Ticket URL: <https://code.djangoproject.com/ticket/21518#comment:10>