Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#18417 closed Bug (fixed)

override_settings doesn't work with unittest.TestCase subclasses

Reported by: Claude Paroz Owned by: nobody
Component: Testing framework Version: 1.4
Severity: Normal Keywords:
Cc: asendecka@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, if you decorate with override_settings a subclass of unittest.TestCase, all tests in the class are silently skipped. This is happening because of the issubclass(TransactionTestCase) test here: https://github.com/django/django/blob/master/django/test/utils.py#L188

We cannot simply change the issubclass test, because ut.TestCase doesn't have any _pre_setup mechanism.

I can see two ways forward:

  • Document that override_settings cannot decorate unittest.TestClass subclasses, and raise an error when the decorator is applied to anything other than TransactionTestCase subclasses.
  • Instead of using _pre_setup, use setUpClass to enable the overriden settings.

I'd really like to see the second alternative implemented. However, this means that the override_settings machinery will only run at the start of the TestClass tests, and no more between each test (unless the specific test itself is decorated by override_settings). This shouldn't disturb the test system more than that.

Attachments (2)

18417-1.diff (2.0 KB ) - added by Claude Paroz 12 years ago.
Allow unittest.TestCase to be decorated by override_settings
18417-2.diff (6.0 KB ) - added by Claude Paroz 11 years ago.
Raise if decorated class is not subclass of SimpleTestCase

Download all attachments as: .zip

Change History (11)

by Claude Paroz, 12 years ago

Attachment: 18417-1.diff added

Allow unittest.TestCase to be decorated by override_settings

comment:1 by Aleksandra Sendecka, 12 years ago

Cc: asendecka@… added
Triage Stage: AcceptedReady for checkin

Patch works nice for me. I tested that by adding new unittest TestCase with assert False. Before applying the patch, the test was skipped and after - the test failed (as expected). That's a pity it's no good way to add a real test for that.

comment:2 by Aymeric Augustin, 11 years ago

Triage Stage: Ready for checkinDesign decision needed

override_settings is Django-specific; I believe it's fair to restrict it to subclasses of django.test.TransactionTestCase or TestCase, provided an exception is raised when it's misused.

Currently, override_settings guarantees that the settings are reset before each test in the test case. Altering them in a test won't affect other tests. This isn't documented but it's likely that some people are relying on this behavior.

So the current patch will lead to test failures for some users after upgrading Django. These test failures won't happen if the failing test is run in isolation, making them hard to track down.

In my eyes this is as a choice between:

  • raising an exception for people who've written tests that never ran; and
  • taking the risk to introduce unexpected test failures for people who've written working tests.

I'm +1 on the first option and -0 on the second.

in reply to:  2 ; comment:3 by Claude Paroz, 11 years ago

Replying to aaugustin:

override_settings is Django-specific; I believe it's fair to restrict it to subclasses of django.test.TransactionTestCase or TestCase, provided an exception is raised when it's misused.

I don't follow you here. There are clearly use-cases currently in our test suite that would need override_settings for unittest.TestCase. It does not appear so unnatural to me.

Currently, override_settings guarantees that the settings are reset before each test in the test case. Altering them in a test won't affect other tests. This isn't documented but it's likely that some people are relying on this behavior.

That's right, and that's why I didn't commit it until now.

(...)

In my eyes this is as a choice between:

  • raising an exception for people who've written tests that never ran; and
  • taking the risk to introduce unexpected test failures for people who've written working tests.

I'm +1 on the first option and -0 on the second.

There might be a third option, use _pre_setup/_post_teardown for Django test subclasses, and setUpClass/tearDownClass for unittest.TestCase subclasses. It's a bit peculiar to have different behaviours depending on the test class, but at least compatibility is kept, and we can document it.

in reply to:  3 comment:4 by Aymeric Augustin, 11 years ago

Replying to claudep:

Replying to aaugustin:

override_settings is Django-specific; I believe it's fair to restrict it to subclasses of django.test.TransactionTestCase or TestCase, provided an exception is raised when it's misused.

I don't follow you here. There are clearly use-cases currently in our test suite that would need override_settings for unittest.TestCase. It does not appear so unnatural to me.

Right -- unittest.TestCase is sometimes preferred because it's faster.

The current practice in Django's test suite is to use django.test.(Transaction)TestCase when the test cases requires some Django-specific behavior (fixtures, test client, etc.) and unittest.TestCase when it doesn't.

Since override_settings is Django-specific, I suggest switching test cases decorated with @override_settings to django.test.(Transaction)TestCase.

If override_settings raises an exception for unsupported classes, we'll immediately know which ones need this change.

by Claude Paroz, 11 years ago

Attachment: 18417-2.diff added

Raise if decorated class is not subclass of SimpleTestCase

comment:5 by Claude Paroz, 11 years ago

The attached patch raises an exception if the decorated class is not a subclass of SimpleTestCase. Note that currently this works only for TransactionTestCase, but the current documentation tells that SimpleTestCase can be decorated by override_settings (and I also think it should) (https://docs.djangoproject.com/en/dev/topics/testing/#django.test.SimpleTestCase).

Last edited 11 years ago by Claude Paroz (previous) (diff)

comment:6 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 9f7cefd5059e78d75f1e1a852b51a6286ecec728:

Fixed #18417 -- Raised exception when unittest.TestCase is decorated with override_settings

comment:7 by Claude Paroz <claude@…>, 11 years ago

In a5d47415f47025b014e580af1533e9023dc32945:

Enabled SimpleTestCase to be decorated by override_settings

Refs #18417. Also fixed some test case classes which subclassed
the wrong parent.

comment:8 by Claude Paroz <claude@…>, 11 years ago

In 4389b51fabf6d5d98d3d59681a527e0b4fbe4331:

[1.5.x] Fixed #18417 -- Raised exception when unittest.TestCase is decorated with override_settings

Backport of 9f7cefd5 from master.

comment:9 by Claude Paroz <claude@…>, 11 years ago

In 6945f60c2b2406ba06d889020d50aaba80e7d317:

[1.5.x] Enabled SimpleTestCase to be decorated by override_settings

Refs #18417. Also fixed some test case classes which subclassed
the wrong parent.
Backport of a5d47415f from master.

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