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

[Martin Basti]

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.

>
>> 2)
>> Link to ticket should be last in the commit message
>>
>> 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.

>
>> 3b)
>> can we rename it to something different? (replicas_cleanup,
>> replicas_uninstall, replicas_teardown)
> replicas_cleanup, or even topo_cleanup sounds OK to me.
>
>> 4)
>> Please fix commit message
>> - Wile trated correctly
>> - followiong
>> - rewrote -> rewrite
> Will do
>
>> 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

>
>> 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 :)
>
>