[Django] #32552: Change DiscoverRunner to use a logger instead of print
Django
-------------------------------------+-------------------------------------
Reporter: Chris | Owner: nobody
Jerdonek |
Type: New | Status: new
feature |
Component: Testing | Version: 4.0
framework | Keywords:
Severity: Normal | DiscoverRunner,print,logging,stdout,stderr
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Currently, the `DiscoverRunner` class (used when running tests) "logs" by
printing directly to stdout. Here is one example:
https://github.com/django/django/blob/7bdd09d016f418719f2d0297f58bd81c5349101d/django/test/runner.py#L592-L596
It would be much better if `DiscoverRunner` could instead use a Python
logger. The "verbosity" of each `print()` statement could be converted to
a log message at an appropriate level.
Related to this, passing a `--verbosity {0,1,2,3}` (e.g. when using
[https://github.com/django/django/blob/7bdd09d016f418719f2d0297f58bd81c5349101d/tests/runtests.py#L433-L436
runtests.py]) could configure a logger at the appropriate level.
This ticket is related to (but different from) ticket #21429, which is
about `BaseCommand`. While `DiscoverRunner` can be triggered by the `test`
command, it can also be run separately from a management command, e.g. via
`runtests`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32552>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
DiscoverRunner,print,logging,stdout,stderr| 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'll note that one thing this would help with is when testing
`DiscoverRunner`. Currently, testing some `DiscoverRunner` methods results
in undesired console output. If a logger was used instead of printing,
then the test framework could take care of this without any extra work.
Using a logger would also make it easier to test that certain messages are
being emitted, because the testing framework's logging assertions could be
used.
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:1>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
DiscoverRunner,print,logging,stdout,stderr| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Daniyal Abbasi):
Is this ticket available? I would like to take up.
I went through the `DiscoverRunner` class and I found 7 instances where
`print` was used.
I believe a good starting approach would be to classify the print
statements on the bases of their logging levels.
105: `print("\nOpening PDB: %r" % exc_value)` -> DEBUG
142: `print("""Subtest failed: ...``` -> ERROR
172 & 186 : Two print statements in the `check_picklable()` -> ERROR
594: `print('Including test tag(s): %s.' % ', '.join(sorted(self.tags)))`
-> INFO
596: `print('Excluding test tag(s): %s.' % ',
'.join(sorted(self.exclude_tags)))` -> INFO
681: `print('Skipping setup of unused database(s): %s.' % ',
'.join(sorted(unused_databases)))` -> INFO
Let me know if this is the right way to proceed!
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:2>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
DiscoverRunner,print,logging,stdout,stderr| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
Only the last three are in the `DiscoverRunner` class. The others have
different considerations.
I don't think this ticket is necessarily straightforward. For example, I
think the behavior should be opt-in (at least for now) because callers
might not have logging enabled. I'm not sure yet what the API should be
for opting in though. I'm still thinking about it.
I think a good first step would be to add a `log(level, msg)` method to
the `DiscoverRunner` class that preserves the existing `print()` behavior.
I.e. the signature of the method would match:
https://docs.python.org/3/library/logging.html#logging.Logger.log
And then change the class to use that method for its print statements. The
method would use the given `level` and compare to `self.verbosity` to
determine whether the message should be printed.
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:3>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
DiscoverRunner,print,logging,stdout,stderr| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Daniyal Abbasi):
Sure. As suggested, we could add a member function, something like this
{{{
def log(self, msg, level=2):
if self.verbosity >= level:
sys.stderr.write(msg + os.linesep)
}}}
Should we stick with `print` for now or use `sys.stderr.write` / `Logger`?
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:4>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage:
DiscoverRunner,print,logging,stdout,stderr| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
Since this would be a simple refactor with no behavior change, it should
be `print()`. For the level though, I was thinking a logging level since
that's where we're headed and that's the API that most people are familiar
with. The method can do the conversion from logging level to "verbosity."
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:5>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
DiscoverRunner,print,logging,stdout,stderr|
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
I think this is a good move, so let's accept the ticket.
> I don't think this ticket is necessarily straightforward. For example, I
think the behavior should be opt-in (at least for now) because callers
might not have logging enabled. I'm not sure yet what the API should be
for opting in though. I'm still thinking about it.
Yes... 🤔 And folks have a hard time getting up and running with logging,
so we need to be conscious of that. Part of it is good documentation.
Thanks for all the work you're doing in this area Chris, and thanks for
wanting to pick this up Daniyal!
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:6>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
DiscoverRunner,print,logging,stdout,stderr|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Daniyal Abbasi):
Sure Carlton! Will be sending a PR soon.
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:7>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
DiscoverRunner,print,logging,stdout,stderr|
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:8>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
DiscoverRunner,print,logging,stdout,stderr|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* has_patch: 0 => 1
* needs_tests: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:9>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
DiscoverRunner,print,logging,stdout,stderr|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
By the way, I think the step after Daniyal's PR would be to change
`DiscoverRunner`'s constructor to accept an optional Python `logger`
object. Then, `DiscoverRunner.log()` could use that, if available, and
falling back to `print()` if `self.logger` isn't set.
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
DiscoverRunner,print,logging,stdout,stderr|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
DiscoverRunner,print,logging,stdout,stderr| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
DiscoverRunner,print,logging,stdout,stderr| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"a0410ffe8f43d8dcfbaa261e068879698fafdca3" a0410ffe]:
{{{
#!CommitTicketReference repository=""
revision="a0410ffe8f43d8dcfbaa261e068879698fafdca3"
Refs #32552 -- Added DiscoverRunner.log() to allow customization.
Thanks Carlton Gibson, Chris Jerdonek, and David Smith for reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
DiscoverRunner,print,logging,stdout,stderr|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:14>
Django
-------------------------------------+-------------------------------------
| Jerdonek
Type: New feature | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
DiscoverRunner,print,logging,stdout,stderr|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Chris Jerdonek
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Type: New feature | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
DiscoverRunner,print,logging,stdout,stderr|
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/14759
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Type: New feature | Status: assigned
Component: Testing framework | Version: 4.0
Severity: Normal | Resolution:
DiscoverRunner,print,logging,stdout,stderr| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
| Jerdonek
Component: Testing framework | Version: 4.0
Keywords: | Triage Stage: Ready for
DiscoverRunner,print,logging,stdout,stderr| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"b263f4b69db4093847ccc3b85e51cc7f3759e42c" b263f4b6]:
{{{
#!CommitTicketReference repository=""
revision="b263f4b69db4093847ccc3b85e51cc7f3759e42c"
Fixed #32552 -- Added logger argument to DiscoverRunner.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32552#comment:18>