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

[Martin Babinsky]

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.

-- 
Martin^3 Babinsky