Display "short test summary info" after (main) warnings again #4399

blueyed · 2018-11-15T16:40:44Z

blueyed · 2018-11-15T16:51:07Z

src/_pytest/terminal.py

        self.summary_warnings()
+        yield


"short test summary info" comes via the skipping plugin, which I found rather confusing.
Should/can this be moved?

It would also might make sense to have "short test summary info" after "summary_passes" even, but I wanted to not touch that behavior (and am not using it - -ra does not include P).

Hmmm if we move it to pytest_terminal_summary, will it simplify things? It doesn't look so simple to move it, because there's some logic in there to handle summaries for xpass, xfail, etc.

Yes, it is quite messy.

Perhaps we should add a comment here explaining why we display warnings twice, what do you think?

Makes sense.

nicoddemus · 2018-11-22T23:07:53Z

src/_pytest/terminal.py

@@ -726,11 +727,19 @@ def summary_warnings(self):
            if not all_warnings:
                return

+            final = hasattr(self, "_already_displayed_warnings")


Hmm I'm not sure, I understand the rationale for the change, but I'm not so happy with how this turns out... 🤔

I've tried to use args for this first, but considered that there are only 2 steps and an attribute/marker can do it.

I've tried to keep the patch minimal.

blueyed · 2018-11-22T23:24:50Z

@nicoddemus
Thanks for reviewing this anyway already! :)

nicoddemus

This solves a real problem and I don't see a clearer way to implement this, so I'm 👍 for the change, thanks!

Would like a second eyes on it before we merge it though.

nicoddemus · 2018-11-23T18:55:26Z

src/_pytest/terminal.py

        self.summary_warnings()
+        yield


Perhaps we should add a comment here explaining why we display warnings twice, what do you think?

Fixes pytest-dev#3952.

blueyed · 2018-11-23T19:07:32Z

Added the comment.

(normally I would have pushed this as a fixup with [ci skip], and then it could have been squashed merged, but yeah.. - full CI run now for this..)

codecov · 2018-11-23T19:08:15Z

Codecov Report

Merging #4399 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4399      +/-   ##
==========================================
+ Coverage   95.87%   95.92%   +0.04%     
==========================================
  Files         111      111              
  Lines       25061    25001      -60     
  Branches     2445     2440       -5     
==========================================
- Hits        24028    23981      -47     
+ Misses        726      723       -3     
+ Partials      307      297      -10

Flag	Coverage Δ
#docs	`29.93% <16.66%> (-0.13%)`	⬇️
#doctesting	`29.93% <16.66%> (-0.13%)`	⬇️
#linting	`29.93% <16.66%> (-0.13%)`	⬇️
#linux	`95.75% <100%> (+0.04%)`	⬆️
#nobyte	`92.35% <100%> (-0.31%)`	⬇️
#numpy	`93.13% <100%> (-0.3%)`	⬇️
#pexpect	`41.79% <100%> (-0.02%)`	⬇️
#py27	`94.05% <100%> (+0.04%)`	⬆️
#py34	`92.17% <100%> (+0.06%)`	⬆️
#py35	`92.18% <100%> (+0.05%)`	⬆️
#py36	`92.2% <100%> (+0.05%)`	⬆️
#py37	`94.12% <100%> (+0.03%)`	⬆️
#trial	`93.13% <100%> (-0.3%)`	⬇️
#windows	`94.11% <100%> (-0.01%)`	⬇️
#xdist	`93.95% <100%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
src/_pytest/terminal.py	`93.1% <100%> (+1.11%)`	⬆️
testing/test_terminal.py	`100% <100%> (+0.19%)`	⬆️
testing/python/raises.py	`93.1% <0%> (-0.5%)`	⬇️
src/_pytest/monkeypatch.py	`93.75% <0%> (-0.13%)`	⬇️
testing/test_tmpdir.py	`98.75% <0%> (-0.12%)`	⬇️
testing/test_assertrewrite.py	`83.38% <0%> (-0.08%)`	⬇️
src/_pytest/_code/code.py	`95.42% <0%> (-0.05%)`	⬇️
src/_pytest/pytester.py	`87.41% <0%> (-0.04%)`	⬇️
testing/test_session.py	`96.5% <0%> (-0.03%)`	⬇️
testing/test_cacheprovider.py	`99.71% <0%> (-0.02%)`	⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b73d6d...0cf45ee. Read the comment docs.

RonnyPfannschmidt

how and where to communicate the details is indeed up to taste and api stability

we can think about writing this cleaner as a followup - the decomposition wont be directly oblivious as its indeed a tricky case

nicoddemus · 2018-11-23T20:06:11Z

I will get this merged now as we know it has passed in the previous commit, and the only change has been a comment. 😁

Thanks @blueyed!

nicoddemus · 2018-11-23T21:28:10Z

src/_pytest/terminal.py

+            else:
+                warnings = all_warnings
+            self._already_displayed_warnings = len(warnings)
+


We should move the conditional from the top of the method to this point:

if not warnings: return

That's why we are seeing double warnings summary even if the "final" summary has no warnings to show. 👍

Ref: pytest-dev#4450 (comment) Ref: pytest-dev#4399

blueyed commented Nov 15, 2018

View reviewed changes

This was referenced Nov 16, 2018

Move 'short test summary info' to the end of the output again (regression) #3952

Closed

Move warnings summary above short test summary. #3956

Closed

nicoddemus reviewed Nov 22, 2018

View reviewed changes

nicoddemus approved these changes Nov 23, 2018

View reviewed changes

Display "short test summary info" after (main) warnings again

0cf45ee

Fixes pytest-dev#3952.

blueyed force-pushed the summary branch from 4cf20aa to 0cf45ee Compare November 23, 2018 19:06

RonnyPfannschmidt approved these changes Nov 23, 2018

View reviewed changes

nicoddemus mentioned this pull request Nov 23, 2018

Release version 4.0.1 #4449

Closed

nicoddemus merged commit 23e4447 into pytest-dev:master Nov 23, 2018

nicoddemus mentioned this pull request Nov 23, 2018

Release version 4.0.1 #4450

Merged

nicoddemus reviewed Nov 23, 2018

View reviewed changes

blueyed added a commit to nicoddemus/pytest that referenced this pull request Nov 23, 2018

Fix warnings summary header appearing twice

0bfc1a7

Ref: pytest-dev#4450 (comment) Ref: pytest-dev#4399

blueyed added a commit to nicoddemus/pytest that referenced this pull request Nov 23, 2018

Fix warnings summary header appearing twice

be3b8fc

Ref: pytest-dev#4450 (comment) Ref: pytest-dev#4399

blueyed deleted the summary branch April 7, 2019 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display "short test summary info" after (main) warnings again #4399

Display "short test summary info" after (main) warnings again #4399

blueyed commented Nov 15, 2018 •

edited

Loading

blueyed Nov 15, 2018

nicoddemus Nov 22, 2018

blueyed Nov 22, 2018

nicoddemus Nov 23, 2018

blueyed Nov 23, 2018

nicoddemus Nov 22, 2018

blueyed Nov 22, 2018

blueyed commented Nov 22, 2018

nicoddemus left a comment

nicoddemus Nov 23, 2018

blueyed commented Nov 23, 2018

codecov bot commented Nov 23, 2018

RonnyPfannschmidt left a comment

nicoddemus commented Nov 23, 2018

nicoddemus Nov 23, 2018

Display "short test summary info" after (main) warnings again #4399

Display "short test summary info" after (main) warnings again #4399

Conversation

blueyed commented Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueyed commented Nov 22, 2018

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueyed commented Nov 23, 2018

codecov bot commented Nov 23, 2018

Codecov Report

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

nicoddemus commented Nov 23, 2018

Choose a reason for hiding this comment

blueyed commented Nov 15, 2018 •

edited

Loading