<div dir="ltr">Morgan,<div><br></div><div>Thank you for your input. I improved coverage job in this patch: </div><div><a href="https://review.openstack.org/#/c/175557/1">https://review.openstack.org/#/c/175557/1</a><br></div><div><br></div><div>Now: </div><div>* It is based on missing lines and not coverage percentage. <br></div><div>* It shows nice messages and coverage diffs: <br></div><div><br></div><font face="monospace, monospace" color="#666666"> Allowed to introduce missing lines : 8<br> Missing lines in master : 649<br> Missing lines in proposed change : 669<br><br> Name Stmts Miss Branch BrMiss Cover<br> @@ -43 +43 @@<br> -rally/benchmark/processing/utils 52 0 30 1 98.780%<br> +rally/benchmark/processing/utils 52 20 30 15 57.317%<br> @@ -190 +190 @@<br> -TOTAL 9400 649 2284 394 91.073%<br> +TOTAL 9400 669 2284 408 90.782%<br><br> Please write more unit tests, we should keep our test coverage :( </font><div><br></div><div><br></div><div>Best regards,</div><div>Boris Pavlovic </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 20, 2015 at 9:11 PM, Hayes, Graham <span dir="ltr"><<a href="mailto:graham.hayes@hp.com" target="_blank">graham.hayes@hp.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 20/04/15 18:01, Clint Byrum wrote:<br>
> Excerpts from Boris Pavlovic's message of 2015-04-18 18:30:02 -0700:<br>
>> Hi stackers,<br>
>><br>
>> Code coverage is one of the very important metric of overall code quality<br>
>> especially in case of Python. It's quite important to ensure that code is<br>
>> 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 is<br>
>> covered by unit tests and we raised unit test coverage from ~78% to almost<br>
>> 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 like<br>
>> 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, and<br>
>> now it compares master vs master + patch coverage. If new coverage is less<br>
>> 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>
><br>
> The link to the important line was key, because without it, just clicking<br>
> through from the review was incomprehensible to me. Can I suggest some<br>
> whitespace or bordering so we can see where the error is easily?<br>
><br>
> Anyway, interesting thoughts from everyone. I have to agree with those<br>
> that say this isn't reliable enough to make it vote. Non-voting would be<br>
> interesting though, if it gave a clear score difference, and a diff of<br>
> the two coverage reports. I think this is more useful as an automated<br>
> pointer to how things probably should be, but sometimes it's entirely<br>
> o-k to regress this number a few points.<br>
><br>
> Also graphing this over time in a post-commit job seems like a no-brainer.<br>
<br>
<br>
</div></div>Designate has started doing this - it is still a WIP as we continue<br>
tweaking settings, but we have a dashboard here -<br>
<br>
<a href="http://sonar.designate-ci.com/" target="_blank">http://sonar.designate-ci.com/</a><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>
><br>
<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>