Petr Spacek wrote: > On 3.12.2015 16:07, Rob Crittenden wrote: >> Petr Spacek wrote: >>> On 3.12.2015 15:34, Rob Crittenden wrote: >>>> Martin Kosek wrote: >>>>> On 12/03/2015 09:08 AM, Petr Spacek wrote: >>>>>> On 2.12.2015 19:14, Rob Crittenden wrote: >>>>>>> Is it still mandatory that tests pass the unit tests before acceptance? >>>>>>> I've seen a number of cases over the past couple of months where a >>>>>>> change goes through then shortly afterward a patch to fix the tests. >>>>>>> IMHO this should be caught in advance. >>>>>>> >>>>>>> Things slip through and goodness knows I've acked more than a few >>>>>>> patches without running the full suite. I just have a feeling it has >>>>>>> become more frequent lately. >>>>>> >>>>>> When we are at it... An automated thingy which accepts URL to a Git >>>>>> repo, does >>>>>> all the test magic, and spits out test results without user interaction >>>>>> would >>>>>> be an awesome Christmas present! >>>>>> >>>>>> Bonus points if we can get Github integration so I can just push and >>>>>> have it >>>>>> tested automatically so I cannot forget to do that before sending the >>>>>> patch >>>>>> for review. >>>>> >>>>> +1. Having basic CI test suite run on top of a Pull Request would be >>>>> awesome. >>>>> >>>> >>>> I'd be happy with just the ipatests being run manually with each review. >>>> >>>> And it's then reviewer that I'm focusing on here. A developer _should_ >>>> also run the tests but part of the reviewer's responsibility is to >>>> ensure the patch does what it says it does without breaking things. >>> >>> Sure. I'm just saying that people are notoriously bad automation tool, so I >>> would like to off-load this kind of checks to a tool which will not forget >>> or >>> be lazy :-) >>> >> >> Sure, but it's the _job_ of the reviewer to do these things. My previous >> statement is better read as: A developer _shall_ also run the tests. >> >> And even beyond that reviewers need to ensure that the patch works, does >> it properly in the context of IPA, doesn't break anything, works in >> SELinux enforcing mode, updates man pages, etc. >> >> In other words, when a patch breaks something, don't blame the >> developer, blame the reviewer. > > I would blame both ;-) > > Again, I'm not against proper reviews, just saying that it simply does not > scale, so it needs automation. >
I haven't worked on IPA for a while so won't comment on the scaling other than to say it's always been a busy project and reviews are always a trade-off. I don't want to devolve this into automation as a panacea. There are steps for review that aren't being followed either closely enough or at all. That's what I'm trying to address. There should not be follow-up patches to fix tests that are failing. There should not be follow-up patches to add missing manpage information. And that's on the reviewer. That's all I'm saying. rob -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
