Hey guys,
In the interest of streamlining the patch-submission process, we've been thinking about writing down the informal vetting process we've been using for patches. The first cut of this is written down in the wiki:
http://wiki.rubyonrails.org/rails/pages/PatchRequirements
Any comments or questions?
Sounds cool.
You can probably mention #rails-contrib as well for finding potential reviewer.
And how about having a rule to close the ticket as invalid/wontfix if it's been there for over 2-3 months without any updates ? And may be get a core member to reopen it if one feels the patch has a chance of making it to core ever. I was just going through some of the old tickets, and I feel that very small % of them will ever make it to core. Probably closing the remaining tickets would be good to keep things clean on trac as some of them are even 2 years old.
One more thing I always wonder about is if I should assign the ticket to a core member or just leave it with default assignee "core". You think it might be a good idea to assign ticket to the particular core member who has been the most active in the area that patch affects ? If yes, may be you can put the mapping at that wiki page ? Just a thought.
Thanks, Pratik
Requiring peer review is great. Also, multiple peer review can help
mitigate the tyranny of the zealous 
I don't personally have time to make core patches any more but would
be happy to help the community in this way by moderating. Great ideas!
Looks about right.
Maybe expand on the warning bitsweat added to the trac home page.
"Expect your ticket to be closed with an untested, undocumented, or incomplete resolution if it's missing tests, documentation, or implementation. Don't panic; the ticket hasn't been killed! These resolutions are the pathway to commit. Update your patch and reopen the ticket."
This seems heavy-handed for patches which fix defects. I mean, if a defect report didn't have a patch at all, it'd stay open, but if it's a *better* report (ie closer to being fixed, just not quite all the way there) then it gets closed? That seems... counterproductive. (Disclaimer: I'm not a neutral party on this topic -- I've had a couple of bugs with patches treated this way recently; it *really* made me less interested in contributing future patches). I understand why patches get triaged, and there is the possibility of bad patches hanging around forever, but surely the policy of expiring old bug reports could also apply to half-baked patches, rather than instant closure?
Also, I presume that this PatchProcess page will be rolled into or otherwise linked from http://dev.rubyonrails.org once it's considered acceptable?
- Matt
I understand why patches get triaged, and there is the possibility of bad patches hanging around forever, but surely the policy of expiring old bug reports could also apply to half-baked patches, rather than instant closure?
It's not just the possibility of patches sitting around, we had hundreds of patches just sitting around and the submitters commented that the lack of feedback was more frustrating than having a patch 'rejected'.
What was it about the closure that made it irksome? Tickets get reopened all the time, closure isn't a death sentence. But it's more honest than just having an atrophied patch sitting in the queue.
Also, I presume that this PatchProcess page will be rolled into or otherwise linked from http://dev.rubyonrails.org once it's considered acceptable?
Yep.
So, if I’m the 3rd reviewer on someone’s patch and I leave a thumbs-up comment, it’s free for me to add the “verified” keyword?
@Josh Peek: +1 for the “don’t panic” notice. First-time contributors are easily discouraged otherwise.
Really? Cuz I had patches that got ignored, so I stopped contributing. Having them rejected with a reason would have helped.
That said, I like the new wiki page. Simple and to the point. The patch process becoming more transparent is a real win, thanks guys.
So, if I'm the 3rd reviewer on someone's patch and I leave a thumbs-up comment, it's free for me to add the "verified" keyword?
That's the intention yeah.
@Josh Peek: +1 for the "don't panic" notice. First-time contributors are easily discouraged otherwise.
It's probably worthwhile coming up with some 'canned' text to paste in when taking certain actions. I'd guess it's much more reassuring if you can see a path to getting your patch applied.
Really? Cuz I had patches that got ignored, so I stopped contributing. Having them rejected with a reason would have helped.
That's the conclusion we've come to. Better to reject something quickly, than let it atrophy forever for fear of offending someone.
That said, I like the new wiki page. Simple and to the point. The patch process becoming more transparent is a real win, thanks guys.
So far this is just a plan, hopefully it'll help out.
So far this is just a plan, hopefully it'll help out.
To clarify, we're doing this. Time will tell if it improves the
turnaround time on patches.
> I understand why patches get triaged, and > there is the possibility of bad patches hanging around forever, but surely > the policy of expiring old bug reports could also apply to half-baked > patches, rather than instant closure?
It's not just the possibility of patches sitting around, we had hundreds of patches just sitting around and the submitters commented that the lack of feedback was more frustrating than having a patch 'rejected'.
Yes, lack of feedback sucks, but there are forms of feedback other than "closed defect: incomplete".
What was it about the closure that made it irksome? Tickets get reopened all the time, closure isn't a death sentence.
'Closed' is one of the fundamental states of a ticket, and it means something fairly definite to me -- something along the lines of "absent some exceptional condition, this is done with".
The thing that really got to me was that, if I had just reported the bug and left off the patch, it would have stayed open for someone else to deal with. However, having taken the extra time to hunt down a solution and post the patch (however incomplete it may have been), the ticket was closed and, absent further interaction from me, it was as though the defect was never reported. If the patch is dodgy, then reject the *patch*, not the entire defect report. I very nearly opened a completely separate bug report without a patch just to see what would happen.
What would happen if a bug was reported by one person but an incomplete patch contributed by someone else? Would the report get closed (which to the initial reporter suggests the issue is solved) because the patch is dodgy?
- Matt
I have nothing against feedback. Feedback is good. The only way you're going to get better patches over time is if you communicate how the patches you're getting are insufficient. My *only* gripe is with the manner in which the communication is being performed. I have a particular definition of what 'closed' means to me, and it's not "OK, time to write a test case".
- Matt
Does the fact that it's being said that a closure isn't a death sentence, just a matter of feedback change your opinion of 'closed'? Do the 'untested', 'undocumented' resolutions hurt less?
I agree there, because nothing gets peoples attention like closing
their ticket 
Usually patches with good units tests that prove a bug are fixed rather quickly. The problem is we have tons of unverified (and bogus) defect reports with no easy way to prove them. No one has the time to personally debug someone elses problems.
We should post a note to try the Mailing List or IRC channel first if you are unsure about your patch.
> I have a particular > definition of what 'closed' means to me, and it's not "OK, time to write a > test case". > > - Matt
Does the fact that it's being said that a closure isn't a death sentence, just a matter of feedback change your opinion of 'closed'?
No, because it's an emotional response, not a purely intellectual one.
Do the 'untested', 'undocumented' resolutions hurt less?
They're not "resolutions", they're states. You set them as flags or something. The very meaning of word "resolution" implies some sort of finality, but a defect report that isn't fixed isn't resolved.
- Matt
> What would happen if a bug was reported by one person but an incomplete > patch contributed by someone else? Would the report get closed (which to > the initial reporter suggests the issue is solved) because the patch is > dodgy?
Usually patches with good units tests that prove a bug are fixed rather quickly.
(Good thing you put 'usually' in there, because I've got a patch in the system that fixes a bug in the *test suite* that's been hanging for over a week now)
Is it the policy of the Rails project that defect reports without a verifiable test case are invalid and will be closed?
If a project was going to adopt that policy, something like Rails would be the most likely to be able to pull it off, because every user should be a developer. Having spent a few hours a day the last few weeks helping on the #rubyonrails IRC channel, though, I'm not sure it's a practical proposition -- most people don't "get" testing, and plenty of Rails' users aren't sufficiently good programmers to be able to write a solid test case for something as complex as Rails.
The problem is we have tons of unverified (and bogus) defect reports with no easy way to prove them.
So mark them "more info required" with a note that they need to have more info attached or they'll be closed in N days/weeks/months.
No one has the time to personally debug someone elses problems.
<advocate type="devil"> That cuts both ways, though -- I, as a developer, don't have time to debug a problem in Rails. That problem isn't mine -- I didn't write the buggy code that's causing the hassle. It's *your* fault, dammit. </advocate>
In reality, I know all about the OSS project philosophy, I'm a great believer in it myself. If you want support, take out a support contract, and all that.
Going back to my original point, though, about actively *closing* tickets that document *real* bugs that are trivially verifiable[1] simply because the patch isn't sufficient is only reasonable in one circumstance -- if defect reports without test cases are summarily closed. I haven't seen that policy documented anywhere (and I would imagine something that it would want to be in big letters on the front page), and in fact the exact opposite is stated on the dev front page -- "Tickets are fine". I'm just reporting an irritating inconsistency in the handling of tickets, biased against well-meaning but insufficiently clued contributors (which are the sort of people I'd be wanting to encourage, not discourage, if it were my project).
- Matt
[1] one of the bugs I found I didn't even have to run the code to find it -- a desk check was enough to make it perfectly obvious.
Is it the policy of the Rails project that defect reports without a verifiable test case are invalid and will be closed?
I guess that'll be a nice policy to have considering number of open tickets trac has. Well, that seems to be an undocumented policy, a good thing anyways.
I'm not sure it's a practical proposition -- most people don't "get" testing, and plenty of Rails' users aren't sufficiently good programmers to be able to write a solid test case for something as complex as Rails.
Let them use core mailing list then. Which is a perfect medium.
So mark them "more info required" with a note that they need to have more info attached or they'll be closed in N days/weeks/months.
That'd be same as closing as "incomplete".
<advocate type="devil"> That cuts both ways, though -- I, as a developer, don't have time to debug a problem in Rails. That problem isn't mine -- I didn't write the buggy code that's causing the hassle. It's *your* fault, dammit. </advocate>
Oh well. That wasn't really needed. May be you can elaborate on *your* part.
Going back to my original point, though, about actively *closing* tickets that document *real* bugs that are trivially verifiable[1] simply because the patch isn't sufficient is only reasonable in one circumstance -- if defect reports without test cases are summarily closed. I haven't seen that policy documented anywhere (and I would imagine something that it would want to be in big letters on the front page), and in fact the exact opposite is stated on the dev front page -- "Tickets are fine". I'm just reporting an irritating inconsistency in the handling of tickets, biased against well-meaning but insufficiently clued contributors (which are the sort of people I'd be wanting to encourage, not discourage, if it were my project).
Perfect in theory. But practically speaking, if there is a problem that affects majority of people, it won't go unnoticed. And otherwise, people can always use this very mailing list. The reason why I'm in favor of actively closing tickets without patches/verifying tests is the current state of trac. 100s of open tickets. And a tiny % of them are valid bugs. And because of that, many of the proper patches go unnoticed for long time. They just get "lost".
(Good thing you put 'usually' in there, because I've got a patch in the system that fixes a bug in the *test suite* that's been hanging for over a week now)
It's not on the verified patches list, so it's unlikely to get much attention.
http://dev.rubyonrails.org/report/12
Is it the policy of the Rails project that defect reports without a verifiable test case are invalid and will be closed?
No, defect reports without test cases aren't supposed to be automatically closed. A great way for people to help is to find defect reports, then attach failing test cases, or at the very least confirm that they experience the same behaviour.
However I believe you're overstating the impact of closing a defect. There are two possibilities when someone reports a bug:
a) The bug was invalid, and caused either by the user's system, the alignment of jupiter's moons or some other freak occurrence b) The bug is valid, someone else will be able to verify it
Prematurely closing defects has no adverse impact on the first case. In the second case, the submitter can simply reopen them. Now even if you assume that closing the ticket has so completely enraged the submitter that they've gone off to write web frameworks in ocaml, we're still ok. Someone else will hit the same bug, and will either reopen the original report (unlikely because trac's search is shit) or submit a new one.
The new ticket can then be fixed, or we can repeat this process again.
The reality is that there are bugs in every system, and if there's a defect which is so rare that only one person has ever hit it, then I'm not going to lose any sleep over continuing to ship that bug.
> The problem is we have tons of unverified (and bogus) > defect reports with no easy way to prove them.
So mark them "more info required" with a note that they need to have more info attached or they'll be closed in N days/weeks/months.
That's exactly the same as closing them, as they're unlikely to continue to receive any attention. I think lying to our submitters by leaving an issue 'open' when no one intends to even look at it is worse than closing it prematurely.
<advocate type="devil"> That cuts both ways, though -- I, as a developer, don't have time to debug a problem in Rails. That problem isn't mine -- I didn't write the buggy code that's causing the hassle. It's *your* fault, dammit. </advocate>
In reality, I know all about the OSS project philosophy, I'm a great believer in it myself. If you want support, take out a support contract, and all that.
That's a complete mischaracterisation of the situation we're in, I'm more than willing to help people fix the problems they have. I spend a good portion of my time doing just that.
We have too many reports, so many that it's impossible to make a dent in them. Reporters hear nothing back about their bug report, get disenchanted and leave. The patch queue fills up to the point we don't know where to start. If we get better turnaround time on patches and encourage contributors, and in the process prematurely close a few edge-case defects. So be it.
Going back to my original point, though, about actively *closing* tickets that document *real* bugs that are trivially verifiable[1] simply because the patch isn't sufficient is only reasonable in one circumstance -- if defect reports without test cases are summarily closed. I haven't seen that policy documented anywhere (and I would imagine something that it would want to be in big letters on the front page), and in fact the exact opposite is stated on the dev front page -- "Tickets are fine". I'm just reporting an irritating inconsistency in the handling of tickets, biased against well-meaning but insufficiently clued contributors (which are the sort of people I'd be wanting to encourage, not discourage, if it were my project).
Indeed, while reports without failing test cases aren't as likely to get attention, they're still valuable information, and could provide indications of where to start contributing for people who are keen to help out. Please don't close them unless you've tried to reproduce the bug, and are unable to do so.
However I don't buy the sky is falling hypothesis of closing a ticket prematurely, just reopen it, explain it's still a bug. If the disagreement continues, start a thread here.