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

[Martin Basti]

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