And just to be clear, I think everyone would welcome more testing for both regressions of new code correctness. I think everyone would appreciate the time savings around more automation. That should give more time for a thoughtful review - which is likely what new contributors really need to get familiar with different parts of the codebase. These LHF reviews won’t be like the code reviews of the vnode or the 8099 ticket obviously, so it won’t be a huge burden. But it has some very tangible benefits imo, as has been said.
> On Oct 19, 2016, at 11:08 AM, Jonathan Ellis <jbel...@gmail.com> wrote: > > I specifically used the phrase "problems that the test would not" to show I > am talking about more than mechanical correctness. Even if the tests are > perfect (and as Jeremiah points out, how will you know that without reading > the code?), you can still pass tests with bad code. And is expecting > perfect tests really realistic for multithreaded code? > > But besides correctness, reviews should deal with > > 1. Efficiency. Is there a quadratic loop that will blow up when the number > of nodes in a cluster gets large? Is there a linear amount of memory used > that will cause problems when a partition gets large? These are not > theoretical problems. > > 2. Maintainability. Is this the simplest way to accomplish your goals? Is > there a method in SSTableReader that would make your life easier if you > refactored it a bit instead of reinventing it? Does this reduce technical > debt or add to it? > > 3. "Bus factor." If only the author understands the code and all anyone > else knows is that tests pass, the project will quickly be in bad shape. > Review should ensure that at least one other person understand the code, > what it does, and why, at a level that s/he could fix bugs later in it if > necessary. > > On Wed, Oct 19, 2016 at 10:52 AM, Jonathan Haddad <j...@jonhaddad.com> wrote: > >> Shouldn't the tests test the code for correctness? >> >> On Wed, Oct 19, 2016 at 8:34 AM Jonathan Ellis <jbel...@gmail.com> wrote: >> >>> On Wed, Oct 19, 2016 at 8:27 AM, Benjamin Lerer < >>> benjamin.le...@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 >>> >> > > > > -- > Jonathan Ellis > co-founder, http://www.datastax.com > @spyced
