Esiegerman/summary colors by esiegerman · Pull Request #776 · pytest-dev/pytest

esiegerman · 2015-06-15T23:58:38Z

Recreating this PR on Github. No change from the Bitbucket version, except that it's been rebased against master -- so there's nothing new to review until I update it.

nicoddemus · 2015-06-16T00:27:29Z

Thanks, taking the liberty to add a link to the original PR: https://bitbucket.org/pytest-dev/pytest/pull-request/304

bubenkoff · 2015-06-17T11:31:25Z

testing/test_terminal.py

pls use pep8

Re. the spacing, you mean? Will do. (A previous change of mine was accepted looking like this, so I did the same this time.)

yes spacing, it's not per pep8
not sure why new code should break pep8

Well, because, as PEP8 itself says:

Some other good reasons to ignore a particular guideline:

When applying the guideline would make the code less readable, even for someone who is used to reading code that follows this PEP.

IMO, code that's essentially a big table is more readable when it's formatted as such.

I wasn't sure what pytest's conventions are, so I submitted the code in the format I prefer. If that isn't what pytest prefers, I'm happy to change it -- but I'd rather not do that until I'm finished with it, i.e. until everything else about the PR has been approved.

nicoddemus · 2015-06-18T22:12:54Z

I'm 👍 on the idea behind this PR, btw. 😄

nicoddemus · 2015-06-18T22:13:13Z

testing/test_terminal.py

Shouldn't this be "yellow"?

esiegerman · 2015-06-19T17:14:06Z

Propagating comments from Bitbucket...

@hpk42 wrote:

the practical reason for xpass not causing red is that xfail is somewhat often used for flaky tests.

To which I responded:

Fair enough. I haven't used it that way -- only for hard failures -- but I see your point. (Indeed, I have a couple of tests that this would make sense for.)

How would you feel about a way to distinguish intermittent/flaky tests from those that are expected to fail every time (e.g. tests for bugs that haven't been fixed yet). Could do this either as an argument to xfail, or as a new marker entirely. My preference would be the latter; known-broken test and flaky test feel to me like different things, rather than variations of the same thing.

If the idea works for you, I'll dive deeper into pytest to implement it.

In the meantime, I agree; if people are using xfail for flaky tests, red is too serious for xpassed. I can justify arguments for any of yellow, green, or "boring" (i.e. has no effect on the colour). What's your preference?

If folks like the idea of separate markers for "fails intermittently" vs. "fails always":

Which of those should xfail (continue to) be used for? The documentation says it's for tests that are "expected to fail", which I take to mean "fails always". But if in practice it's used more often for tests that fail intermittently, that could be an argument for redesignating xfail as such and updating the docs accordingly
I'd appreciate suggestions for naming the other one

The-Compiler · 2015-06-19T19:26:52Z

I agree there should be some difference between "is expected to fail" and "is flaky".

I personally use xfail for "I found a bug while writing tests, or found a bug and wrote a test for it - but I don't want to fix it immediately, and I don't want my CI failing because of it". (I don't have any flaky tests, fortunately)

IMHO, xpassed should be red, and a new pytest.mark.flaky marker should be added - then it's also immediately clear for someone reading the tests that this test is flaky, not expected to fail.

nicoddemus · 2015-06-19T21:30:01Z

IMHO, xpassed should be red, and a new pytest.mark.flaky marker should be added - then it's also immediately clear for someone reading the tests that this test is flaky, not expected to fail.

I like this idea... I have used xfail exclusively for flaky tests currently, but a flaky mark would be more appropriate.

Backward compatibility aside: Supposing then that we have two different marks, xfail and flaky, what would happen if an xfail test started to pass? That should count as a failure I think, because that might be a red flag (no pun intended) that something is amiss, or the expected failure was actually fixed so the test can have the xfail mark removed.

Backward compatibility back on: unfortunately we can't change the current meaning of xfail regarding test suite failure/success, so my proposal in the previous paragraph wouldn't be possible.

Perhaps a new mark is out of the scope of this PR, and we should just agree that xfail as it stands right now actually mean both "will always fail because of reasons" and "flaky", and decide which color to assign to it? I vote for yellow. 😁

esiegerman · 2015-06-19T23:05:14Z

Perhaps a new mark is out of the scope of this PR

Agreed.

I think changing xpass/xfail behaviour is also out of scope. I thought that making xpass red was a trivial change -- didn't realize the larger issues involved. So I'll factor that out of this PR.

esiegerman · 2015-07-01T00:17:27Z

[ OK, I've pretty much started from scratch. I wasn't sure how this project feels about git push -f on a still-being-reviewed PR branch, so I git reverted my old commits instead. That can be cleaned up prior to merging. ]

In this version, xpass is treated the same as xfailed, deselected and skip: it's ignored when deciding what color the summary bar should be. Thus, if you have "5 passed, 3 xfailed, 1 xpassed", you get a green bar, but if you only have "3 xfailed, 1 xpassed", you get yellow. Rationale: since the meaning of xpass is indeterminate (see earlier discussion in this PR), there's no obvious correct color to give it -- green is no better than red, since either one might be a lie.

I've also backed the implementation off to the old one, much more lightly modified, at @hpk42's request.

I have not yet PEP8'ified the test cases. I'll do that just before merge; as long as we're still hashing things out, I find the nicely columnar format a lot easier to work with.

nicoddemus · 2015-07-01T02:53:50Z

OK, I've pretty much started from scratch. I wasn't sure how this project feels about git push -f on a still-being-reviewed PR branch, so I git reverted my old commits instead. That can be cleaned up prior to merging.

We have not discussed this in detail, but I think it is OK if you're the only person working on the branch. But please do rebase it/clean up the commits once we are ready to merge. 😄

I have not yet PEP8'ified the test cases. I'll do that just before merge; as long as we're still hashing things out, I find the nicely columnar format a lot easier to work with.

Seems fair enough to me! 😄

nicoddemus · 2015-07-01T03:10:22Z

Looks 👍 to me. 😄

The-Compiler · 2015-07-01T08:41:54Z

Looks good to me as well!

--HG-- branch : esiegerman/summary_colors

This makes it easier to identify failing tests.

Check for the empty-key special case in the first loop, not the second.

Also if we see any statuses the code doesn't know about.

Passing tests override that default, making the color green; but several other "boring" statuses (xfailed, xpassed, deselected, skipped) have no effect. Net effect: if only "boring" tests are seen, or no tests at all, the summary bar is yellow.

esiegerman · 2015-07-02T17:42:56Z

Junk commits removed; remaining commits slightly reorganized; PEP8ified; rebased. No functional change.

nicoddemus · 2015-07-02T17:53:44Z

Many thanks @esiegerman! 😄

One last request (sorry for not mentioning it earlier): Please add yourself to AUTHORS and add a note to the CHANGELOG describing this change.

I will merge this shortly if no one opposes this.

esiegerman · 2015-07-02T19:51:15Z

Done. I've called it only a partial fix for #500, as that issue's OP asked in a comment for an explicit warning message (as well as the color change).

nicoddemus mentioned this pull request Jun 16, 2015

Complete move to GitHub #769

Closed

6 tasks

bubenkoff reviewed Jun 17, 2015
View reviewed changes

nicoddemus reviewed Jun 18, 2015
View reviewed changes

testing/test_terminal.py Outdated

Copy link

Member

nicoddemus Jun 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be "yellow"?

esiegerman force-pushed the esiegerman/summary_colors branch from 5493562 to 7b989e6 Compare July 2, 2015 17:33

Eric Siegerman added 8 commits July 2, 2015 13:39

Factor out build_summary_stats_line(), and add tests

bfc3e48

--HG-- branch : esiegerman/summary_colors

test_summary_stats() now prints its parameter values

7993afa

This makes it easier to identify failing tests.

Add tests for the empty-key special case

e07144a

Refactor slightly

0282da9

Check for the empty-key special case in the first loop, not the second.

Add missing "error" status to the list

cb21d84

If there are warnings, make the status bar yellow

044d874

Also if we see any statuses the code doesn't know about.

Default color is now yellow

6c395cb

Passing tests override that default, making the color green; but several other "boring" statuses (xfailed, xpassed, deselected, skipped) have no effect. Net effect: if only "boring" tests are seen, or no tests at all, the summary bar is yellow.

PEP8ify parametrized-test parameters

afcad74

esiegerman force-pushed the esiegerman/summary_colors branch from 7b989e6 to afcad74 Compare July 2, 2015 17:41

Add CHANGELOG and AUTHORS entries

2c419c4

The-Compiler mentioned this pull request Jul 3, 2015

Mark for flaky tests #814

Closed

Uh oh!

Conversation

esiegerman commented Jun 15, 2015

Uh oh!

nicoddemus commented Jun 16, 2015

Uh oh!

bubenkoff Jun 17, 2015

Choose a reason for hiding this comment

Uh oh!

esiegerman Jun 19, 2015

Choose a reason for hiding this comment

Uh oh!

bubenkoff Jun 23, 2015

Choose a reason for hiding this comment

Uh oh!

esiegerman Jul 1, 2015

Choose a reason for hiding this comment

Uh oh!

nicoddemus commented Jun 18, 2015

Uh oh!

nicoddemus Jun 18, 2015

Choose a reason for hiding this comment

Uh oh!

esiegerman commented Jun 19, 2015

Uh oh!

The-Compiler commented Jun 19, 2015

Uh oh!

nicoddemus commented Jun 19, 2015

Uh oh!

esiegerman commented Jun 19, 2015

Uh oh!

esiegerman commented Jul 1, 2015

Uh oh!

nicoddemus commented Jul 1, 2015

Uh oh!

nicoddemus commented Jul 1, 2015

Uh oh!

The-Compiler commented Jul 1, 2015

Uh oh!

esiegerman commented Jul 2, 2015

Uh oh!

nicoddemus commented Jul 2, 2015

Uh oh!

esiegerman commented Jul 2, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants