Simplify wait_until function and friends

The previous version of these functions worked, but I found its design
was a bit over-complicated. I believe this also led to using it
incorrectly, with some calls doing a "double wait_until" invocation,
where an outer wait_until call in the test code used as a condition a
function that itself used wait_until.

This commit re-designs the wait_until function and its callers. The
first difference in this new version is that the timeout and interval
values always use their default values (although they can still be set
by on each invocation), as in the previous version all invocations
passed the same value explicitly. Secondly, instead of passing an update
function and a set of arguments to call it with, we simply pass an
update function, which in the caller site is bound to local arguments
(using a lambda in our cases, but could be a functools.partial if
required). These two changes simplified the function calling convention
a bit.

Finally, I've removed the "double wait_until" instances with what I
believe is a correct replacement. While I'm not particularly sure I've
done the right thing, I still see the same test code coverage, and all
tests are still passing. This leads me to believe I did the right thing,
but I will need confirmation. As a bonus, this change reduced the test
walltime from ~110 seconds to ~< 60.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>

daliuge-engine/test/manager/test_daemon.py

@@ -27,48 +27,28 @@
 
 from dlg import utils, restutils
 from dlg.manager import constants
-from dlg.manager.client import MasterManagerClient, DataIslandManagerClient
+from dlg.manager.client import MasterManagerClient
 from dlg.manager.proc_daemon import DlgDaemon
-from six.moves import http_client as httplib  # @UnresolvedImport
 
 _TIMEOUT = 10
+IDENTITY = lambda x: x
 
-
-def _wait_until(update_condition, test_condition, timeout, interval=0.1, *args):
+def wait_until(update_condition, test_condition=IDENTITY, timeout=_TIMEOUT, interval=0.1):
     timeout_time = time.time() + timeout
-    output = None
-    while not test_condition(output) and time.time() < timeout_time:
-        output = update_condition(*args)
+    while time.time() < timeout_time:
+        output = update_condition()
+        if test_condition(output):
+            return output
         time.sleep(interval)
-    return output
-
-
-def _get_dims_from_client(timeout, client):
-    def _update_nodes(*args):
-        return args[0].dims()
-
-    def _test_dims(_dims):
-        return _dims
-
-    dims = _wait_until(_update_nodes, _test_dims, timeout, 0.1, client)
-    return dims
-
-
-def _get_nodes_from_client(timeout, client):
-    def _update_nodes(*args):
-        return args[0].nodes()
+    return None
 
-    def _test_nodes(_nodes):
-        if not _nodes or len(_nodes) == 0:
-            return False
-        return _nodes
 
-    nodes = _wait_until(_update_nodes, _test_nodes, timeout, 0.1, client)
-    return nodes
+def _get_dims_from_client(client, **kwargs):
+    return wait_until(lambda: client.dims(), **kwargs)
 
 
-def _update_nodes_with_timeout(*args):
-    return _get_nodes_from_client(_TIMEOUT, args[0])
+def _get_nodes_from_client(client, **kwargs):
+    return wait_until(lambda: client.nodes(), **kwargs)
 
 
 class TestDaemon(unittest.TestCase):
@@ -158,7 +138,7 @@ def test_zeroconf_discovery(self):
         # if we query the MM it should know about its nodes, which should have
         # one element
         mc = MasterManagerClient()
-        nodes = _get_nodes_from_client(_TIMEOUT, mc)
+        nodes = _get_nodes_from_client(mc)
         self.assertIsNotNone(nodes)
         self.assertEqual(
             1,
@@ -167,29 +147,20 @@ def test_zeroconf_discovery(self):
         )
 
     def _test_zeroconf_dim_mm(self, disable_zeroconf=False):
-        # Start daemon with no master and no NM
+        # Start an empty daemon, then a DIM and a Master on their own
         self.create_daemon(master=False, noNM=True, disable_zeroconf=disable_zeroconf)
-        # Start DIM - now, on it's own
         self._start("island", http.HTTPStatus.OK, {"nodes": []})
-        # Start daemon with master but no NM
         self._start("master", http.HTTPStatus.OK)
-        # Check that dim registers to MM
-        dims = None
-        mc = MasterManagerClient()
-
-        def _update_dims(*args):
-            _dims = _get_dims_from_client(_TIMEOUT, args[0])
-            return _dims
 
-        def _test_dims(_dims):
-            if dims is not None and len(dims["islands"]) > 0:
-                return dims
-            else:
-                return False
-
-        dims = _wait_until(_update_dims, _test_dims, _TIMEOUT, 0.1, mc)
-        self.assertIsNotNone(dims)
-        return dims
+        # Check that dim registers to MM when using zeroconf
+        mc = MasterManagerClient()
+        if not disable_zeroconf:
+            def _test_dims(dims):
+                return dims and dims["islands"]
+            dims = _get_dims_from_client(mc, test_condition=_test_dims)
+            self.assertIsNotNone(dims)
+            return dims
+        return mc.dims()
 
     def test_zeroconf_dim_mm(self):
         dims = self._test_zeroconf_dim_mm(disable_zeroconf=False)
@@ -210,15 +181,7 @@ def test_without_zeroconf_dim_mm(self):
     def _add_zeroconf_nm(self):
         self._start("node", http.HTTPStatus.OK)
         mc = MasterManagerClient()
-
-        def _test_nodes(_nodes):
-            if _nodes is not None and len(_nodes) > 0:
-                return _nodes
-            else:
-                return False
-
-        nodes = _wait_until(_update_nodes_with_timeout, _test_nodes, _TIMEOUT, 0.1, mc)
-        return nodes
+        return _get_nodes_from_client(mc)
 
     def test_zeroconf_dim_nm_setup(self):
         """
@@ -242,14 +205,10 @@ def test_zeroconf_nm_down(self):
         self._stop("node", http.HTTPStatus.OK)
         mc = MasterManagerClient()
 
-        def _test_nodes(_nodes):
-            if not _nodes:
-                return False
-            if not _nodes['nodes']:
-                return _nodes['nodes']
-            return False
+        def _test_nodes(nodes):
+            return not nodes['nodes']
 
-        new_nodes = _wait_until(_update_nodes_with_timeout, _test_nodes, _TIMEOUT, 0.1, mc)['nodes']
+        new_nodes = _get_nodes_from_client(mc, test_condition=_test_nodes)['nodes']
         self.assertEqual(0, len(new_nodes))
 
     def test_start_dataisland_via_rest(self):
@@ -260,7 +219,7 @@ def test_start_dataisland_via_rest(self):
         # if we query the MM it should know about its nodes, which should have
         # one element
         mc = MasterManagerClient()
-        nodes = _get_nodes_from_client(_TIMEOUT, mc)
+        nodes = _get_nodes_from_client(mc)
         self.assertIsNotNone(nodes)
         self.assertEqual(
             1,
@@ -280,14 +239,14 @@ def test_stop_dataisland_via_rest(self):
         # start master and island manager
         self.create_daemon(master=True, noNM=False, disable_zeroconf=False)
         mc = MasterManagerClient()
-        nodes = _get_nodes_from_client(_TIMEOUT, mc)
+        nodes = _get_nodes_from_client(mc)
         self._start("island", http.HTTPStatus.OK, {"nodes": nodes})
 
         # Both managers started fine. If they zeroconf themselves correctly then
         # if we query the MM it should know about its nodes, which should have
         # one element
         mc = MasterManagerClient()
-        nodes = _get_nodes_from_client(_TIMEOUT, mc)
+        nodes = _get_nodes_from_client(mc)
         self.assertIsNotNone(nodes)
         self.assertEqual(
             1,
@@ -311,7 +270,7 @@ def test_stop_start_node_via_rest(self):
         # if we query the MM it should know about its nodes, which should have
         # one element
         mc = MasterManagerClient()
-        nodes = _get_nodes_from_client(_TIMEOUT, mc)
+        nodes = _get_nodes_from_client(mc)
         self.assertIsNotNone(nodes)
         self.assertEqual(
             1,
@@ -355,7 +314,7 @@ def test_get_dims(self):
         self.create_daemon(master=True, noNM=True, disable_zeroconf=False)
         # Check that the DataIsland starts with the given nodes
         mc = MasterManagerClient()
-        dims = _get_dims_from_client(_TIMEOUT, mc)
+        dims = _get_dims_from_client(mc)
         self.assertIsNotNone(dims)
         self.assertEqual(
             0,