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. -- Petr^2 Spacek -- 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
