Open Stack

On 09:30 Apr 20, Jay Pipes wrote:
> On 04/20/2015 07:13 AM, Sean Dague wrote:
> >On 04/18/2015 09:30 PM, Boris Pavlovic wrote:
> >>Hi stackers,
> >>
> >>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.
> >>
> >>One of the nice thing is coverage job.
> >>
> >>In Rally we are running it against every check which allows us to get
> >>detailed information about
> >>coverage before merging patch:
> >>http://logs.openstack.org/84/175084/1/check/rally-coverage/c61d5e1/cover/
> >>
> >>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%.
> >>
> >>But it produces few issues:
> >>1) >9k nitpicking - core reviewers have to put -1 if something is not
> >>covered by unit tests
> >>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
> >>3) not friendly community - it's not nice to get on your code -1 from
> >>somebody that is asking just to write unit tests
> >>4) coverage regressions - sometimes we accidentally accept patches that
> >>reduce coverage
> >>
> >>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.
> >>
> >>Here is the patch for job enhancement:
> >>https://review.openstack.org/#/c/174645/
> >>
> >>Here is coverage job in action:
> >>patch https://review.openstack.org/#/c/174677/
> >>job message
> >>http://logs.openstack.org/77/174677/4/check/rally-coverage/ba49c90/console.html#_2015-04-17_15_57_17_695
> >
> >Honestly, I'm suspicious of approaches like this. While 90% coverage is
> >clearly better than 0% coverage, it's not clear that 91% coverage is
> >always better than 90% coverage. Especially when you talk about larger
> >and more complex code bases.
> 
> Well, I think there are very few cases where *less* coverage is better.
> 
> >I actually firmly feel that #3 is wrong. I think it's a lot better to
> >have an early conversation with people about unit tests that need to be
> >written when they don't submit any. Because I think it's a lot less
> >friendly to have someone iterate 10 patches to figure out how to pass a
> >bot's idea of good tests, and then have a reviewer say it's wrong.
> 
> This I completely agree with. Asking for unit tests is a common
> thing, and if done early in the review process, is not a
> "non-friendly" thing. It's just matter of fact. And if the comment is
> given with some example unit test code -- or a pointer to a unit test
> that could be used as an example -- even better. It grows the
> submitter.

I also agree talking with people early about this is fine. I deal with new
reviewers often, and it has never been a negative experience. Just don't be
a jerk when asking, and follow up soon.

-- 
Mike Perez