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

Reply via email to