[Freeipa-devel] patch acceptance criteria

[Rob Crittenden]

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.

rob