Opened 5 years ago

Last modified 8 months ago

#22449 assigned New feature

Colorize output of test results

Reported by: Jake Rothenbuhler Owned by: Yuri Shikanov
Component: Testing framework Version: master
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 Changed 5 years ago by Jake Rothenbuhler

Owner: changed from nobody to Jake Rothenbuhler
Status: newassigned

comment:2 Changed 5 years ago by Jake Rothenbuhler

Has patch: set

comment:3 Changed 5 years ago by Baptiste Mispelon

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 Changed 5 years ago by Jake Rothenbuhler

Patch needs improvement: unset

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

comment:5 Changed 4 years ago by Tim Graham

Patch needs improvement: set

I added some more comments for improvement on the PR.

comment:6 Changed 9 months ago by Yuri Shikanov

Owner: changed from Jake Rothenbuhler to Yuri Shikanov

I'll work on it.

comment:7 Changed 9 months ago by Chris Jerdonek

Cc: Chris Jerdonek added

comment:8 Changed 8 months ago by Yuri Shikanov

comment:9 Changed 8 months ago by Yuri Shikanov

Patch needs improvement: unset

comment:10 Changed 8 months ago by Carlton Gibson

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 Changed 8 months ago by Yuri Shikanov

Patch needs improvement: unset

comment:12 Changed 8 months ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:13 Changed 8 months ago 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 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 Changed 8 months ago 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.

comment:15 Changed 8 months ago by Yuri Shikanov

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

comment:16 Changed 8 months ago 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).

comment:17 Changed 8 months ago 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.

comment:18 Changed 8 months ago by Tim Graham

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 Changed 8 months ago by Tim Graham

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

comment:20 Changed 8 months ago 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.

comment:21 Changed 8 months ago 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.

comment:22 Changed 8 months ago 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.

comment:23 Changed 8 months ago 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.

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