[development] What about reviewing patches?

[Senpai]

On Aug 14, 2008, at 10:29 AM, Angela Byron wrote:

> Let's have a community outcry about the RTBC queue once the  
> situation is reversed, and we have 325 patches waiting for core  
> committers' blessing, and only 40 patches that need community  
> review. Until then, we have work to do.


@Angie:
Hear hear! And while I'm at it, let me just say that a patch review  
doesn't take 20 minutes. Unless the patch only has < 5 lines of  
changed code in it, it takes an hour~ to review it. This is how long  
it takes me to read the entire issue queue to figure out which version  
of the patch is the community approved one, identify the goals and  
desires of the patch and weed out the scope creep, download the patch,  
reset my testing sandbox from the last time I ran one of these,  
install the core or contrib that I'm about to test against, go forth  
and familiarize myself with the existing functionality that's about to  
be changed or altered by this patch, apply the patch against its  
intended destination, re-familiarize myself with the intended set of  
changes, actually test those changes to see if they work, methodically  
approach the targeted subset of functional code to see if I can think  
of a way to break it, break it, observe the results of that breakage,  
and then reliably report the results back to the issue queue n a way  
that can be understood by all parties involved. And that's not even  
including the Simpletests that pass/fail, of the lack of Simpletest(s)  
for certain areas of the code that should really be tested, and would  
therefore hold back an RTBC status from being bestowed upon the issue.

Just reading that last paragraph should make your head hurt. It did  
me, and I wrote it. The one thing that speeds up the testing process  
for me, and I mean greater than 50% is a clear set of testing  
instructions. If I can find an issue that has less than 12 replies,  
and an attached patch, I'll jump on it because it should be far easier  
to discern what the intent of the patch is and test accordingly.

On the other hand, once an issue reaches > 15 comments, or has more  
than two patches attached to it, I automatically assign it a "one  
precious hour of my personal time" sticker, and am far less motivated  
to tackle the thing. The only way that an issue can really force me to  
test it is if I see recent action on the part of the person who  
actually makes the commit, or a clearly defined two-sentence  
instructional on how to test this patch from the coder's mindset at  
the time they wrote it.

Which brings me to the end of this little diatribe, but because I  
strive to never point out a problem without also proffering a  
solution, I'd like to ask if we might be able to create a 5-line  
textarea for each issue in the queue that would store the Patch  
Testing Instructions. Old issues would not have any info in this  
field, but if a developer wished to receive some near-instant reviews  
from the community, they could write up a one-paragraph set of  
instructions on how to adequately test this patch so that it's  
committable.

We can place this field at the top of each issue as a collapsed div,  
and proffer an expand/collapse jQuerified link right below the
First unread comment
Most recent comment
Add new comment
[+]Patch review instructions

-- 
Senpai