[Freeipa-devel] [TEST][patch-0033] Added assertion errors to topology tests, track N 5772

[Martin Basti]

On 12.04.2016 16:15, Martin Babinsky wrote:
> On 04/06/2016 02:40 PM, Oleg Fayans wrote:
>> Hi Martin,
>>
>> The updated patches are attached
>>
>> On 04/04/2016 06:46 PM, Martin Babinsky wrote:
>>> On 03/31/2016 05:15 PM, Oleg Fayans wrote:
>>>> Hi Martin,
>>>>
>>>> Thanks for the review. The updated patch(es) are included
>>>>
>>>> Testrun output can be found here:
>>>>
>>>> http://fpaste.org/347800/59421745/
>>>>
>>>> On 03/31/2016 01:10 PM, Martin Basti wrote:
>>>>>
>>>>>
>>>>> On 31.03.2016 12:07, Oleg Fayans wrote:
>>>>>> Please, disregard it for a while, it does not pass lint.
>>>>>>
>>>>>> On 03/31/2016 12:05 PM, Oleg Fayans wrote:
>>>>>>>
>>>>>>>
>>>>> NACK
>>>>>
>>>>> Please send unrelated changes in separate patches. I do not see 
>>>>> relation
>>>>> between changing variable names, adding assertion messages and 
>>>>> setting
>>>>> replication sleep-a-bit wait.
>>>>
>>>> Agreed. There are two necessary bugfixes for the testsuite to run. 
>>>> They
>>>> were put into a separate patch
>>>>
>>>>>
>>>>> IMO to the ticket in the patch only assertion changes are related.
>>>>>
>>>>> For the pylint related errors:
>>>>> assert ('any value', 'in tuple')
>>>>> is always true.
>>>>> right syntax is
>>>>> assert (any test), ('error msg')
>>>>
>>>> thank you!
>>>>
>>>>>
>>>>> Martin^2
>>>>
>>>>
>>>>
>>> Hi Oleg,
>>>
>>> Patch 0033:
>>>
>>> 1.)
>>> First of all, the commit message does not tell much about what the 
>>> patch
>>> does, I would prefer something like "Improve reporting of failed tests
>>> in topology test suite".
>>
>> Done
>>
>>>
>>> 2.) Since what you are doing is basically comparing the contents of a
>>> list of dicts containing expected data with a list of dicts with data
>>> parsed from test results, why are you not using "assert_deepequal"
>>> function from 'ipatests/util.py' module? It is also used in e.g. XMLRPC
>>> tests and it reports the failures fairly well (e.g. redundant keys,
>>> missing keys, expected value mismatch, container length mismatch etc.)
>>
>> Indeed :) Done.
>>
>>>
>>> Patch 034:
>>>
>>> I think this is a good use case for 'wait_for_replication' function 
>>> from
>>> 'ipatests/test_integration/tasks.py' module. You need to establish LDAP
>>> connection to the host but this is very easy, see how it's done in
>>> 'test_simple_replication' suite[1].
>>>
>>> I would prefer this approach (if applicable) instead of using sleep().
>>
>> Implemented.
>>
>>>
>>> In any way, your formatting of assertions is wrong and makes the code
>>> nearly unreadable. See the attached patch for an example of pep8
>>> compliant and pleasant formatting
>>
>> Thank you, that looks really way more readable.
>>
>>>
>>> [1]
>>> https://git.fedorahosted.org/cgit/freeipa.git/tree/ipatests/test_integration/test_simple_replication.py#n42 
>>>
>>>
>>>
>>>
>>>
>>
>
> Thanks, ACK.
>
Pushed

master:
* 1974f20aec8de61d0e8d5a550df6a5fabd4b383a Improve reporting of failed 
tests in topology test suite
* 1c79c1ea2d077d8699c7e3190526a45e627a7a18 Bugfixes in managed topology 
tests
ipa-4-3:
* b41164f237341ccf2546b73585e804aac4cffc8e Improve reporting of failed 
tests in topology test suite
* 10e9e33ac057d5ad4e42cd7207b7203ce1d100d1 Bugfixes in managed topology 
tests