Gordon, +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.
It is similar to say let's remove unit/functional/pep8/pylint/"any other testing" because it will lead to lazy reviews. Best regards, Boris Pavlovic On Mon, Apr 20, 2015 at 5:14 PM, gordon chung <g...@live.ca> wrote: > ---------------------------------------- > > Date: Mon, 20 Apr 2015 07:13:31 -0400 > > From: s...@dague.net > > To: openstack-dev@lists.openstack.org > > Subject: Re: [openstack-dev] [all][code quality] Voting coverage job (-1 > if coverage get worse after patch) > > > > 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. > > > > 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. > > > > Honestly, coverage for projects seems to me to be more of an analog > > measure that would be good to graph over time and make sure it's not > > regression, but personally I think the brownian motion of individual > > patches seems like it's not a great idea to gate on every single patch. > > I personally would be -1 for adding this to projects that I have +2 on. > > > > -Sean > > > > -- > > Sean Dague > > http://dague.net > > +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. > > cheers, > gord > > > __________________________________________________________________________ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >
__________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev