Before we go changing process, is this really a systemic issue? Bugs happen, and people sometimes land code, I'de rather be nimble in fixing those issues than rigid in preventing them. Personal opinion.
-Michal On Wed, Jun 12, 2013 at 9:54 AM, Ian Clelland <iclell...@chromium.org>wrote: > That's not an issue of "we need better contributions from new people", > that's "We, the committers, need to be more diligent in reviewing and > testing before committing". > > We shouldn't be discouraging contributions from anyone, but we don't have > to accept every pull request as-is, either. We can push back on the patch > if it isn't clean enough, or if it isn't clear that it's the right > solution, or even if it doesn't come with tests / docs. > > I'd have no problem with a lot more pull requests, if there was healthy > discussion on JIRA for each one, including the core committers and the > original reporter, before accepting (or rejecting) them. > > Regarding this particular commit, the original patch is ugly, and probably > shouldn't have been accepted without some cleanup, review, and attached > tests. The additional issue that it caused, though, is obviously something > that wasn't covered by our existing tests. It was only noticed a couple of > months later, in a post on StackOverflow, and as far as I can tell, it was > only mentioned in Github 18 days ago, and Andrew was right on it. (He's not > working on it currently, for other reasons) > > That regression doesn't seem to me to be a failure in process, so much as a > gap in our test framework (which we can rectify easily enough) and maybe an > issue of not-paying-enough-attention-to-stackoverflow, or > not-having-a-clear-channel-to-report-bugs. Maybe if it were easier for > people to tell us when something broke and we didn't catch it, we might > have fixed this a month sooner. > > > > On Wed, Jun 12, 2013 at 2:09 AM, Joe Bowser <bows...@gmail.com> wrote: > > > Hey > > > > I decided to check my e-mail and respond to an issue. It turns out > > that we're not properly testing or even checking the pull requests > > that we're getting into the project. The bug in question is CB-3766, > > which was caused by a patch accepted to fix CB-2458. The error isn't > > totally obvious, and doesn't get easily picked up on unit test (our > > CordovaWebView test mostly works, but fires a TIMING ERROR), but if > > you use an emulator, due to the emulator's crappiness, it apparently > > creates an ugly stack trace. > > > > Honestly, there's no way that this should have made it in. I > > personally think that I'm going to have to rename loadUrlIntoView to > > initAndLoadUrl or something more obvious, since it's too similar to > > loadUrl. That being said, we need to be more strict about quality > > while at the same time encouraging people to contribute. I'd rather > > deal with hundreds of pull requests than hundreds of issues where > > people know the answer but don't tell you. > > > > Any ideas how we can accomplish this? Has anyone else seen pull > > requests get in that shouldn't have? > > > > Joe > > >
