On 25 May 2012, at 13:34, Sanne Grinovero wrote: > On 25 May 2012 13:16, Manik Surtani <ma...@jboss.org> wrote: >> >> On 25 May 2012, at 12:47, Sanne Grinovero wrote: >> >>> guys, please don't take me as the one who is again complaining about >>> failing tests; I'm having doubts about the development process and the >>> amount of time this is wasting on all of us. >>> >>> We're all humans and do mistakes, still it happens so extremely often >>> that this is getting systemic, and discipline could definitely be >>> improved: people regularly send pull requests with failing tests or >>> broken code, and very regularly this is just merged in master. >> >> Yes, this must not happen. I have been culprit of this a few times, but we >> really shouldn't do this. Galder integrating BuildHive into the process >> should help to some degree. >> >>> >>> I did it myself a couple of days ago: didn't notice a failure, all >>> looked good, sent a pull, it was merged with no complaints. Three days >>> later, I resume my work and am appalled to see that it was broken. Now >>> fixing it, but I'll have to send another pull and wait for it - which >>> feels very pointless, as I'm pretty sure nobody is checking anyway. >>> >>> It looks like as the pull request procedure is having this effect: >>> >>> # patch writer is not as carefull as he used to be: "someone else >>> will check if it's fine or not. I have no time to run the tests >>> again..". >> >> Yes, the patch author should still be just as vigilant. Having a review >> process is no excuse for not writing good code and testing it properly. >> >>> >>> # reviewer has as quick look. "Looks good - in fact I don't care >>> much, it's not my code and need to return to my own issues.. worst >>> case someone else will fix it blaming the original author" >>> >>> And then again some incomplete test makes it to master, or a patch >>> which doesn't even compile is integrated. >>> >>> This pull request process is being a big failure. Shall we stop >>> wasting time on it and just push on master? >> >> -1. We should fix the process. > > Ok we can work on that too of course. but I also think we should relax > the pull request rule a bit. I pushed two obviously easy patches today > directly as I was sure of them and they would fix broken tests. > Of course, I take responsibility of that, but ultimately I feel doing > it to not waste time of others on such a trivial and obvious patch. > > Also, I still value pull requests in those areas in which discussion > is needed, or complex things are changes; but in those cases the > complexity (i.e. responsibility) should really not be pushed to the > reviewer: it's the author who should labour his commits in a clear > sequence with enough comments and tests to make everyone comfortable. > > Another thing which occasionally happens is that a messy pull request > is fine, but very complex to review. I'm pretty sure it happened to > everybody at least once to have the feeling they are taking more time > to review something than it took the original author to write it; > that's also a smelly sign of quality and the process overall. > People should make sure their pull requests are straight forward to > understand, not only by the reviewer but by anyone looking at the > history, including himself after a year.
Yes, and the reviewer should reject such pull requests. It isn't just code quality that the reviewer is looking for, but style as well. > > >>> Which doesn't mean I'm suggesting "let's make it worse" | "unleash >>> hell": we should all take responsibility on any change very seriously. >>> >>> Again, I'm not enjoying the role of "whom who complains on the >>> testsuite again". Just stating a fact, and trying to propose something >>> to make it work better. We have great individuals on this team, but we >>> need to admit that team work isn't working and we should deal with it >>> at it's best; denying it won't help. >> >> I'll take over that role. One thing I'd like to propose: >> >> 1. The reviewer is ultimately responsible for the patch. If a broken patch >> makes it to upstream it is the reviewer's fault. >> 2. The patch author is responsible for wasting reviewer's time with bad >> patches. > > The problem with such a game is that we lack reviewers, or are often > slow at it. That's one of my reasons to avoid pull requests, > especially when related to "fix the current build": it takes too long. Sure, for such small, targeted patches as per your example, that makes sense. But maybe a quick discussion with at least one other reviewer on IRC about what's going to happen makes sense. > >> >> I think we should make a game of this: we have a "bad commits" jar. Each >> time a reviewer pushes a bad patch up, he needs to put $1 into the jar. The >> next time, $(1 << 2), the next $(1 << 3), etc. :) > > Such a game would discourage people from doing reviews, which is the > opposite of what we need. No, reviews are necessary. The project doesn't progress without it, full stop. > >> To prevent incomplete pull requests wasting reviewer's time, the reviewer is >> allowed to impose a similar penalty on the patch author. Naturally, this >> only covers blatantly bad patches - i.e., incomplete tests, broken builds - >> and not genuine feedback on the patch, etc. >> >> And of course the "bad commits jar" is used to buy beer the next time we all >> meet. Inspired by a tweet Sanne fwd'd a while back. ;) >> >> If we're happy with this, I'll maintain the accounting spreadsheet for this >> bad commits jar. >> >> - M >> -- >> Manik Surtani >> ma...@jboss.org >> twitter.com/maniksurtani >> >> Lead, Infinispan >> http://www.infinispan.org > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Manik Surtani ma...@jboss.org twitter.com/maniksurtani Lead, Infinispan http://www.infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev
