Opened 11 years ago
Closed 10 years ago
#21281 closed Cleanup/optimization (fixed)
Make override_settings act at class level when used as a TestCase decorator
Reported by: | anonymous | Owned by: | nobody |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | alasdair, t.chaumeny@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The override_settings decorator doesn't effect the setUpClass method (I believe this is because class decorators don't effect classmethods) and as the self.settings() method requires an object instance, there is no easy way to change the settings for the setUpClass method.
Is there any reason settings() requires an instance and couldn't be class based?
Attachments (1)
Change History (15)
comment:1 by , 11 years ago
Cc: | added |
---|
comment:2 by , 11 years ago
by , 11 years ago
Attachment: | 21281-1.diff added |
---|
comment:3 by , 11 years ago
Has patch: | set |
---|---|
Needs documentation: | set |
The real reason why settings are not overriden in setUpClass
is that we want the settings override process to happen before each test, and not only once for the entire class, so changed settings cannot leak between tests. I've attached a patch with a possible approach where we both override settings once in setUpClass
and again in _pre_setup
before each test. This would be a bit redundant and would require to call the parent setUpClass
before settings are really overridden (would need to be documented).
comment:4 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Cleanup/optimization |
Version: | 1.5 → master |
comment:5 by , 10 years ago
Cc: | added |
---|---|
Needs documentation: | unset |
We discussed it with claudep and timograham on #django-dev and came to the conclusion that overriding settings directly was a mistake and that we should not rely on override_settings
to correct that in some cases. A specific warning was added to tell users not to do so https://github.com/django/django/commit/3f651b3e88ac1ba8d04acd5a074362866a0a963a.
The proposed PR moves the whole override logic at class level when the class is decorated. This way, settings are overriden within setUpClass
and tearDownClass
(I included bmispelon's test case above).
A side effect of that change is that users should always call super
when defining setUpClass
/ tearDownClass
. This side effect has been discussed on #20392, which this ticket is a prerequisite.
comment:6 by , 10 years ago
Summary: | trying to override settings in test case setUpClass → Make override_settings act at class level when used as a TestCase decorator |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:7 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:9 by , 10 years ago
Has patch: | unset |
---|---|
Resolution: | fixed |
Severity: | Normal → Release blocker |
Status: | closed → new |
Triage Stage: | Ready for checkin → Accepted |
I found a regression here that causes override_settings()
to leak when used on a subclass test class. See DateTimePickerShortcutsSeleniumFirefoxTests
in admin_widgets
(we didn't notice it at the time because that's a selenium test which isn't run on Jenkins).
Here's a simple test script which fails on the second test.
from unittest import TestCase from django.conf import settings from django.contrib.admin.tests import AdminSeleniumWebDriverTestCase from django.test import override_settings @override_settings(TIME_ZONE='Asia/Singapore') class Test(AdminSeleniumWebDriverTestCase): def test(self): assert True class Test2(TestCase): def test(self): self.assertEqual(settings.TIME_ZONE, "America/Chicago")
comment:10 by , 10 years ago
After some investigation, that seems to be a consequence of a duplicated super(..., cls).setUpClass()
in LiveServerTestCase
:
https://github.com/django/django/blob/52f0b2b62262743d5f935ddae29428e661b5d8ea/django/test/testcases.py#L1210
and https://github.com/django/django/blob/52f0b2b62262743d5f935ddae29428e661b5d8ea/django/test/testcases.py#L1258 (looks like I introduced that)
If I remove one of those lines, the test passes fine.
comment:11 by , 10 years ago
Has patch: | set |
---|
I'd suggest removing the last one (https://github.com/django/django/pull/3834). OK?
comment:12 by , 10 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
This fixes the test failure I'm seeing.
comment:14 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Hi,
I can reproduce the issue described (using this testcase: https://gist.github.com/bmispelon/7041561).
I don't know how fixable it is, but if anything, the limitation should at least be documented, so I'm accepting this ticket.
Thanks for the report.