Also record coverage for tests by asottile · Pull Request #3875 · pytest-dev/pytest

asottile · 2018-08-25T22:25:44Z

As seen in #3873, there's sneaky dead code lurking in ./testing. Let's surface that a little bit better.

I started combing through the results but didn't get terribly far.

blueyed · 2018-08-25T22:57:37Z

I've picked up #2800 again.
I would say to wait with this here until it is in.

I think we should also add branch=1 separately, and I wanted to do it after #2800 to have better reporting for what it changes.

coveralls · 2018-08-25T22:59:35Z

Coverage increased (+1.2%) to 93.812% when pulling 3bb4d8a on asottile:coverage_for_tests into a319674 on pytest-dev:master.

blueyed · 2018-08-25T23:01:56Z

.coveragerc

@@ -1,4 +1,26 @@
 [run]
+branch = True
+source = .


Better use _pytest,testing here - less to omit then.

hmm but then you have to have a special snowflake .coveragerc for each project

?

I think it is OK for a project to have its own sources defined here - after all you would then have special omits instead.

🤷‍♂️ no special omits here -- I copied this from one of my projects

Still different than "defaults", no? :)

blueyed · 2018-08-25T23:03:24Z

.coveragerc

+    ^if __name__ == ['"]__main__['"]:$
+
+    # Intentionally skipped tests
+    ^\s*@pytest\.mark\.skipif


@pytest\.mark\.skipif should not be excluded: one of the builds should hit it.
(more relevant with/after #2800)

I don't see a reason to block on #2800 -- until then this improves local runs of isolated environments.

Yes, it is not blocked by #2800, but with #2800 you would not have added it in the first place, would you? (because it should be covered then, and if not it would mean no test setup is hitting it).

blueyed · 2018-08-25T23:04:21Z

src/_pytest/__init__.py

+except ImportError:  # pragma: no cover
    # broken installation, we don't even try
    # unknown only works because we do poor mans version compare
    __version__ = "unknown"


Better leave this code as uncovered.
We're not trying to hit 100%, but is is OK for this to be not covered - also for other things below.

We're not trying to hit 100%

We're not? 😆

Confirmation that, in fact, we are:

#2800 (review)

Ultimately we should strive to obtain 100% coverage...

/cc @nicoddemus

I think the crucial part from there is:

Even if we "cheat", we gain the benefit of demanding that new code from now on has 100% coverage.

This should be achieved instead by requiring a diff to be covered 100% - something which codecov provides then.

Sure, it is ok to add some # pragma: no cover here and there for code that we can't really cover on tests in a practical manner, here is a prime example of that: this is just a safe guard for broken pytest installations.

blueyed · 2018-08-25T23:04:53Z

src/_pytest/_argcomplete.py



-if os.environ.get("_ARGCOMPLETE"):
+if os.environ.get("_ARGCOMPLETE"):  # pragma: no cover


This one e.g. would be good to get covered in the long run.

Don't you think this could be covered in the long run?

blueyed

I think this should be trimmed down to only expanding sources to _pytest,testing.
And it should get rebased on master after we have basic codecov, via #3877.

nicoddemus · 2018-08-26T12:18:32Z

I think this should be trimmed down to only expanding sources to _pytest,testing.

Agree 👍

asottile · 2018-08-26T22:36:43Z

I've rebased and set source = _pytest,testing

nicoddemus

Awesome, thanks @asottile!

asottile · 2018-08-26T23:16:55Z

@blueyed -- I'll defer to your review -- let me know if/when this is mergeable!

codecov · 2018-08-27T02:29:59Z

Codecov Report

Merging #3875 into master will decrease coverage by 0.36%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3875      +/-   ##
==========================================
- Coverage   92.59%   92.22%   -0.37%     
==========================================
  Files          51      104      +53     
  Lines        9960    23189   +13229     
  Branches        0     2285    +2285     
==========================================
+ Hits         9222    21386   +12164     
- Misses        738     1436     +698     
- Partials        0      367     +367

Impacted Files	Coverage Δ
src/_pytest/__init__.py	`100% <ø> (+40%)`	⬆️
src/_pytest/_code/code.py	`92.47% <ø> (-1.27%)`	⬇️
src/_pytest/_argcomplete.py	`95.83% <ø> (+15.83%)`	⬆️
testing/test_skipping.py	`97.84% <ø> (ø)`
src/_pytest/assertion/__init__.py	`86.36% <0%> (-10.61%)`	⬇️
src/_pytest/pastebin.py	`85.48% <0%> (-8.07%)`	⬇️
src/_pytest/warnings.py	`70.17% <0%> (-7.02%)`	⬇️
src/_pytest/assertion/truncate.py	`92.15% <0%> (-5.89%)`	⬇️
src/_pytest/config/argparsing.py	`87.78% <0%> (-5.89%)`	⬇️
src/_pytest/debugging.py	`55.81% <0%> (-4.66%)`	⬇️
... and 88 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 a319674...3bb4d8a. Read the comment docs.

blueyed · 2018-08-27T17:00:30Z

@asottile
I've meant to only change source, nothing more.
This would then show which tests are not covered.
The next step would then be adding branch = 1 (maybe also in this PR), but nothing more.

asottile · 2018-08-27T17:08:48Z

what's the reason?

blueyed · 2018-08-27T21:41:59Z

@asottile
I would rather keep it simpler - but that's just my opinion.

asottile · 2018-08-27T21:55:15Z

I'm sorry, can you highlight what portions of the PR you're in opposition to and your rationale for opposing them? I'm not sure I'm understanding :(

blueyed

@asottile
Sorry for coming across negatively.

I think we should get #2800 in first, then a separate PR adding branch = 1 to clearly see what it changes, and then we can discuss this one further.
In general I think we should not use # pragma: no cover at first.

blueyed · 2018-08-29T20:59:30Z

src/_pytest/_argcomplete.py



-if os.environ.get("_ARGCOMPLETE"):
+if os.environ.get("_ARGCOMPLETE"):  # pragma: no cover


Don't you think this could be covered in the long run?

blueyed · 2018-08-29T20:59:43Z

src/_pytest/_argcomplete.py



-else:
+else:  # pragma: no cover


nicoddemus · 2018-09-01T14:55:36Z

I gather that now that #3920 has been merged we can pick this up again?

asottile · 2018-09-01T15:21:08Z

I guess there's no point now that @blueyed cherry picked all the parts out

asottile mentioned this pull request Aug 25, 2018

Remove dangerous sys.path manipulations in test_pluginmanager #3873

Merged

blueyed reviewed Aug 25, 2018

View reviewed changes

blueyed suggested changes Aug 26, 2018

View reviewed changes

Also record coverage for tests

3bb4d8a

nicoddemus approved these changes Aug 26, 2018

View reviewed changes

asottile mentioned this pull request Aug 26, 2018

Improve the coverage of testing/code #3887

Merged

blueyed reviewed Aug 29, 2018

View reviewed changes

asottile closed this Sep 1, 2018

asottile deleted the coverage_for_tests branch September 1, 2018 15:21



		if os.environ.get("_ARGCOMPLETE"):
		if os.environ.get("_ARGCOMPLETE"): # pragma: no cover



		else:
		else: # pragma: no cover

Uh oh!

Conversation

asottile commented Aug 25, 2018

Uh oh!

blueyed commented Aug 25, 2018

Uh oh!

coveralls commented Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueyed left a comment

Choose a reason for hiding this comment

Uh oh!

nicoddemus commented Aug 26, 2018

Uh oh!

asottile commented Aug 26, 2018

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

asottile commented Aug 26, 2018

Uh oh!

codecov bot commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueyed commented Aug 27, 2018

Uh oh!

asottile commented Aug 27, 2018

Uh oh!

blueyed commented Aug 27, 2018

Uh oh!

asottile commented Aug 27, 2018

Uh oh!

blueyed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoddemus commented Sep 1, 2018

Uh oh!

asottile commented Sep 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

coveralls commented Aug 25, 2018 •

edited

Loading

codecov bot commented Aug 27, 2018 •

edited

Loading