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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to