URL: https://github.com/SSSD/sssd/pull/345
Author: fidencio
 Title: #345: Improve the situation of recurrent failing CI tests
Action: opened

PR body:
"""
Taking a look in our CI the latest issues we had were related to not waiting 
enough time in some sleep() that are around the tests code.

For a more detailed explanation, please, take a look on each patch of this 
small series.

Last 6 CI runs were successful (all triggered with these patches). There were 
still 5 runs to go, but those jobs were aborted without any kind of notice. :-\
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/345/head:pr345
git checkout pr345
From 25761e51e2938684fa7895a08b54fad51d9a92c5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com>
Date: Wed, 9 Aug 2017 07:59:41 +0200
Subject: [PATCH 1/2] INTG: Increase the sleep() time so the changes are
 reflected on SSSD
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Those tests have been failing a lot recently and it does happen becase
the time to reflect the changes on SSSD is not enough for the machine
where the tests are running.

There's no reasonable explanation in the code why 4 seconds is used as
INTERACTIVE_TIMEOUT, neither a reasonable explanation why 2 seconds is
used as the time waited in order to have those changes reflected on
SSSD (neither in the code nor in the commit messages).

This patch uses the most simple empiric way to determine a better value
for this timeout, which was "run the tests a considerable amount of time
and check that there were no failures".

So, in order to avoid failures and our tests giving us more reliable
information, let's give more time so the changes are reflected on SSSD.

Resolves:
https://pagure.io/SSSD/sssd/issue/3463

Signed-off-by: Fabiano FidĂȘncio <fiden...@redhat.com>
---
 src/tests/intg/test_enumeration.py | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/tests/intg/test_enumeration.py b/src/tests/intg/test_enumeration.py
index 47772dea2..24175fe05 100644
--- a/src/tests/intg/test_enumeration.py
+++ b/src/tests/intg/test_enumeration.py
@@ -32,6 +32,15 @@
 from util import *
 
 LDAP_BASE_DN = "dc=example,dc=com"
+
+# There is no explation neither in the code nor in the commit message that
+# introduced this timeout why 4 was chosen as value. The very same happens
+# with respect to why the time we should wait in order to have the changes
+# reflected on SSSD is INTERACTIVE_TIMEOUT/2.
+# Having INTERACTIVE_TIMEOUT/2 has been causing a lot of failures in some of
+# our CI tests, so it's been changed to INTERACTIVE_TIMEOUT and the way it's
+# been tested was just empirically by running or CI several times and checking
+# whether a failure happened or not.
 INTERACTIVE_TIMEOUT = 4
 
 
@@ -406,7 +415,7 @@ def user_and_groups_rfc2307_bis(request, ldap_conn):
 def test_add_remove_user(ldap_conn, blank_rfc2307):
     """Test user addition and removal are reflected by SSSD"""
     e = ldap_ent.user(ldap_conn.ds_inst.base_dn, "user", 2001, 2000)
-    time.sleep(INTERACTIVE_TIMEOUT/2)
+    time.sleep(INTERACTIVE_TIMEOUT)
     # Add the user
     ent.assert_passwd(ent.contains_only())
     ldap_conn.add_s(*e)
@@ -421,7 +430,7 @@ def test_add_remove_user(ldap_conn, blank_rfc2307):
 def test_add_remove_group_rfc2307(ldap_conn, blank_rfc2307):
     """Test RFC2307 group addition and removal are reflected by SSSD"""
     e = ldap_ent.group(ldap_conn.ds_inst.base_dn, "group", 2001)
-    time.sleep(INTERACTIVE_TIMEOUT/2)
+    time.sleep(INTERACTIVE_TIMEOUT)
     # Add the group
     ent.assert_group(ent.contains_only())
     ldap_conn.add_s(*e)
@@ -436,7 +445,7 @@ def test_add_remove_group_rfc2307(ldap_conn, blank_rfc2307):
 def test_add_remove_group_rfc2307_bis(ldap_conn, blank_rfc2307_bis):
     """Test RFC2307bis group addition and removal are reflected by SSSD"""
     e = ldap_ent.group_bis(ldap_conn.ds_inst.base_dn, "group", 2001)
-    time.sleep(INTERACTIVE_TIMEOUT/2)
+    time.sleep(INTERACTIVE_TIMEOUT)
     # Add the group
     ent.assert_group(ent.contains_only())
     ldap_conn.add_s(*e)
@@ -450,7 +459,7 @@ def test_add_remove_group_rfc2307_bis(ldap_conn, blank_rfc2307_bis):
 
 def test_add_remove_membership_rfc2307(ldap_conn, user_and_group_rfc2307):
     """Test user membership addition and removal are reflected by SSSD"""
-    time.sleep(INTERACTIVE_TIMEOUT/2)
+    time.sleep(INTERACTIVE_TIMEOUT)
     # Add user to group
     ent.assert_group_by_name("group", dict(mem=ent.contains_only()))
     ldap_conn.modify_s("cn=group,ou=Groups," + ldap_conn.ds_inst.base_dn,
@@ -472,7 +481,7 @@ def test_add_remove_membership_rfc2307_bis(ldap_conn,
     """
     base_dn_bytes = ldap_conn.ds_inst.base_dn.encode('utf-8')
 
-    time.sleep(INTERACTIVE_TIMEOUT/2)
+    time.sleep(INTERACTIVE_TIMEOUT)
     # Add user to group1
     ent.assert_group_by_name("group1", dict(mem=ent.contains_only()))
     ldap_conn.modify_s("cn=group1,ou=Groups," + ldap_conn.ds_inst.base_dn,

From 2cb8daa235436a234077c76cf7e82b5efc8624a2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com>
Date: Wed, 9 Aug 2017 08:03:48 +0200
Subject: [PATCH 2/2] INTG: Increase the sleep() time for test_idle_timeout
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The way the timeouts are calculated is not as precise as it could be and
this choice was made over having a lot of memory operations, please, see
560daa14ef0 for more details.

Keeping this in mind, let's increase the sleep() used in
test_idle_timeout so we ensure that the time (hopefully) will be enough.

This patch helps us to have our tests running in a more reliable way.

Related:
https://pagure.io/SSSD/sssd/issue/3463

Signed-off-by: Fabiano FidĂȘncio <fiden...@redhat.com>
---
 src/tests/intg/test_secrets.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/tests/intg/test_secrets.py b/src/tests/intg/test_secrets.py
index a66431ff4..eee107009 100644
--- a/src/tests/intg/test_secrets.py
+++ b/src/tests/intg/test_secrets.py
@@ -429,8 +429,10 @@ def test_idle_timeout(setup_for_cli_timeout_test):
     # because the internal timer ticks every timeout/2 seconds, so it would
     # tick at 5, 10 and 15 seconds and the client timeout check uses a
     # greater-than comparison, so the 10-seconds tick wouldn't yet trigger
-    # disconnect
-    time.sleep(15)
+    # disconnect. However, using a sleep for 15 seconds may still be error
+    # prone as the timeout is not as precise as it could be, thus let's
+    # sleep for 20 seconds.
+    time.sleep(20)
 
     nfds_post = get_num_fds(secpid)
     assert nfds_pre == nfds_post
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

Reply via email to