asterixdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From abdullah alamoudi <bamou...@gmail.com>
Subject Re: [DISCUSS] Improving reviews
Date Tue, 06 Dec 2016 19:14:43 GMT
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 <bamousaa@gmail.com> 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 <bamousaa@gmail.com> 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 <sjaco002@ucr.edu> 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 <bamousaa@gmail.com>
>>> 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 <bamousaa@gmail.com>
>>>> 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 <chillery@hillery.land>
>>>> 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 <tillw@apache.org>
>>>> 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
>>>>>>> 
>>>>> 
>>>> 
>>>> 
>> 
> 


Mime
View raw message