Opened 21 months ago

Closed 6 months 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: master
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)

21281-1.diff (1.6 KB) - added by claudep 21 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 21 months ago by alasdair

  • Cc alasdair added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 21 months ago by bmispelon

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.

Changed 21 months ago by claudep

comment:3 Changed 21 months ago by claudep

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

  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization
  • Version changed from 1.5 to master

comment:5 Changed 8 months ago by tchaumeny

  • Cc t.chaumeny@… 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 Changed 8 months ago by timgraham

  • Summary changed from trying to override settings in test case setUpClass to Make override_settings act at class level when used as a TestCase decorator
  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 8 months ago by Tim Graham <timograham@…>

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

In d89f56dc4d03f6bf6602536b8b62602ec0d46d2f:

Fixed #21281 -- Made override_settings act at class level when used as a TestCase decorator.

comment:8 Changed 7 months ago by Tim Graham <timograham@…>

In e77462249339f465395f49f0d0b149b670a696f3:

Moved release note for refs #21281 from "deprecation" to "backwards incompatible".

comment:9 Changed 6 months ago by timgraham

  • Has patch unset
  • Resolution fixed deleted
  • Severity changed from Normal to Release blocker
  • Status changed from closed to new
  • Triage Stage changed from Ready for checkin to 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 Changed 6 months ago by tchaumeny

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

  • Has patch set

I'd suggest removing the last one (https://github.com/django/django/pull/3834). OK?

comment:12 Changed 6 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

This fixes the test failure I'm seeing.

comment:13 Changed 6 months ago by Claude Paroz <claude@…>

In 3bac904607f1999136b97249d9aa220f1db94258:

Removed extraneous super call in LiveServerTestCase

Refs #21281. Thanks Tim Graham and Thomas Chaumeny for investigations.

comment:14 Changed 6 months ago by timgraham

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top