[Freeipa-devel] [TEST][Patch-0027] Fixed test failure during in-tree session, ticket N 5736

[Oleg Fayans]

Hi Martin,

On 03/16/2016 03:35 PM, Martin Basti wrote:
> 
> 
> On 16.03.2016 15:13, Martin Basti wrote:
>>
>>
>> On 16.03.2016 14:59, Oleg Fayans wrote:
>>> Hi Martin
>>>
>>> On 03/16/2016 02:39 PM, Martin Basti wrote:
>>>>
>>>> On 16.03.2016 10:59, Oleg Fayans wrote:
>>>>> With this patch applied integration tests pass and in-tree tests are
>>>>> gracefully skipped.
>>>>>
>>>>> @mkubik, It is not possible to put the decorator to util.py as per our
>>>>> discussion, because it uses tasks, so tasks must be imported. But
>>>>> tasks
>>>>> already import util, which leads to circular imports. So I've put
>>>>> it to
>>>>> tasks.py
>>>>>
>>>>>
>>>>>
>>>> NACK
>>>>
>>>> 1)
>>>> Use right ticket in commit message (#5723)
>>> But (#5736) is exactly the issue that is being addressed. Probably note
>>> both tickets in the commit message?
>> But as I wrote in ticket #5736, this ticket should be closed, because
>> issue is caused by ticket which is not finished yet, so we should
>> continue just with original ticket.

Done

>>
>>>
>>>> 2)
>>>> Link to ticket should be last in the commit message

Done

>>>>
>>>> 3)
>>>> dereplicafy
>>>>
>>>> 3a)
>>>> wrong doc string, it removes *only* replicas not clients
>>> No, in fact it removes both:
>>> uninstall_replica(args[0].master, host)
>>> uninstall_client(host)
>>>
>>> Both tasks have raiseonerr set to False, which means that even if
>>> replica was not installed but the client was - it will also be removed
>> I see just
>> for host in args[0].replicas
>>
>> I don't see any
>> for host in args[0].clients
>> there
>>
>> Also uninstall_client should not be there. ipa-server-install
>> --uninstall removes client too. The extra call of uninstall client is
>> IMO there just because an ancient bug that is already fixed.

That's done because some tests install client separately and then
deliberately install replica the wrong way to test that the installer
fails in a predicted way. That's why this separate uninstall_client
call. The doc string was corrected.


>>
>>>
>>>> 3b)
>>>> can we rename it to something different? (replicas_cleanup,
>>>> replicas_uninstall, replicas_teardown)
>>> replicas_cleanup, or even topo_cleanup sounds OK to me.

replicas_cleanup it is

>>>
>>>> 4)
>>>> Please fix commit message
>>>> - Wile trated correctly
>>>> - followiong
>>>> - rewrote -> rewrite
>>> Will do

Done

>>>
>>>> 5)
>>>> decorator
>>>> +    def wrapped(*args):
>>>> +        func(*args)
>>>> +        for host in args[0].replicas:
>>>>
>>>> Shouldn't be there try-finally around func() call, or something?
>>> No, the wrapped function is a test_* method: if it fails we need to see
>>> the original failure
>> but if something raise an exception in func(), cleanup will not be
>> executed.
>>
>> You can do
>> In [4]: try:
>>    ...:     raise ValueError('Hello')
>>    ...: finally:
>>    ...:     try:
>>    ...:         raise ValueError('Cleanup')
>>    ...:     except Exception:
>>    ...:         pass
>>    ...:
>> ---------------------------------------------------------------------------
>>
>> ValueError                                Traceback (most recent call
>> last)
>> <ipython-input-4-affb927f7603> in <module>()
>>       1 try:
>> ----> 2     raise ValueError('Hello')
>>       3 finally:
>>       4     try:
>>       5         raise ValueError('Cleanup')
>>
>> ValueError: Hello
> On the other hand, I do not want cleanup with --pdb option, so maybe it
> should just fail
> 
>>
>>>
>>>> Are you sure that there is no need to return result of func()?
>>> The same applies here: we never return results from test_* methods
>> ok
>>>
>>>> *) Please create additional patch that will add licence there
>>>>
>>>>
>>> Will do :)
>>>
>>>
>>
> 

The license-related patch is attached too

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0027.1-rewrote-a-misprocessed-teardown_method-method.patch
Type: text/x-patch
Size: 4655 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160321/87db5e31/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0029-Added-copyright-info-to-replica-promotion-tests.patch
Type: text/x-patch
Size: 919 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160321/87db5e31/attachment-0001.bin>