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
