Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18417 closed Bug (fixed)

override_settings doesn't work with unittest.TestCase subclasses

Reported by: claudep 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 claudep 3 years ago.
Allow unittest.TestCase to be decorated by override_settings
18417-2.diff (6.0 KB) - added by claudep 3 years ago.
Raise if decorated class is not subclass of SimpleTestCase

Download all attachments as: .zip

Change History (11)

Changed 3 years ago by claudep

Allow unittest.TestCase to be decorated by override_settings

comment:1 Changed 3 years ago by ethlinn

  • Cc asendecka@… added
  • Triage Stage changed from Accepted to Ready 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 follow-up: Changed 3 years ago by aaugustin

  • Triage Stage changed from Ready for checkin to Design 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.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 3 years ago by 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.

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.

comment:4 in reply to: ↑ 3 Changed 3 years ago by aaugustin

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.

Changed 3 years ago by claudep

Raise if decorated class is not subclass of SimpleTestCase

comment:5 Changed 3 years ago by claudep

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 3 years ago by claudep (previous) (diff)

comment:6 Changed 3 years ago by Claude Paroz <claude@…>

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

In 9f7cefd5059e78d75f1e1a852b51a6286ecec728:

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

comment:7 Changed 3 years ago by Claude Paroz <claude@…>

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 Changed 3 years ago by Claude Paroz <claude@…>

In 4389b51fabf6d5d98d3d59681a527e0b4fbe4331:

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

Backport of 9f7cefd5 from master.

comment:9 Changed 3 years ago by Claude Paroz <claude@…>

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