[Django] #22449: Colorize output of test results
Django
--------------------------------------------+--------------------
Reporter: jrothenbuhler | Owner: nobody
Type: New feature | Status: new
Component: Core (Management commands) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------------+--------------------
Colorized test result output will improve readability
--
Ticket URL: <https://code.djangoproject.com/ticket/22449>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jrothenbuhler | Owner:
Type: New feature | jrothenbuhler
Component: Core (Management | Status: assigned
commands) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by jrothenbuhler):
* owner: nobody => jrothenbuhler
* needs_docs: => 0
* status: new => assigned
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:1>
Django
Reporter: jrothenbuhler | Owner:
Type: New feature | jrothenbuhler
Component: Core (Management | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by jrothenbuhler):
* has_patch: 0 => 1
Comment:
Submitted a pull request: https://github.com/django/django/pull/2565
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:2>
Django
Reporter: jrothenbuhler | Owner:
Type: New feature | jrothenbuhler
Component: Core (Management | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
I think this would be a useful feature.
The patch looks good overall but I left a few comments on the pull
request.
Once those are fixed, you can remove the `patch needs improvement` flag.
Thanks
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:3>
Django
Reporter: jrothenbuhler | Owner:
Type: New feature | jrothenbuhler
Component: Core (Management | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jrothenbuhler):
* needs_better_patch: 1 => 0
Comment:
Thanks for the good suggestions. I've updated the
[https://github.com/django/django/pull/2565 pull request]
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:4>
Django
Reporter: jrothenbuhler | Owner:
Type: New feature | jrothenbuhler
Component: Core (Management | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
Comment:
I added some more comments for improvement on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:5>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Yuri Shikanov):
* owner: Jake Rothenbuhler => Yuri Shikanov
Comment:
I'll work on it.
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:6>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Chris Jerdonek):
* cc: Chris Jerdonek (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:7>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Yuri Shikanov):
Submitted a pull request: https://github.com/django/django/pull/9688
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:8>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Yuri Shikanov):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:9>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
There's a small issue that the tests can be affected by the
`DJANGO_COLORS` environment variable of the calling process. (See comment
on PR).
But other than that, it looks a nice patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:10>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Yuri Shikanov):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:11>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:12>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
I'm not sure that including this in Django is a good idea. In particular,
copying entire parts of Python like `TextTestRunner.run()` could be a
maintenance headache. I found some third party solutions like
[https://github.com/django/django/pull/9688 redgreenunittests] (which does
the same I guess, but since it's a separate package, it can adapt more
quickly than Django can).
Colorizing test results sounds more like something that Python should
allow rather than a problem that Django should solve.
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:13>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
FWIW, I agree with Tim. Also, the proposed implementation seems to tightly
couple much of Django's test harness with the idea of coloring. For
example, `DebugSQLTextTestResult` now inherits from
`ColoredTextTestResult` when conceptually debugging SQL should be
orthogonal to coloring. I would rather see a looser coupling (e.g. making
it easier to "plug" coloring in through appropriate hooks or composition,
etc.) rather than making the class hierarchies even heavier weight.
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:14>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Yuri Shikanov):
So, should I try to refine current implementation in PR?
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:15>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
Speaking just for myself, is there anything in your PR that can't be done
by the third-party library Tim mentioned? Or perhaps is there some change
to Django that would make it easier to integrate such a library? For
example, I notice that the library requires you to use a test runner
called `RedGreenDiscoverRunner`, which subclasses `DiscoverRunner`. But it
seems like there should be a lighter weight way to enable coloring (e.g.
using composition rather than taking over the whole class hierarchy with
inheritance).
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:16>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Yuri Shikanov):
Speaking just for myself, is there anything in your PR that can't be done
by the third-party library Tim mentioned?
If there is nothing in this PR that can't be done by third-party, Does
this mean that this shouldn't be included in Django?
I think colored test results is something that should be included in the
framework by default.
Anyway, feel free to close this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:17>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
I wouldn't say we could never add this, but the current implementation
doesn't look suitable. It might be that Python's unittest doesn't
currently offer the proper hooks to create an elegant implementation.
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:18>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Core (Management | Version: master
commands) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tim Graham):
* stage: Accepted => Someday/Maybe
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:19>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tim Graham):
* component: Core (Management commands) => Testing framework
* stage: Accepted => Someday/Maybe
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:19>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Rich Jones):
This is a bad decision and the related PR should be merged.
The fact that there exists an obscure, ancient, unmaintained and unused
third party library that also implements this (with 7 stars on GitHub and
the last serious commit 4 years ago) is not a reason to ignore this facet
of major testing usability problem that Django has.
If you don't like the solution because there are upstream changes which
need to be added, you should then create those upstream tickets, and in
the meantime accept the PR until the upstream changes land. It's not good
maintainership to ignore good work and completely punt the issue.
Testing usability in Django is an absolute mess and anything to help
improve it should be welcomed, not abandoned after all the work is done.
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:20>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Chris Jerdonek):
Personally, I disagree that the lack of color is a "major testing
usability problem," or even that it's a problem at all. I would consider
it a nice to have. Also, in my opinion, the PR that was proposed had
issues independent of upstream, but the author volunteered to close the
ticket.
Lastly, while there are certainly things that can be added or improved
about Django testing, I think testing usability on the whole is great. I
don't see how you can be calling it a mess.
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:21>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Yuri Shikanov):
Actually, I agree that proposed implementation has issues. I asked whether
I should try to refine implementation. As I understand, any implementation
wouldn't be merged, because unittest doesn't let to create a much more
elegant implementation.
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:22>
Django
Reporter: Jake Rothenbuhler | Owner: Yuri
| Shikanov
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Someday/Maybe
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Chris Jerdonek):
Tim didn't say any implementation wouldn't be merged. He said the current
implementation doesn't look suitable, and that `unittest` //might// not
allow an elegant implementation. I agree. Personally, though, I do think
there is lots of room for a more elegant approach and that the decision
should be based on the particular patch (and its maintainability, etc).
It's probably better for implementation issues to be discussed on the PR
rather than here though.
--
Ticket URL: <https://code.djangoproject.com/ticket/22449#comment:23>