<div dir="ltr">Gordon,<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-size:12.8000001907349px">+1. i don't think this would be a good idea as a non-voting job either as it can/will lead to lazy reviews.</span></blockquote><div><br></div><div>It is similar to say let's remove unit/functional/pep8/pylint/"any other testing" because it will lead to lazy reviews. </div><div><br></div><div>Best regards,</div><div>Boris Pavlovic </div><div><br></div><div><br></div><div><br></div><div> </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 20, 2015 at 5:14 PM, gordon chung <span dir="ltr"><<a href="mailto:gord@live.ca" target="_blank">gord@live.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">----------------------------------------<br>
> Date: Mon, 20 Apr 2015 07:13:31 -0400<br>
> From: <a href="mailto:sean@dague.net">sean@dague.net</a><br>
<span class="">> To: <a href="mailto:openstack-dev@lists.openstack.org">openstack-dev@lists.openstack.org</a><br>
> Subject: Re: [openstack-dev] [all][code quality] Voting coverage job (-1 if coverage get worse after patch)<br>
><br>
</span><div><div class="h5">> On 04/18/2015 09:30 PM, Boris Pavlovic wrote:<br>
>> Hi stackers,<br>
>><br>
>> Code coverage is one of the very important metric of overall code<br>
>> quality especially in case of Python. It's quite important to ensure<br>
>> that code is covered fully with well written unit tests.<br>
>><br>
>> One of the nice thing is coverage job.<br>
>><br>
>> In Rally we are running it against every check which allows us to get<br>
>> detailed information about<br>
>> coverage before merging patch:<br>
>> <a href="http://logs.openstack.org/84/175084/1/check/rally-coverage/c61d5e1/cover/" target="_blank">http://logs.openstack.org/84/175084/1/check/rally-coverage/c61d5e1/cover/</a><br>
>><br>
>> This helped Rally core team to automate checking that new/changed code<br>
>> is covered by unit tests and we raised unit test coverage from ~78% to<br>
>> almost 91%.<br>
>><br>
>> But it produces few issues:<br>
>> 1)>9k nitpicking - core reviewers have to put -1 if something is not<br>
>> covered by unit tests<br>
>> 2) core team scaling issues - core team members spend a lot of time just<br>
>> checking that whole code is covered by unit test and leaving messages<br>
>> like this should be covered by unit test<br>
>> 3) not friendly community - it's not nice to get on your code -1 from<br>
>> somebody that is asking just to write unit tests<br>
>> 4) coverage regressions - sometimes we accidentally accept patches that<br>
>> reduce coverage<br>
>><br>
>> To resolve this issue I improved a bit coverage job in Rally project,<br>
>> and now it compares master vs master + patch coverage. If new coverage<br>
>> is less than master job is marked as -1.<br>
>><br>
>> Here is the patch for job enhancement:<br>
>> <a href="https://review.openstack.org/#/c/174645/" target="_blank">https://review.openstack.org/#/c/174645/</a><br>
>><br>
>> Here is coverage job in action:<br>
>> patch <a href="https://review.openstack.org/#/c/174677/" target="_blank">https://review.openstack.org/#/c/174677/</a><br>
>> job message<br>
>> <a href="http://logs.openstack.org/77/174677/4/check/rally-coverage/ba49c90/console.html#_2015-04-17_15_57_17_695" target="_blank">http://logs.openstack.org/77/174677/4/check/rally-coverage/ba49c90/console.html#_2015-04-17_15_57_17_695</a><br>
><br>
> Honestly, I'm suspicious of approaches like this. While 90% coverage is<br>
> clearly better than 0% coverage, it's not clear that 91% coverage is<br>
> always better than 90% coverage. Especially when you talk about larger<br>
> and more complex code bases.<br>
><br>
> I actually firmly feel that #3 is wrong. I think it's a lot better to<br>
> have an early conversation with people about unit tests that need to be<br>
> written when they don't submit any. Because I think it's a lot less<br>
> friendly to have someone iterate 10 patches to figure out how to pass a<br>
> bot's idea of good tests, and then have a reviewer say it's wrong.<br>
><br>
> Honestly, coverage for projects seems to me to be more of an analog<br>
> measure that would be good to graph over time and make sure it's not<br>
> regression, but personally I think the brownian motion of individual<br>
> patches seems like it's not a great idea to gate on every single patch.<br>
> I personally would be -1 for adding this to projects that I have +2 on.<br>
><br>
> -Sean<br>
><br>
> --<br>
> Sean Dague<br>
> <a href="http://dague.net" target="_blank">http://dague.net</a><br>
<br>
</div></div>+1. i don't think this would be a good idea as a non-voting job either as it can/will lead to lazy reviews.<br>
<br>
cheers,<br>
gord<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div></div></blockquote></div><br></div>