Opened 5 years ago

Closed 5 years ago

#27019 closed Cleanup/optimization (fixed)

DiscoverRunner does not restore old settings.DEBUG value in teardown

Reported by: Chris Jerdonek Owned by: Chris Jerdonek
Component: Testing framework Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

DiscoverRunner.teardown_test_environment() does not restore the pre-test value of settings.DEBUG, as for example test.utils.teardown_test_environment() does for 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.

Change History (10)

comment:1 Changed 5 years ago by Tim Graham

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

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.

comment:2 Changed 5 years ago by Chris Jerdonek

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.

comment:3 Changed 5 years ago by Chris Jerdonek

I posted a pull request for this here on GitHub.

comment:4 Changed 5 years ago by Chris Jerdonek

Has patch: set

comment:5 Changed 5 years ago by Tim Graham

Patch needs improvement: set

Left a few comments for improvement.

comment:6 Changed 5 years ago by Chris Jerdonek

Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:7 Changed 5 years ago by Chris Jerdonek

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.

comment:8 Changed 5 years ago by Chris Jerdonek

Patch needs improvement: unset

comment:9 Changed 5 years ago by Tim Graham

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.

comment:10 Changed 5 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 7f9fd42:

Fixed #27019 -- Made teardown_test_environment() restore the old DEBUG.

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