Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag

[Dave Chinner]

On Mon, Jun 02, 2014 at 09:30:45PM -0400, Steven Rostedt wrote:
> On Tue, 3 Jun 2014 11:11:25 +1000
> Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> 
> 
> > You've ignored the (c).(2) "free of known issues" criteria there.
> > You cannot say a patch is free of issues if you haven't applied,
> > compiled and tested it.
> > 
> > > We should not, for instance, prevent someone from providing a
> > > Reviewed-by (as opposed to an Acked-by) for a driver whose hardware few
> > > people actually have.  There's significant value in code review even
> > > without the ability to test.
> > 
> > I don't disagree with you that there's value in code review, but
> > that's not the only part of what "reviewed-by" means.
> > 
> > You can test that the code is free of known issues without reviewing
> > it (i.e. tested-by). You can read the code and note that you can't
> > see any technical issues without testing it (Acked-by).
> 
> Unless you run every test imaginable on all existing hardware, you are
> not stating that it is free of known issues. I say your logic is flawed
> right there.

If you take it to an extremes. Think about what you can test in 15
minutes. Or for larger patchsets, how long it takes you to read the
patchset?

IMO, every reviewer has their own developement environment and they
should be at least testing that the change they are reviewing
doesn't cause problems in that environment, just like they do for
their own code before they post it for review.

Code being reviewed should pass the same testing bar that the
developer uses for code they write and send for review. A maintainer
quickly learns whose test environments are up to scratch or not. :/

> > But you can't say that is it both free of techical and known
> > issues without both reading the code and testing it (Reviewed-by).
> 
> I disagree. Testing only tests what you run. It's useless otherwise.
> Most code I review, and find bugs for in that review, will not be
> caught by tests unless you ran it on a 1024 CPU box for a week.
> 
> I value looking hard at code much more than booting it and running some
> insignificant micro test.

Running "insignficant micro tests" is exactly that - insignificant -
and it's not a robust code review if that is all that is done.
Runing *regression tests*, OTOH....

I know from experience that a "quick" 15 minute run on xfstests on a
ramdisk will catch 99% of typical problems a filesystem patch might
introduce. Code coverage reporting (done recently by RH QE
engineers) tells me that this covers between 50-70% of the
filesystem, VFS and MM subsystems (numbers vary with fs being
tested), and so that's a pretty good, fast smoke test that I can run
on any patch or patchset that is sent for review.

Any significant patch set requires longer to read and digest than a
full xfstests run across all my test machines (about 80 minutes for
a single mkfs/mount option configuration). So by the time I've
actually read the code and commented on it, it's been through a full
test cycle and it's pretty clear if there are problems or not..

And so when I say "reviewed-by" I'm pretty certain that there aren't
any known issues.  Sure, it's not going to catch the "ran it on a
1024 CPU box for a week" type of bug, but that's the repsonsibility
of the bug reporter to give you a "tested-by" for that specific
problem.

And really, that points out *exactly* the how "reviewed-by" is a far
more onerous than "tested-by". Tested-by only means the patch fixes
the *specific problem* that was reported.  Reviewed-by means that,
as far as the reviewer can determine, it doesn't cause regressions
or other problems. It may or may not imply "tested-by" depending on
the nature of the bug being fixed, but it certainly implies "no
obvious regressions".  They are two very different meanings, and
reviewed-by has a much, much wider scope than "tested-by".

> > And, yes, this is the definition we've been using for "reviewed-by"
> > for XFS code since, well, years before the "reviewed-by" tag even
> > existed...
> 
> Fine, just like all else. That is up to the maintainer to decide. You
> may require people to run and test it as their review, but I require
> that people understand the code I write and look for those flaws that
> 99% of tests wont catch.
> 
> I run lots of specific tests on the code I write, I don't expect those
> that review my code to do the same. In fact that's never what I even
> ask for when I ask someone to review my code. Note, I do ask for
> testers when I want people to test it, but those are not the same
> people that review my code.

Very few subsystems have dedicated testers and hence rely on the
test environments that the subsystem developers use every day to
test their own code. IOWs, in most cases "tester" and the "reviewer"
roles are performed by the same people.

> I find the reviewers of my code to be the worse testers. That's because
> those that I ask to review my code know what it's suppose to do, and
> those are the people that are not going to stumble over bugs. It's the
> people that have no idea how your code works that will trigger the most
> bugs in testing it. My best testers would be my worse reviewers.

"Reviewers are the worst testers".

Yet you accept the code they write as sufficiently well tested for
merging? :/

> What do you require as a test anyway? I could boot your patches, but
> since I don't have an XFS filesystem, I doubt that would be much use
> for you.

I don't want you to review XFS code - you have no domain specific
knowledge nor a test environment for XFS changes. IOWs, you meet
none of the requirements to give a "reviewed-by" for an XFS change,
and so to say you reviewed a change makes a mockery of the
expectations outlines in the SubmittingPatches guidelines.

Similarly, I would expect any other maintainer to give me the same
"go read the guidelines - you know better than that" line if I did
the same thing for a patch that I clearly don't have a clue about or
are able to test.

Reviewed-by only has value when it is backed by process and domain
specific knowledge. If the person giving that tag doesn't have
either of these, then it's worthless and they need to be taught what
the correct thing to do is. Most people (even projects) don't learn
proper software engineering processes until after they have been at
the pointy end of a pile of crap at least once. :/

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/