Opened 10 years ago

Last modified 6 years ago

#22449 assigned New feature

Colorize output of test results

Reported by: Jake Rothenbuhler Owned by: Yuri Shikanov
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Chris Jerdonek Triage Stage: Someday/Maybe
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Colorized test result output will improve readability

Change History (23)

comment:1 by Jake Rothenbuhler, 10 years ago

Owner: changed from nobody to Jake Rothenbuhler
Status: newassigned

comment:2 by Jake Rothenbuhler, 10 years ago

Has patch: set

comment:3 by Baptiste Mispelon, 10 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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

comment:4 by Jake Rothenbuhler, 10 years ago

Patch needs improvement: unset

Thanks for the good suggestions. I've updated the pull request

comment:5 by Tim Graham, 10 years ago

Patch needs improvement: set

I added some more comments for improvement on the PR.

comment:6 by Yuri Shikanov, 6 years ago

Owner: changed from Jake Rothenbuhler to Yuri Shikanov

I'll work on it.

comment:7 by Chris Jerdonek, 6 years ago

Cc: Chris Jerdonek added

comment:8 by Yuri Shikanov, 6 years ago

comment:9 by Yuri Shikanov, 6 years ago

Patch needs improvement: unset

comment:10 by Carlton Gibson, 6 years ago

Patch needs improvement: set

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.

comment:11 by Yuri Shikanov, 6 years ago

Patch needs improvement: unset

comment:12 by Carlton Gibson, 6 years ago

Triage Stage: AcceptedReady for checkin

comment:13 by Tim Graham, 6 years ago

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 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.

comment:14 by Chris Jerdonek, 6 years ago

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.

comment:15 by Yuri Shikanov, 6 years ago

So, should I try to refine current implementation in PR?

comment:16 by Chris Jerdonek, 6 years ago

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).

comment:17 by Yuri Shikanov, 6 years ago

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.

comment:18 by Tim Graham, 6 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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.

comment:19 by Tim Graham, 6 years ago

Component: Core (Management commands)Testing framework
Triage Stage: AcceptedSomeday/Maybe

comment:20 by Rich Jones, 6 years ago

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.

comment:21 by Chris Jerdonek, 6 years ago

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.

comment:22 by Yuri Shikanov, 6 years ago

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.

comment:23 by Chris Jerdonek, 6 years ago

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.

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