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

[Martin Basti]

On 22.03.2016 13:50, Martin Basti wrote:
>
>
> On 21.03.2016 09:11, Oleg Fayans wrote:
>> 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
>>
> Patch 0029 pushed to:
> master: c2042900382190b1c9d7a44bd719cacd804749b3
> ipa-4-3: 1d5b8b8781e5d6300c5029bdd68c6ddf98f6ecd3
>
>
> Patch 27 is on review
>
ACK

Pushed to:
ipa-4-3: 2ddae844dca5860398a46979ecf984f1cfd9a6ac
master: d58cd04e8a618b0bf33d36099f782149c93dbd33