[Django] #32939: Change override_settings to do its subclass check lazily
Django
------------------------------------------------+------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
Currently, the `@override_settings` decorator raises a `ValueError` if
it's used to decorate a class that doesn't inherit from `SimpleTestCase`:
https://github.com/django/django/blob/56f9579105c324ff15250423bf9f8bdf1634cfb4/django/test/utils.py#L519-L521
However, this prevents the decorator from being useable as a test-class
mixin for a number of `SimpleTestCase` subclasses. When creating shared
test-class functionality, it's often better to use a mixin rather than a
concrete `TestCase` class. This way the mixin won't have its `setUp`
methods run unnecessarily on a test-case class with no tests, and it won't
count towards the parallel test runner's test-case class count, etc.
The check could instead be done lazily, e.g. inside `setUpClass()`, while
still having the same desired effect.
I noticed this when seeing if I could convert `AuthViewsTestCase` into a
mixin:
https://github.com/django/django/blob/56f9579105c324ff15250423bf9f8bdf1634cfb4/tests/auth_tests/test_views.py
The `AuthViewsTestCase` change could be done in a nicer way once this
ticket is implemented, perhaps even as part of this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/32939>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Chris Jerdonek | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
PS - should this be a `TypeError` instead of `ValueError`?
--
Ticket URL: <https://code.djangoproject.com/ticket/32939#comment:1>
Django
Reporter: Chris Jerdonek | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Chris Jerdonek:
Old description:
> Currently, the `@override_settings` decorator raises a `ValueError` if
> it's used to decorate a class that doesn't inherit from `SimpleTestCase`:
> https://github.com/django/django/blob/56f9579105c324ff15250423bf9f8bdf1634cfb4/django/test/utils.py#L519-L521
>
> However, this prevents the decorator from being useable as a test-class
> mixin for a number of `SimpleTestCase` subclasses. When creating shared
> test-class functionality, it's often better to use a mixin rather than a
> concrete `TestCase` class. This way the mixin won't have its `setUp`
> methods run unnecessarily on a test-case class with no tests, and it
> won't count towards the parallel test runner's test-case class count,
> etc.
>
> The check could instead be done lazily, e.g. inside `setUpClass()`, while
> still having the same desired effect.
>
> I noticed this when seeing if I could convert `AuthViewsTestCase` into a
> mixin:
> https://github.com/django/django/blob/56f9579105c324ff15250423bf9f8bdf1634cfb4/tests/auth_tests/test_views.py
> The `AuthViewsTestCase` change could be done in a nicer way once this
> ticket is implemented, perhaps even as part of this ticket.
New description:
Currently, the `@override_settings` decorator raises a `ValueError` if
it's used to decorate a class that doesn't inherit from `SimpleTestCase`:
https://github.com/django/django/blob/56f9579105c324ff15250423bf9f8bdf1634cfb4/django/test/utils.py#L519-L521
However, this prevents the decorator from being useable as a test-class
mixin for a number of `SimpleTestCase` subclasses. When creating shared
test-class functionality, it's often better to use a mixin rather than a
concrete `TestCase` class. This way the mixin won't have its `setUp`
methods run unnecessarily on a test-case class with no tests, and it won't
count towards the parallel test runner's test-case class count, etc.
The check could instead be done lazily, e.g. inside `setUpClass()`, while
still having the same desired effect.
I noticed this when seeing if I could convert
[https://github.com/django/django/blob/56f9579105c324ff15250423bf9f8bdf1634cfb4/tests/auth_tests/test_views.py#L46
AuthViewsTestCase] into a mixin. This `AuthViewsTestCase` change could be
done in a nicer way once this ticket is implemented, perhaps even as part
of this ticket.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32939#comment:2>
Django
from unittest.TestCase
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* owner: nobody => Chris Jerdonek
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32939#comment:3>
Django
from unittest.TestCase
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
I realize now that the "non-lazy" check could be kept by modifying it to
be done only when the class inherits from `unittest.TestCase` (i.e. when
the class is a concrete test class):
{{{
--- a/django/test/utils.py
+++ b/django/test/utils.py
@@ -515,7 +515,7 @@ class override_settings(TestContextDecorator):
def decorate_class(self, cls):
from django.test import SimpleTestCase
- if not issubclass(cls, SimpleTestCase):
+ if issubclass(cls, TestCase) and not issubclass(cls,
SimpleTestCase):
raise ValueError(
"Only subclasses of Django SimpleTestCase can be
decorated "
"with override_settings")
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32939#comment:4>
Django
from unittest.TestCase
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/32939#comment:5>
Django
from unittest.TestCase
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
> This way the mixin won't have its setUp methods run unnecessarily on a
test-case class with no tests, and it won't count towards the parallel
test runner's test-case class count, etc.
Investigating further, it turns out neither of these seem to happen in
practice, since test classes without tests get filtered out. Nevertheless,
being able to apply the decorator to test mixins would still be a useful
enhancement.
--
Ticket URL: <https://code.djangoproject.com/ticket/32939#comment:6>
Django
from unittest.TestCase
Reporter: Chris Jerdonek | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* owner: Chris Jerdonek => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/32939#comment:7>
Django
from unittest.TestCase
--------------------------------------+------------------------------------
Reporter: Chris Jerdonek | Owner: (none)
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Florian Demmer):
* cc: Florian Demmer (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32939#comment:8>
Django
from unittest.TestCase
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jwang234):
* owner: (none) => jwang234
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32939#comment:9>
Django
from unittest.TestCase
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jwang234):
* owner: jwang234 => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/32939#comment:10>
Django
from unittest.TestCase
Reporter: Chris Jerdonek | Owner: Jonathan
Type: | Wang
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jwang234):
* owner: (none) => Jonathan Wang
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32939#comment:11>