[Django] #27019: DiscoverRunner does not restore old settings.DEBUG value in teardown
Django
-----------------------------------+--------------------
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------
`DiscoverRunner.teardown_test_environment()`
[https://github.com/django/django/blob/967aa7f6cc720b11e38b7e0cbeffc2c95c64fa05/django/test/runner.py#L529
does not] restore the pre-test value of `settings.DEBUG`, as for example
`test.utils.teardown_test_environment()` does for
[https://github.com/django/django/blob/967aa7f6cc720b11e38b7e0cbeffc2c95c64fa05/django/test/utils.py#L122
the values it knows about].
Since `test.utils`'s `setup_test_environment()` and
`teardown_test_environment()` already have a pattern of storing and
restoring environment values, I propose that this be addressed by adding a
`debug` argument to `test.utils.setup_test_environment()` that
`DiscoverRunner.setup_test_environment()` can then use.
Alternatively, `test.utils.setup_test_environment()` could take
responsibility for setting `DEBUG` to false.
--
Ticket URL: <https://code.djangoproject.com/ticket/27019>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: cjerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.10
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 timgraham):
* needs_better_patch: => 0
* needs_docs: => 0
* type: Bug => Cleanup/optimization
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Is it a practical problem? Typically, more Django code doesn't run at the
end of a test run. I guess the argument to `setup_test_environment()` is
okay as long as it defaults to `debug=None` (i.e. don't change the value)
for backwards compatibility.
--
Ticket URL: <https://code.djangoproject.com/ticket/27019#comment:1>
Django
Reporter: cjerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
No, it hasn't been a practical problem for me. But I think it will help
conceptually and for consistency to have this logic in one place.
--
Ticket URL: <https://code.djangoproject.com/ticket/27019#comment:2>
Django
Reporter: cjerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
I posted a pull request for this
[https://github.com/django/django/pull/7025 here] on GitHub.
--
Ticket URL: <https://code.djangoproject.com/ticket/27019#comment:3>
Django
Reporter: cjerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by cjerdonek):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27019#comment:4>
Django
Reporter: cjerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Left a few comments for improvement.
--
Ticket URL: <https://code.djangoproject.com/ticket/27019#comment:5>
Django
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Component: Testing framework | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by cjerdonek):
* owner: nobody => cjerdonek
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27019#comment:6>
Django
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by cjerdonek):
I'm still thinking about the best way to test (and whether to test for the
purposes of this patch).
If possible, I'm thinking it would be better to make the function itself
more testable (e.g. by dependency injection) or perhaps by patching. But
both of those approaches would be considerably more work than calling
`teardown_test_environment()`, as there are a number of objects in play
(e.g. the `settings` object, the `_SavedSettings` object, the `Template`
class, the `mail` and `request` modules, `deactivate()`, etc). In a later
ticket, it would be straightforward to switch from using the "arbitrary"
modules to the `_SavedSettings` object. That would at least be a
simplification, and would make it easier to test directly via patching or
dependency injection.
--
Ticket URL: <https://code.djangoproject.com/ticket/27019#comment:7>
Django
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by cjerdonek):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27019#comment:8>
Django
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Yes, I think switching things over to `_SavedSettings` and then using
`@mock.patch('django.test.utils._SavedSettings')` (or whatever the correct
incantation of that is) might be the way to go.
--
Ticket URL: <https://code.djangoproject.com/ticket/27019#comment:9>
Django
Reporter: cjerdonek | Owner: cjerdonek
Cleanup/optimization |
Component: Testing framework | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"7f9fd42b9316e4beddb690bae61940e940281805" 7f9fd42]:
{{{
#!CommitTicketReference repository=""
revision="7f9fd42b9316e4beddb690bae61940e940281805"
Fixed #27019 -- Made teardown_test_environment() restore the old DEBUG.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27019#comment:10>