On 26 February 2017 at 23:51, Jean-Paul Calderone <exarkun@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