We could even take it further and require a very high coverage for changes to go without reviews. This would put the ball in the change owner's court. Getting coverage to a very high level is good but requires a lot of work since one needs to cover failure scenarios in their tests which I would argue is more beneficial than a lengthy code review cycle.
Clearly, my vote is [+1] for trying something new I should stop now :) ~Abdullah. > On Dec 6, 2016, at 11:11 AM, abdullah alamoudi <[email protected]> wrote: > > If I remember correctly, the tool is JaCoCo. I believe we have a server > running somewhere which has the coverage information for each build. I could > be mistaken though. > > Cheers, > Abdullah. > >> On Dec 6, 2016, at 10:58 AM, abdullah alamoudi <[email protected]> wrote: >> >> Steven, >> There is already a tool that runs code coverage with each build. (forgot >> what it is called) but I am sure we can automate checking that as part of >> the voting process on changes. >> >> Cheers, >> Abdullah. >> >>> On Dec 6, 2016, at 10:55 AM, Steven Jacobs <[email protected]> wrote: >>> >>> @Abdullah: >>> "we look at the code coverage and how much it improved and based on that, >>> either allow the change in or deny it." >>> >>> What would this look like in practice, i.e. who is the "we" here? >>> >>> Steven >>> >>> On Tue, Dec 6, 2016 at 10:42 AM, abdullah alamoudi <[email protected]> >>> wrote: >>> >>>> I would like to stress one point though: Having no comments after a >>>> timeout (72hr) means that the reviewer couldn't find time to do the review. >>>> In which case, the change owner needs to be extra careful as the whole >>>> blame will be on them if their change breaks something. If you're not >>>> totally sure about your change and are not testing every little possible >>>> case, you can still insist on a review. >>>> >>>> One issue that comes to mind is: what if someone's changes continuously >>>> break things? >>>> Of course we can't revoke commit privileges (Can we?) but what we can do >>>> is: >>>> 1. pay more attention to changes submitted by people who's changes break >>>> more often. >>>> 2. increase the timeout period for them "temporarily" 72->96->120..... If >>>> you don't like this, don't make bad changes. >>>> 3. MOST IMPORTANTLY, all of us should strive to have better test coverage >>>> to automatically detect breaks caused by wild changes. >>>> >>>> Cheers, >>>> ~Abdullah. >>>> >>>>> On Dec 6, 2016, at 10:33 AM, abdullah alamoudi <[email protected]> >>>> wrote: >>>>> >>>>> Ceej, >>>>> You spoke my mind and I agree with every word. I believe the way to go >>>> is smoother code review process (maybe through time limit) and more focus >>>> on automated testing. One thing we could do is: >>>>> If the 72 hours period pass with no comments, we look at the code >>>> coverage and how much it improved and based on that, either allow the >>>> change in or deny it. This will push change submitters to add tests to >>>> increase coverage even before the 72h limit passes. This doesn't remove the >>>> responsibility of doing the review. The review is still to be done. However >>>> as Ceej said: One of the goals of doing the reviews is to catch large scale >>>> design errors. Those I think still need humans to look at but they can be >>>> caught fairly quickly with minimal effort. >>>>> >>>>> As for spreading the knowledge, will leave that to a different >>>> discussion. I will end this with some tweets about code simplicity and >>>> changes taken from Max Kanat-Alexander, author of Code Simplicity: >>>>> >>>>> 1. You don't have to be perfect. If you make a bad change, just fix it. >>>> (mistakes will happen with or without reviews) >>>>> 2. If somebody is improving code quality, don't shoot them down because >>>> their improvement isn't perfect. (to reviewers) >>>>> 3. The point is to have a maintainable system, not to show how clever >>>> you are. (to submitters) >>>>> 4. Code quality isn't something you fix once and it stays good forever. >>>> It's something you learn to do and continue doing. >>>>> 5. Engineers don't beg, "Please let me build a bridge that will stay >>>> up." You shouldn't need permission to write good software. >>>>> 6. Anybody who tells you that you can fix years of bad code in months or >>>> days is a liar. >>>>> 7. Even huge codebases can be refactored incrementally. >>>>> 8. Sometimes a refactoring will break something. Often, this proves that >>>> the code was too fragile and so needed the refactoring! >>>>> 9. If your code "works," but it's an unstable pile of complexity, do you >>>> feel good about it? >>>>> 10. Refactoring is often easier and more rewarding than you expect. >>>>> 11. Don't try to write "perfect" code, just write *better* code until >>>> you have *good* code. >>>>> 12. Don't worry about how to do the perfect refactoring. Just keep >>>> improving the code in small steps. >>>>> >>>>> I am glad we're talking about this. Cheers, >>>>> ~Abdullah. >>>>> >>>>>> On Dec 5, 2016, at 11:13 PM, Chris Hillery <[email protected]> >>>> wrote: >>>>>> >>>>>> It's always been my opinion that code reviews are a very nice-to-have, >>>> but >>>>>> not more than that. The real value in proposing changes for review comes >>>>>> from the automated testing that can be performed at that stage. I think >>>>>> we'd be better served overall by shoring up and expanding our automated >>>>>> testing rather than spending time discussing and implementing >>>> non-technical >>>>>> process. >>>>>> >>>>>> The main benefits of code reviews are catching large-scale design errors >>>>>> and spreading code knowledge. You can't really have the former until you >>>>>> already have the latter - if only one person really understands an area, >>>>>> nobody else will be able to catch design errors in that area. That's >>>>>> clearly a risky place to be, but IMHO at least it's not a problem that >>>> can >>>>>> be solved through rules. It requires a cultural shift from the team to >>>> make >>>>>> spreading code knowledge an actual priority, rather than someone >>>> everyone >>>>>> wants but nobody has time or interest to achieve. >>>>>> >>>>>> If we as a team don't have the drive to do that, then we should accept >>>> that >>>>>> about ourselves and move on. You'll always do best spending time on >>>>>> enhancing the strengths of a team, not fighting against the things they >>>>>> don't excel at. I'm also not trying to make any kind of value judgment >>>> here >>>>>> - software development is always about tradeoffs and compromise, risk >>>>>> versus goals. Any time taken to shift focus towards spreading code >>>>>> knowledge will by necessity pull from other parts of the development >>>>>> process, and the upshot may well not be an overall improvement in >>>>>> functionality or quality. >>>>>> >>>>>> Ceej >>>>>> aka Chris Hillery >>>>>> >>>>>> On Mon, Dec 5, 2016 at 10:49 PM, Till Westmann <[email protected]> >>>> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> today a few of us had a discussion about how we could make the >>>> reviewing >>>>>>> process moving along a little smoother. The goal is to increase the >>>>>>> likeliness >>>>>>> that the reviews and review comments get addressed reasonably quickly. >>>> To >>>>>>> do >>>>>>> that, the proposal is to >>>>>>> a) try to keep ourselves to some time limit up to which a reviewer or >>>>>>> author >>>>>>> responds to a review or a comment and to >>>>>>> a) regularly report via e-mail about open reviews and how long they >>>> have >>>>>>> been >>>>>>> open (Ian already has filed an issue to automate this [1]). >>>>>>> Of course one is not always able to spend the time to do a thorough >>>> review >>>>>>> [2] >>>>>>> / respond fully to comments, but in this case we should aim to let the >>>>>>> other >>>>>>> participants know within the time limit that the task is not feasible >>>> so >>>>>>> that >>>>>>> they adapt their plan accordingly. >>>>>>> The first proposal for the time limit would be 72h (which is taken >>>> from the >>>>>>> minimal time that a [VOTE] stays open to allow people in all different >>>>>>> locations and timezones to vote). >>>>>>> Another goal would be to abandon reviews, if nobody seems to be >>>> working on >>>>>>> them >>>>>>> for a while (and we’d need to find out what "a while" could be). >>>>>>> >>>>>>> Thoughts on this? >>>>>>> A good idea or too much process? >>>>>>> Is the time limit reasonable? >>>>>>> Please let us know what you think (ideally more than a +1 or a -1 ...) >>>>>>> >>>>>>> Cheers, >>>>>>> Till >>>>>>> >>>>>>> [1] https://issues.apache.org/jira/browse/ASTERIXDB-1745 >>>>>>> [2] https://cwiki.apache.org/confluence/display/ASTERIXDB/Code+Reviews >>>>>>> >>>>> >>>> >>>> >> >
