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.
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.
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. :)
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
manik(a)jboss.org
twitter.com/maniksurtani
Lead, Infinispan
http://www.infinispan.org