[Twisted-Python] Test coverage requirements

[Adi Roiban]

On 26 February 2017 at 23:51, Jean-Paul Calderone
<exarkun at twistedmatrix.com> wrote:
> Hi,
>
> I'm looking at some recent trunk commits (also, others) that seem to have
> non-trivial untested code at at ReviewProcess.  I can't tell if the codecov
> reports are wrong or if the development process documentation is wrong or if
> the commits just violate policy or (I guess) some mix of the three.
>
> Can anyone shed any light on this?

Besise https://codecov.io/gh/twisted/twisted/pulls/509/diff, the other
PRs looks OK and with good coverage.

In terms of codecov.io coverage I am using the "diff" view as I find
it less confusing.

The strong red lines are the ones which codecov considered that were
part of the patch and were not covered.
The yellow lines are for missing branch coverage in this patch... and
they will decrease the coverage percentage.

In summary:

For PR 509 - The diff coverage is 74.39%. ... this looks bad
For PR 652 -  The diff coverage is 98.7% ... not perfect, but pretty
good as the missing lines are in the test code area :)

--------

At some point we had a commit status check for 100% diff coverage, but
I remember that it was not popular.

If we are not aiming for the ultimate quality dev process we can still
consider having a lower barer of something like 75% or 95% diff
coverage... For PR less of this value, you will see a big warning...
but people from the Twisted admin team can still commit them. Simple
(non-admin) collaborators can only merge them of the diff coverage is
above that limit.

I am for a check for 100% coverage as many times it helped me get out
of comfort zone and "forced" to write better tests  and better code
... I know that libertarians might not like being forced to do things
so I understand why the hard check was not popular.

----------

For the good PRs which were mentioned here

For https://codecov.io/gh/twisted/twisted/pulls/695/diff the only line
without a coverage is the one in which the exception is raised if the
reactor is installed twice.

For https://codecov.io/gh/twisted/twisted/pulls/652/diff the uncovered
lines are the case in which a tests is is not skipped on OSX (this is
strange ... but maybe osx coverage are no longer published) ... and
the other one is the self.fail which is expected not be be called.

I think there was a discussion about covering self.fail() calls by
extracting them into assertion helper and making sure we have tests
for those assertion helpers.... so that we can ask all changes to also
have 100% test code coverage :)

For https://codecov.io/gh/twisted/twisted/pulls/432/diff there were
some changes in formatting (some parenthesis) which were indented and
that code was not previously covered ... so now it is considered code
on which someone made a change without having the tests to cover the
changes.

-- 
Adi Roiban