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

[Martin Basti]

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