Increase apparent test coverage by removing superfluous ellipses

[glatterf42]

Yesterday I learned that one does not need to put any code inside a function that has a docstring. This allows us to remove numerous lines in the abstract layer that just contain ... and are counted by test coverage as missing lines. Removing them should not affect any kind of behaviour or how the docs look, but increase the apparent test coverage :)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.4%. Comparing base (0975ce3) to head (1885f0c).

Additional details and impacted files

@@               Coverage Diff               @@
##           enh/run-clone    #142     +/-   ##
===============================================
+ Coverage           87.1%   88.4%   +1.3%     
===============================================
  Files                231     231             
  Lines               8216    8093    -123     
===============================================
  Hits                7160    7160             
+ Misses              1056     933    -123     

see 19 files with indirect coverage changes

[meksor]

Hmm, so what you are saying does check out. The ellipses is not really strictly required.

from typing import Protocol

class A(Protocol):
    def f(self):
        """This is a docstring"""
        ...

class B(A): ...

B().f()
# ^ Cannot instantiate abstract class "B" with abstract attribute "f"

And alike:

from typing import Protocol

class A(Protocol):
    def f(self):
        """This is a docstring"""

class B(A): ...

B().f()
# ^ Cannot instantiate abstract class "B" with abstract attribute "f"

However as soon as A.fcontains a yield or return the mypy error disappears, so it might be worth leaving these few lines so it doesnt happen accidentally.

[glatterf42]

I'm not sure I understand the problem with this. I created your example in the mypy playground, so please feel free to experiment with that.
The PR only removes ... from functions where this is the only "body" of the function. Whenever a function contains yield or return, it remains unchanged. There is no intersection of yield/return and ... in functions, it's either or. And the error is function-specific, as my playground example shows: B().f() doesn't work even if f() has a return statement if g() is abstract. So with or without the ellipses, one could only call the yield/return functions on child classes that actually provide implementations for the abstract functions.

Please let me know what I'm missing about this.

[meksor]

Im not sure i understand the /benefit/ of this. IMO the ellipsis adds semantic value as in "this space intentionally left blank".
Why are we removing them? To increase "apparent" test coverage !? thats not a priority in my book. So I would not do it.

[glatterf42]

Ideally, as I understand it, we should push for 100% test coverage (and more, since even calling each single line in the tests might not guarantee that it works as intended for various inputs). Codecov also counts lines containing only ... as lines that require testing, so we have a few options of dealing with them in pursuit of the 100% coverage mark:

• We could tell Codecov that all such lines are to be excluded from measuring coverage (similar to if TYPE_CHECKING: lines). This might affect more lines than the ones I removed here, but maybe deservedly so.
• We could come up with some kind of test that calls these lines, but I'm not sure we should. There is no intention to ever call them, I think, so this could take the form of raising NotImplementedErrors when one tries, which we should (and easily could) then test again.
• However, I also learned that these ellipses are not actually required and we could remove them without losing any functionality. I went for this option because it seemed like the cleanest one to me.

We don't have to merge this version, of course. I think an empty function body conveys intent as well as ..., but I can also instead include ellipses from coverage measurements.

[meksor]

Maybe this is a solution that doesn't make us both unhappy: nedbat/coveragepy#1042 (comment)

[glatterf42]

This is essentially the first option from my list :)
Let's see how the coverage reports looks now, swapping in this option while keeping the ellipses. It should reach about 88.1%.

It's 88.4%, so slightly better than expected. From the details view, I can't immediately tell the difference, maybe I forgot a line or two? Either way, the result seems fine :)