terminal: default to fE with -r (reportchars)

[blueyed]

Fixes #6454

[nicoddemus]

LGTM, but we should hear from @The-Compiler and @asottile because they were strongly against this change initially.

[asottile]

I'm still opposed to making the (already bad) situation worse with needing to scroll for error messages worse

I think I'd be less opposed to this change if we reordered the output so that warnings / logging / etc. are above the stacktraces.

I realize that relative to output / warnings this tends to be only a few lines instead of hundreds, though in larger testsuites this still adds an unwieldy amount of output.

[nicoddemus]

Thanks @asottile,

I'm still opposed to making the (already bad) situation worse with needing to scroll for error messages worse

We are still talking about a few lines though. If you don't have any failures, the new default won't show a summary at all. If there are failures, in addition to several lines of tracebacks (hundreds unless using --tb=short) you will also get a 1 line summary per failed test.

I understand your sentiment of "this will make things a bit worse", but I think this is a matter of preference: when you see the current output you think that it is bad, so adding a few lines to the end probably won't make things that much worse for you anyway.

On the other hand it will make things more pleasant and useful for people who prefer to see a short summary of failures at the end (like myself and @blueyed). We already show a lot of information anyway by default, I believe a few more lines won't make it that worse for you.

I also agree with the sentiment in general of having a separate option to obtain a leaner output (#6471).

[asottile]

I would have no opposition to this if it were an option, but I'd like to see pytest's default be less noisy wherever possible (a slim option I don't think actually addresses the problem that pytest's output is a lot of noise and very little signal by default)

[nicoddemus]

I would have no opposition to this if it were an option

It is already an option, you can use -rfE explicitly.

but I'd like to see pytest's default be less noisy wherever possible

I see, but having pytest being less verbose by default is a separate (and more controversial) proposal.

For example, suppose we can give a grade for verbosity, and currently pytest is 8. This proposal is a small increase to that, to say 9. But what you actually want is to reduce verbosity a lot by default, say 2 or 3.

I understand the sentiment, but that's a different take and separate proposal from this one.

[asottile]

if we take that since, the next small change will make it a 10 and then 11 and then 12 -- in my opinion directionally this pr is worse and sets a bad precedent

[nicoddemus]

if we take that since, the next small change will make it a 10 and then 11 and then 12 -- in my opinion directionally this pr is worse and sets a bad precedent

Well I disagree, but I think I've laid all the arguments I had to convince you otherwise, so that's that.

Personally the summary is the first thing I miss once I get more than 1 or 2 failures, so this PR is beneficial in my eyes.

Let's see what others have to say then, if more people feel the same we should then discard the proposal and the PR.

[The-Compiler]

+1 on this PR (other than the thing I've commented on). IMHO it might make the situation better, because you don't necessarily need to scroll through the output anymore when you see a summary of the failures. I was only opposed to making it -ra by default (like the title of #6454 suggested at the time I replied).

[blueyed]

This could also somehow mention that the effective default is wfE without --disable-warnings actually? (#5066)

[nicoddemus]

Good point, that got me thinking and I think -rw should be removed from the docs.

Since we have merged pytest-warnings (which seems like decades ago), -rw is not really a thing, because independently of what you put in -r (except N now in this PR) warnings are always displayed:

No -r option:

λ pytest .tmp\test-reporting.py
...
========================= warnings summary ==========================
test-reporting.py::test_warn
  D:\projects\pytest\.tmp\test-reporting.py:28: UserWarning: some warning
    warnings.warn(UserWarning('some warning'))

-- Docs: https://docs.pytest.org/en/latest/warnings.html
====================== short test summary info ======================
FAILED .tmp\test-reporting.py::test_fail[0] - assert 0
FAILED .tmp\test-reporting.py::test_fail[1] - assert 0
FAILED .tmp\test-reporting.py::test_fail[2] - assert 0
=== 3 failed, 4 passed, 3 skipped, 3 xfailed, 1 warning in 0.11s ====

Some -r combination (note the missing w):

λ pytest .tmp\test-reporting.py -rXfs
...
========================= warnings summary ==========================
test-reporting.py::test_warn
  D:\projects\pytest\.tmp\test-reporting.py:28: UserWarning: some warning
    warnings.warn(UserWarning('some warning'))

-- Docs: https://docs.pytest.org/en/latest/warnings.html
====================== short test summary info ======================
FAILED .tmp\test-reporting.py::test_fail[0] - assert 0
FAILED .tmp\test-reporting.py::test_fail[1] - assert 0
FAILED .tmp\test-reporting.py::test_fail[2] - assert 0
SKIPPED [3] test-reporting.py:15: nas
=== 3 failed, 4 passed, 3 skipped, 3 xfailed, 1 warning in 0.12s ====

So the only way to skip the warnings summary section is by using either -rN or --disable-warnings, as of now.

Warnings being controlled in -r is historical left-over, so much that warnings summary and short test summary are separate today.

I propose then:

• Remove -rw from the docs, given it doesn't do anything (or mention that it is there for historical reasons but has no effect).
• Make -rN not affect warnings at all, warnings can already be disabled with --disable-warnings. No backward compatibility here to consider because N is new, and the only way to hide warnings is with --disable-warnings anyway. This also makes using -rN produce the exact same behavior as before: hide short test summary, but warnings summary is still shown.

[blueyed]

@nicoddemus have you looked at #5066 again?
There we wanted something like -r-w and remove/deprecate the option completely even.

[nicoddemus]

Ahh I didn't, even thought you mentioned it, my bad.

I've re-read that thread, and I still think what I wrote above holds better, specially because warnings and short summaries are reported and dealt separately. It also means we wouldn't need to deprecate anything really.

[bluetech]

I still would have preferred X (xpassed) to be included, however seems others disagree, due to some test suites abusing(?) it. So 👍 from me (I didn't review the code).

[blueyed]

I still would have preferred X (xpassed) to be included

Good point.

however seems others disagree

I think it makes sense also.

due to some test suites abusing(?) it.

IIRC I've found a reference to it somewhere and just mentioned it (#6454 (comment)).
AFAICS nobody disagrees?

[nicoddemus]

So 👍 from me (I didn't review the code).

Feel free to approve the PR then. 😁

AFAICS nobody disagrees?

Perhaps we should be conservative and just stick with fE for now?

[blueyed]

So +1 from me (I didn't review the code).

Feel free to approve the PR then.

Without an actual review? 😕

AFAICS nobody disagrees?

Perhaps we should be conservative and just stick with fE for now?

Yeah, ok - needs another pass to address your feedback anyway, so I've thought it could go in still also. But we can create a followup issue for it then.

[blueyed]

Amended for N to not reset w.
Moved handling of w to the end also, since "warnings and short summaries are reported and dealt separately" according to #6524 (comment).

This also changes the weird behavior that -ra and -rA would previously include warnings even though --disable-warnings was used.

[blueyed]

This also changes the weird behavior that -ra and -rA would previously include warnings even though --disable-warnings was used.

Might be worth a separate changelog entry?

[nicoddemus]

Might be worth a separate changelog entry?

Sounds good, but you might just use the same entry and just add another paragraph (your call). 👍

[blueyed]

Added changelog/6454.bugfix.rst.

[blueyed]

Will merge once CI passes.