cassandra-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeremiah D Jordan <jeremiah.jor...@gmail.com>
Subject Re: Low hanging fruit crew
Date Wed, 19 Oct 2016 15:55:02 GMT
Unless the reviewer reviews the for content, then you don’t know if they do or not.

-Jeremiah

> On Oct 19, 2016, at 10:52 AM, Jonathan Haddad <jon@jonhaddad.com> wrote:
> 
> Shouldn't the tests test the code for correctness?
> 
> On Wed, Oct 19, 2016 at 8:34 AM Jonathan Ellis <jbellis@gmail.com> wrote:
> 
>> On Wed, Oct 19, 2016 at 8:27 AM, Benjamin Lerer <
>> benjamin.lerer@datastax.com
>>> wrote:
>> 
>>> Having the test passing does not mean that a patch is fine. Which is why
>> we
>>> have a review check list.
>>> I never put a patch available without having the tests passing but most
>> of
>>> my patches never pass on the first try. We always make mistakes no matter
>>> how hard we try.
>>> The reviewer job is to catch those mistakes by looking at the patch from
>>> another angle. Of course, sometime, both of them fail.
>>> 
>> 
>> Agreed.  Review should not just be a "tests pass, +1" rubber stamp, but
>> actually checking the code for correctness.  The former is just process;
>> the latter actually catches problems that the tests would not.  (And this
>> is true even if the tests are much much better than ours.)
>> 
>> --
>> Jonathan Ellis
>> co-founder, http://www.datastax.com
>> @spyced
>> 


Mime
View raw message