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.
From 6ce1ad871d2f83558ee64f58bd010d0219c8e3e7 Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Wed, 6 Apr 2016 10:46:58 +0200
Subject: [PATCH] Improve reporting of failed tests in topology test suite

https://fedorahosted.org/freeipa/ticket/5772
---
 ipatests/test_integration/test_topology.py | 39 ++++++++++++++++--------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/ipatests/test_integration/test_topology.py b/ipatests/test_integration/test_topology.py
index 8119aa9d7c9a57e68a89ef1d8086920286e64316..c434c6c441a4d73662eb9805df67c51c69cbf3c0 100644
--- a/ipatests/test_integration/test_topology.py
+++ b/ipatests/test_integration/test_topology.py
@@ -11,6 +11,7 @@ from ipatests.test_integration.base import IntegrationTest
 from ipatests.test_integration import tasks
 from ipatests.test_integration.env_config import get_global_config
 from ipalib.constants import DOMAIN_SUFFIX_NAME
+from ipatests.util import assert_deepequal
 
 config = get_global_config()
 reasoning = "Topology plugin disabled due to domain level 0"
@@ -61,15 +62,16 @@ class TestTopologyOptions(IntegrationTest):
         """
         tasks.kinit_admin(self.master)
         result1 = self.master.run_command(['ipa', 'topologysegment-find',
-                                           DOMAIN_SUFFIX_NAME])
+                                           DOMAIN_SUFFIX_NAME]).stdout_text
         first_segment_name = "%s-to-%s" % (self.master.hostname,
                                            self.replicas[0].hostname)
-        output1 = result1.stdout_text
-        firstsegment = self.tokenize_topologies(output1)[0]
-        assert(firstsegment['name'] == first_segment_name)
-        assert(self.noentries_re.search(output1).group(1) == "1")
-        assert(firstsegment['leftnode'] == self.master.hostname)
-        assert(firstsegment['rightnode'] == self.replicas[0].hostname)
+        expected_segment = {
+            'connectivity': 'both',
+            'leftnode': self.master.hostname,
+            'name': first_segment_name,
+            'rightnode': self.replicas[0].hostname}
+        firstsegment = self.tokenize_topologies(result1)[0]
+        assert_deepequal(expected_segment, firstsegment)
         tasks.install_replica(self.master, self.replicas[1], setup_ca=False,
                               setup_dns=False)
         # We need to make sure topology information is consistent across all
@@ -81,16 +83,17 @@ class TestTopologyOptions(IntegrationTest):
         result4 = self.replicas[1].run_command(['ipa', 'topologysegment-find',
                                                 DOMAIN_SUFFIX_NAME])
         segments = self.tokenize_topologies(result2.stdout_text)
-        assert(len(segments) == 2)
-        assert(result2.stdout_text == result3.stdout_text)
-        assert(result3.stdout_text == result4.stdout_text)
+        assert(len(segments) == 2), "Unexpected number of segments found"
+        assert_deepequal(result2.stdout_text, result3.stdout_text)
+        assert_deepequal(result3.stdout_text,  result4.stdout_text)
         # Now let's check that uninstalling the replica will update the topology
         # info on the rest of replicas.
         tasks.uninstall_master(self.replicas[1])
         tasks.clean_replication_agreement(self.master, self.replicas[1])
         result5 = self.master.run_command(['ipa', 'topologysegment-find',
                                            DOMAIN_SUFFIX_NAME])
-        assert(self.noentries_re.search(result5.stdout_text).group(1) == "1")
+        num_entries = self.noentries_re.search(result5.stdout_text).group(1)
+        assert(num_entries == "1"), "Incorrect number of entries displayed"
 
     def test_add_remove_segment(self):
         """
@@ -110,8 +113,9 @@ class TestTopologyOptions(IntegrationTest):
         assert err == "", err
         # Make sure the new segment is shown by `ipa topologysegment-find`
         result1 = self.master.run_command(['ipa', 'topologysegment-find',
-                                           DOMAIN_SUFFIX_NAME])
-        assert(result1.stdout_text.find(segment['name']) > 0)
+                                           DOMAIN_SUFFIX_NAME]).stdout_text
+        assert(segment['name'] in result1), (
+            "%s: segment not found" % segment['name'])
         # Remove master <-> replica2 segment and make sure that the changes get
         # there through replica1
         deleteme = "%s-to-%s" % (self.master.hostname,
@@ -120,17 +124,16 @@ class TestTopologyOptions(IntegrationTest):
         assert returncode == 0, error
         # make sure replica1 does not have segment that was deleted on master
         result3 = self.replicas[0].run_command(['ipa', 'topologysegment-find',
-                                               DOMAIN_SUFFIX_NAME])
-        assert(result3.stdout_text.find(deleteme) < 0)
+                                               DOMAIN_SUFFIX_NAME]).stdout_text
+        assert(deleteme not in result3), "%s: segment still exists" % deleteme
         # Create test data on master and make sure it gets all the way down to
         # replica2 through replica1
         self.master.run_command(['ipa', 'user-add', 'someuser',
                                  '--first', 'test',
                                  '--last', 'user'])
         time.sleep(60)  # replication requires some time
-        users_on_replica2 = self.replicas[1].run_command(['ipa',
-                                                         'user-find'])
-        assert(users_on_replica2.find('someuser') > 0)
+        result4 = self.replicas[1].run_command(['ipa', 'user-find'])
+        assert('someuser' in result4.stdout_text), 'User not found: someuser'
         # We end up having a line topology: master <-> replica1 <-> replica2
 
     def test_remove_the_only_connection(self):
-- 
1.8.3.1

From c00877714b67ef870ce63fa408371e19741db82e Mon Sep 17 00:00:00 2001
From: Oleg Fayans <ofay...@redhat.com>
Date: Wed, 6 Apr 2016 11:20:38 +0200
Subject: [PATCH] Bugfixes in managed topology tests

Fixed a false negative related to replication taking some time: added
wait_for_replication call before checking for new object in replicas.
---
 ipatests/test_integration/test_topology.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/ipatests/test_integration/test_topology.py b/ipatests/test_integration/test_topology.py
index c434c6c441a4d73662eb9805df67c51c69cbf3c0..e956563c27eafd84deed5786274a73d0d3594642 100644
--- a/ipatests/test_integration/test_topology.py
+++ b/ipatests/test_integration/test_topology.py
@@ -3,7 +3,6 @@
 #
 
 import re
-import time
 
 import pytest
 
@@ -122,7 +121,10 @@ class TestTopologyOptions(IntegrationTest):
                                  self.replicas[1].hostname)
         returncode, error = tasks.destroy_segment(self.master, deleteme)
         assert returncode == 0, error
-        # make sure replica1 does not have segment that was deleted on master
+        # Wait till replication ends and make sure replica1 does not have
+        # segment that was deleted on master
+        replica1_ldap = self.replicas[0].ldap_connect()
+        tasks.wait_for_replication(replica1_ldap)
         result3 = self.replicas[0].run_command(['ipa', 'topologysegment-find',
                                                DOMAIN_SUFFIX_NAME]).stdout_text
         assert(deleteme not in result3), "%s: segment still exists" % deleteme
@@ -131,7 +133,8 @@ class TestTopologyOptions(IntegrationTest):
         self.master.run_command(['ipa', 'user-add', 'someuser',
                                  '--first', 'test',
                                  '--last', 'user'])
-        time.sleep(60)  # replication requires some time
+        dest_ldap = self.replicas[1].ldap_connect()
+        tasks.wait_for_replication(dest_ldap)
         result4 = self.replicas[1].run_command(['ipa', 'user-find'])
         assert('someuser' in result4.stdout_text), 'User not found: someuser'
         # We end up having a line topology: master <-> replica1 <-> replica2
-- 
1.8.3.1

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