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

[Martin Babinsky]

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".

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

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

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

[1] 
https://git.fedorahosted.org/cgit/freeipa.git/tree/ipatests/test_integration/test_simple_replication.py#n42

-- 
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: assert_fmt.patch
Type: text/x-patch
Size: 4540 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160404/7477a8bc/attachment.bin>