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

[Oleg Fayans]

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

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0033.2-Improve-reporting-of-failed-tests-in-topology-test.patch
Type: text/x-patch
Size: 5871 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160406/9a365e09/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0034.1-Bugfixes-in-managed-topology-tests.patch
Type: text/x-patch
Size: 2232 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160406/9a365e09/attachment-0001.bin>