www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Proposal for pull request review process

reply Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
Hi all,

I originally posted this idea in the course of a thread in D.announce but
thought I'd put it here so that more people can pitch in.

It's generally recognized that the reviewing and merging pull requests is one of
the major bottlenecks in D's development process.  My own experience submitting
patches to Phobos (I can't speak for DMD or druntime) has been that often the
actual _review_ part is quite quick, but that the delay is in going from a patch
being approved to being merged.

Confusion can be added because reviewers don't always indicate explicit approval
of the code, so the submitter can be sitting in limbo not knowing whether the
problem is that their code is inadequate or the reviewer just not getting round
to merging it yet.


From the reviewer's point of view, on the other hand, the problem is that they


can't merge code until it's passed the test suite, and as there are no alerts
for test-suite passes, it's easy to miss an opportunity to merge and instead
have to sit through another round of testing that happens due to another patch
being merged first.  This only needs to happen a few times before the code under
review gets shunted to the bottom of the priority queue for testing, so
increasing the wait between opportunities to merge (and, without alerts, the
likelihood to miss them).

So, I'd propose that the review process include a way for the reviewers to
indicate that a pull request is approved subject to passing the test suite, and
that it be obligatory for reviewers to provide such an indicator.

Then, approved pull requests would go on a separate priority test queue with
"first in, first out" priority.  If the test suite passes, the pull request is
auto-merged, if it fails then it is removed from the queue and must be
re-approved.

Ideally it should be possible to distinguish actual test failures from something
going wrong with the test procedure itself (e.g. a test process not spawning
correctly), and in the latter case keeping the pull request in the approved
queue.

The result should be that reviewers can focus on _reviewing_, and don't need to
worry about waiting around for the test suite -- they can just approve in the
safe knowledge that the merge will happen (or not) depending on the test
results.

Does this sound like something good to have, that would be feasible to
implement?

Thanks & best wishes,

     -- Joe
Jul 28 2013
parent reply "Daniel Murphy" <yebblies nospamgmail.com> writes:
"Joseph Rushton Wakeling" <joseph.wakeling webdrake.net> wrote in message 
news:mailman.289.1375044093.22075.digitalmars-d puremagic.com...


 So, I'd propose that the review process include a way for the reviewers to
 indicate that a pull request is approved subject to passing the test 
 suite, and
 that it be obligatory for reviewers to provide such an indicator.


We already do this for dmd by commenting with "LGTM, waiting for green". 
Then if it goes green and the reviewer doesn't notice, the author can post 
something like "It's green now!" on the pull.  This works quite well, as 
another reviewer can pull without re-review if they notice first.


 Then, approved pull requests would go on a separate priority test queue 
 with
 "first in, first out" priority.  If the test suite passes, the pull 
 request is
 auto-merged, if it fails then it is removed from the queue and must be 
 re-approved.


The priority is based on how recently the pull request was modified.  If you 
find your pull request is somewhere at the bottom (or just not close enough 
to the top) you can simply rebase it on the latest master and re-push it. 
If you are not the author, a comment should be enough to ask the author to 
do it.


 Ideally it should be possible to distinguish actual test failures from 
 something
 going wrong with the test procedure itself (e.g. a test process not 
 spawning
 correctly), and in the latter case keeping the pull request in the 
 approved queue.


This would be nice, and should be fairly straighforward to implement if 
anybody feels like it.


 The result should be that reviewers can focus on _reviewing_, and don't 
 need to
 worry about waiting around for the test suite -- they can just approve in 
 the
 safe knowledge that the merge will happen (or not) depending on the test 
 results.


I haven't found this to be a problem with dmd.  Waiting for the test suite 
only seems to be a problem with really old pull requests, and they can be 
bumped easily with a rebase/push.


 Does this sound like something good to have, that would be feasible to 
 implement?


I'm not saying these things wouldn't be good to have, but I suspect the 
bottleneck is not enough people reviewing.
Jul 30 2013
parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
On 07/30/2013 02:27 PM, Daniel Murphy wrote:

 We already do this for dmd by commenting with "LGTM, waiting for green". 
 Then if it goes green and the reviewer doesn't notice, the author can post 
 something like "It's green now!" on the pull.  This works quite well, as 
 another reviewer can pull without re-review if they notice first.


Well, my point is that once "LGTM" is given, the actual merge can be made
automatic conditional on the test suite passing.  It's an unnecessary bottleneck
to rely on reviewers (or code authors) taking time to check that an approved
pull request has gone green, and takes their time from other more productive
activities.

(In my experience "LGTM" doesn't always arrive, probably because the reviewer is
thinking, "I'll just merge it when it goes green", and then when the window of
opportunity is missed, it takes ages for green to come around again...:-)


 The priority is based on how recently the pull request was modified.  If you 
 find your pull request is somewhere at the bottom (or just not close enough 
 to the top) you can simply rebase it on the latest master and re-push it. 
 If you are not the author, a comment should be enough to ask the author to 
 do it.


Yes, I understand this, though personally I've always felt it rather rude to
push a rebased version like this just to speed up testing -- if lots of people
start doing this, the whole test priority queue will become a messy
free-for-all.

That's why I propose a separate priority test queue for approved and
not-yet-approved pull requests -- get the approved stuff dealt with quickly and
out of the test system entirely.

But even without a separate queue, auto-merge-on-test-pass should help to avoid
the problem of pull requests getting tested over and over and over again simply
because of a lack of merge.


 This would be nice, and should be fairly straighforward to implement if 
 anybody feels like it.


Hint understood. :-)  I won't promise to follow up, as my time is constrained,
but I'm sure it would be interesting to look into the test suite and see if a
contribution is possible.


 I haven't found this to be a problem with dmd.  Waiting for the test suite 
 only seems to be a problem with really old pull requests, and they can be 
 bumped easily with a rebase/push.


I suspect there may be a difference between dmd, druntime and Phobos in how the
review process goes.  Apart from anything else the number of people who are
confident/expert enough to review compiler contributions is probably narrower
than the range of people who can work on the standard library.

But even with the number of reviewers being the bottleneck, automated merge
procedures should help those there are maximize their "useful" time, as well as
making testing more efficient by eliminating avoidable re-tests.


 I'm not saying these things wouldn't be good to have, but I suspect the 
 bottleneck is not enough people reviewing. 


More reviewers are always great and certainly necessary, but I can see there
being other scaling issues arising if we don't take advantage of automated
procedures to minimize the amount of time approved pull requests spend in the
test queue.

I'd see this as planning for the volume of pull requests _we'd like to have_ :-)
Jul 30 2013