[Django] #27035: DiscoverRunner's setup_test_environment() hard-codes settings.DEBUG to False
Django
False
--------------------------------------+--------------------
Reporter: cjerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
Currently, the `setup_test_environment()` method of Django's
`DiscoverRunner` hard-codes setting `settings.DEBUG` to False. See
[https://github.com/django/django/blob/a6baada7bdf0c2b14045ee86364f9401c7b91b7c/django/test/runner.py#L414
here] for a direct link to that code.
This is less convenient for subclassers that would like to provide the
ability to selectively run certain tests with `DEBUG` set to True (e.g.
see [https://code.djangoproject.com/ticket/27008 here] for a ticket to
expose this as a command-line option).
This ticket is to make subclassing easier by adding a `debug_mode`
instance attribute to `DiscoverRunner` (the suffix "mode" is to better
distinguish from the current `debug_sql` attribute). This would also make
progress towards ticket #27008.
--
Ticket URL: <https://code.djangoproject.com/ticket/27035>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
False
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Component: Testing framework | Version: master
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 cjerdonek):
* owner: nobody => cjerdonek
* needs_better_patch: => 0
* status: new => assigned
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27035#comment:1>
Django
False
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Did you consider an argument to `DiscoverRunner.setup_test_environment()`
instead? It's not clear to me that subclassing the test runner is the best
approach for #27008.
--
Ticket URL: <https://code.djangoproject.com/ticket/27035#comment:2>
Django
False
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
This issue is just an easy subset of what would be done for #27008
anyways. But it has the nice feature that it would make it easier for
others to add a `--debug` option on their own via subclassing even before
#27008 is closed.
In other words, I wasn't saying #27008 would be addressed via sub-
classing. Rather, it would allow others to subclass in lieu of the issue.
In terms of your suggestion, adding an argument to
`DiscoverRunner.setup_test_environment()` would just push the issue back
another method to `DiscoverRunner.run_tests()`, where
`setup_test_environment()` is called (in which case subclassers would have
to reimplement all of `run_tests()` just to change how
`setup_test_environment()` is called). All of the other command-line
options work by setting an instance attribute in the constructor and using
those attributes throughout the methods, which is what I'm suggesting
doing here.
--
Ticket URL: <https://code.djangoproject.com/ticket/27035#comment:3>
Django
False
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
Or are you suggesting that an argument be added to both
`DiscoverRunner.setup_test_environment()` ''and''
`DiscoverRunner.run_tests()`? That would be another way to address the
subclassing issue without needing to copy / reimplement methods.
See
[https://github.com/django/django/blob/a6baada7bdf0c2b14045ee86364f9401c7b91b7c/django/core/management/commands/test.py#L60
here] for how the test management command constructs the runner and
invokes `run_tests()`. As you can see, there are two options for passing
options: passing to the constructor and passing to `run_tests()`.
Currently, only the test labels are being passed to `run_tests()` and the
rest are passed via the constructor.
--
Ticket URL: <https://code.djangoproject.com/ticket/27035#comment:4>
Django
False
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Following the pattern of the other option in `DiscoverRunner.__init__()`
seems fine, but do you have a subclassing use case in mind?
--
Ticket URL: <https://code.djangoproject.com/ticket/27035#comment:5>
Django
False
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
Yes, to add a `--debug` option more easily prior to ticket #27008 being
addressed. :) But that is all.
The subclassing rationale is mainly to provide a valid reason for this
ticket independent of other tickets. And it is good practice in general to
move away from hard-coding values within method bodies, etc.
--
Ticket URL: <https://code.djangoproject.com/ticket/27035#comment:6>
Django
False
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
Comment:
For some reason, I thought you were proposing a class attribute, but now I
see you wrote instance attribute. It may be a bit simpler to tackle #27008
straight away, as that's how the rest of the `__init__()` arguments were
added, but I guess if you want to work in small chunks, the only
disadvantage is more tickets and pull requests.
--
Ticket URL: <https://code.djangoproject.com/ticket/27035#comment:7>
Django
False
Reporter: cjerdonek | Owner: cjerdonek
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by cjerdonek):
* has_patch: 0 => 1
Comment:
Pull request added [https://github.com/django/django/pull/7052 here].
--
Ticket URL: <https://code.djangoproject.com/ticket/27035#comment:8>
Django
False
Reporter: cjerdonek | Owner: cjerdonek
Cleanup/optimization |
Component: Testing framework | Version: master
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:"a3a5ef4d0e3bf10ff80b26dd2635e40679156a2f" a3a5ef4d]:
{{{
#!CommitTicketReference repository=""
revision="a3a5ef4d0e3bf10ff80b26dd2635e40679156a2f"
Fixed #27035 -- Eased changing settings.DEBUG for DiscoverRunner.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27035#comment:9>