<div dir="ltr">Morgan, <div><br></div><div><br></div><div>Good catch. This can be easily fixed if we add special tag in commit message: e.g. #no-coverage-check </div><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 Sun, Apr 19, 2015 at 9:33 AM, Morgan Fainberg <span dir="ltr"><<a href="mailto:morgan.fainberg@gmail.com" target="_blank">morgan.fainberg@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto"><div>This is an interesting idea, but just a note on implementation:</div><div><br></div><div>It is absolutely possible to reduce the % of coverage without losing (or even gaining) coverage of the code base. This can occur if deprecated code is removed and no new unit tests are added. Overall % of code covered by tests can decline since covered code has been removed with its unit tests, and non-covered code remains the same. In reality, coverage has not changed (or has improved). It is simply a limitation in going purely by % of code covered.  </div><div><br></div><div>I suggest that this move towards checking for classes/methods and make sure that coverage for those do not change between the two runs. If a given method or class has 100% coverage prior to a patch set, and in the new revision, it drops to less than 100% it should at least flag that there was a change in coverage. If a class, function, or method is removed it won't be in the new coverage report, and should not impact the test. </div><div><br></div><div>--Morgan</div><div><br></div><div>Sent via mobile</div><div><div class="h5"><div><br>On Apr 18, 2015, at 18:30, Boris Pavlovic <<a href="mailto:boris@pavlovic.me" target="_blank">boris@pavlovic.me</a>> wrote:<br><br></div><blockquote type="cite"><div><div dir="ltr"><div>Hi stackers, </div><div><br></div><div>Code coverage is one of the very important metric of overall code quality especially in case of Python. It's quite important to ensure that code is covered fully with well written unit tests. </div><div><br></div><div>One of the nice thing is coverage job. </div><div><br></div><div>In Rally we are running it against every check which allows us to get detailed information about</div><div>coverage before merging patch: </div><div><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></div><div><br></div><div>This helped Rally core team to automate checking that new/changed code is covered by unit tests and we raised unit test coverage from ~78% to almost 91%. </div><div><br></div><div>But it produces few issues: </div><div>1) >9k nitpicking - core reviewers have to put -1 if something is not covered by unit tests</div><div>2) core team scaling issues - core team members spend a lot of time just checking that whole code is covered by unit test and leaving messages like this should be covered by unit test </div><div>3) not friendly community - it's not nice to get on your code -1 from somebody that is asking just to write unit tests</div><div>4) coverage regressions - sometimes we accidentally accept patches that reduce coverage  </div><div><br></div><div>To resolve this issue I improved a bit coverage job in Rally project, and now it compares master vs master + patch coverage. If new coverage is less than master job is marked as -1. </div><div><br></div><div>Here is the patch for job enhancement: </div><div><a href="https://review.openstack.org/#/c/174645/" target="_blank">https://review.openstack.org/#/c/174645/</a></div><div><br></div><div>Here is coverage job in action:</div><div>patch <a href="https://review.openstack.org/#/c/174677/" target="_blank">https://review.openstack.org/#/c/174677/</a></div><div>job message <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></div><div><br></div><div><br></div><div>Best regards,</div><div>Boris Pavlovic </div></div>
</div></blockquote></div></div><blockquote type="cite"><div><span>__________________________________________________________________________</span><br><span>OpenStack Development Mailing List (not for usage questions)</span><br><span>Unsubscribe: <a href="mailto:OpenStack-dev-request@lists.openstack.org" target="_blank">OpenStack-dev-request@lists.openstack.org</a>?subject:unsubscribe</span><br><span><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></span><br></div></blockquote></div><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></blockquote></div><br></div>