From 56a60cf904cf9392eaa25060540b3ed8c9895b06 Mon Sep 17 00:00:00 2001 From: Rodrigo Tobar Date: Wed, 1 Jun 2022 16:02:31 +0800 Subject: [PATCH] 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 --- daliuge-engine/test/manager/test_daemon.py | 105 +++++++-------------- 1 file changed, 32 insertions(+), 73 deletions(-) diff --git a/daliuge-engine/test/manager/test_daemon.py b/daliuge-engine/test/manager/test_daemon.py index e7a66bb60..80c7a0aec 100644 --- a/daliuge-engine/test/manager/test_daemon.py +++ b/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,