[Freeipa-devel] patch acceptance criteria

[Petr Spacek]

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