From 544034250b5f990793f48ee3adfeb2a4ebc3532a Mon Sep 17 00:00:00 2001 From: Ivan Pokhabov Date: Wed, 5 Aug 2026 15:59:25 +0300 Subject: [PATCH 1/2] test(scenario): centralize polling waits Replace repeated hand-written sleep/retry loops in scenario tests with shared wait_for and wait_for_completion helpers. Use common default timeouts for BGP state, reachability, and assertion retries so asynchronous checks behave consistently across router, route-server, EVPN, MUP, and zebra scenarios. Keep the tests focused on expected state while preserving retries for eventual convergence and improving timeout messages where useful. --- test/lib/base.py | 59 +++++++------- test/scenario_test/bgp_router_test.py | 76 ++++++------------- test/scenario_test/bgp_zebra_test.py | 48 +++++++----- test/scenario_test/evpn_test.py | 16 ++-- test/scenario_test/graceful_restart_test.py | 12 ++- test/scenario_test/ibgp_router_test.py | 44 +++-------- test/scenario_test/mup_test.py | 28 +++---- test/scenario_test/route_reflector_test.py | 16 +--- test/scenario_test/route_server_as2_test.py | 22 +++--- .../route_server_ipv4_v6_test.py | 44 +++++------ .../route_server_malformed_test.py | 16 +--- .../route_server_policy_grpc_test.py | 15 +--- .../scenario_test/route_server_policy_test.py | 15 +--- test/scenario_test/route_server_test.py | 61 +++++++-------- 14 files changed, 175 insertions(+), 297 deletions(-) diff --git a/test/lib/base.py b/test/lib/base.py index 27483847c..cbb7f9c98 100644 --- a/test/lib/base.py +++ b/test/lib/base.py @@ -30,6 +30,9 @@ DEFAULT_TEST_BASE_DIR = '/tmp/gobgp' TEST_PREFIX = DEFAULT_TEST_PREFIX TEST_BASE_DIR = DEFAULT_TEST_BASE_DIR +DEFAULT_WAIT_TIMEOUT = 120 +DEFAULT_REACHABILITY_TIMEOUT = 20 +DEFAULT_ASSERT_RETRIES = 30 BGP_FSM_IDLE = 'idle' BGP_FSM_ACTIVE = 'active' @@ -110,17 +113,23 @@ def community_str(i): return ':'.join(reversed(values)) -def wait_for_completion(f, timeout=120): - interval = 1 +def wait_for(f, timeout=DEFAULT_WAIT_TIMEOUT, interval=1, timeout_message='timeout'): count = 0 while True: if f(): return time.sleep(interval) - count += interval - if count >= timeout: - raise Exception('timeout') + if timeout is not None: + count += interval + if timeout is not None and count >= timeout: + if callable(timeout_message): + timeout_message = timeout_message() + raise Exception('{0} after {1}s'.format(timeout_message, timeout)) + + +def wait_for_completion(f, timeout=DEFAULT_WAIT_TIMEOUT): + wait_for(f, timeout=timeout) def try_several_times(f, t=3, s=1): @@ -134,7 +143,7 @@ def try_several_times(f, t=3, s=1): raise Exception -def assert_several_times(f, t=30, s=1): +def assert_several_times(f, t=DEFAULT_ASSERT_RETRIES, s=1): e = AssertionError for _ in range(t): try: @@ -533,7 +542,7 @@ def get_global_rib(self, rf): def get_neighbor_state(self, peer_id): raise Exception('implement get_neighbor() method') - def get_reachability(self, prefix, timeout=20): + def get_reachability(self, prefix, timeout=DEFAULT_REACHABILITY_TIMEOUT): version = netaddr.IPNetwork(prefix).version addr = prefix.split('/')[0] if version == 4: @@ -543,34 +552,30 @@ def get_reachability(self, prefix, timeout=20): else: raise Exception('unsupported route family: {0}'.format(version)) cmd = '/bin/bash -c "/bin/{0} -c 1 -w 1 {1} | xargs echo"'.format(ping_cmd, addr) - interval = 1 - count = 0 - while True: + + def _reachable(): res = self.local(cmd, capture=True) print(yellow(res)) - if ('1 packets received' in res or '1 received' in res) and '0% packet loss' in res: - break - time.sleep(interval) - count += interval - if count >= timeout: - raise Exception('timeout') + return ('1 packets received' in res or '1 received' in res) and '0% packet loss' in res + + wait_for(_reachable, timeout=timeout, timeout_message='reachability timeout') return True - def wait_for(self, expected_state, peer, timeout=120): - interval = 1 - count = 0 - while True: + def wait_for(self, expected_state, peer, timeout=DEFAULT_WAIT_TIMEOUT): + def _state_matches(): state = self.get_neighbor_state(peer) print(yellow("{0}'s peer {1} state: {2}".format(self.router_id, peer.router_id, state))) - if state == expected_state: - return - - time.sleep(interval) - count += interval - if count >= timeout: - raise Exception('timeout') + return state == expected_state + + wait_for( + _state_matches, + timeout=timeout, + timeout_message="timeout waiting for {0}'s peer {1} to be {2}".format( + self.router_id, peer.router_id, expected_state, + ), + ) def add_static_route(self, network, next_hop): cmd = '/sbin/ip route add {0} via {1}'.format(network, next_hop) diff --git a/test/scenario_test/bgp_router_test.py b/test/scenario_test/bgp_router_test.py index d49bfaee5..7ae16f250 100644 --- a/test/scenario_test/bgp_router_test.py +++ b/test/scenario_test/bgp_router_test.py @@ -83,29 +83,15 @@ def test_01_neighbor_established(self): def test_02_check_gobgp_global_rib(self): for q in self.quaggas.values(): - # paths expected to exist in gobgp's global rib - routes = list(q.routes.keys()) - timeout = 120 - interval = 1 - count = 0 - - while True: - # gobgp's global rib + expected_routes = set(q.routes.keys()) + + def _has_expected_routes(): state = self.gobgp.get_neighbor_state(q) self.assertEqual(state, BGP_FSM_ESTABLISHED) - global_rib = [p['prefix'] for p in self.gobgp.get_global_rib()] - - for p in global_rib: - if p in routes: - routes.remove(p) - - if len(routes) == 0: - break + global_rib = {p['prefix'] for p in self.gobgp.get_global_rib()} + return expected_routes.issubset(global_rib) - time.sleep(interval) - count += interval - if count >= timeout: - raise Exception('timeout') + wait_for_completion(_has_expected_routes) # check gobgp properly add it's own asn to aspath def test_03_check_gobgp_adj_out_rib(self): @@ -116,29 +102,21 @@ def test_03_check_gobgp_adj_out_rib(self): # check routes are properly advertised to all BGP speaker def test_04_check_quagga_global_rib(self): - interval = 1 - timeout = int(120 / interval) + expected_routes = {r for c in self.quaggas.values() for r in c.routes} for q in self.quaggas.values(): - done = False - for _ in range(timeout): - if done: - break + def _has_expected_routes(): global_rib = q.get_global_rib() global_rib = [p['prefix'] for p in global_rib] if len(global_rib) < len(self.quaggas): - time.sleep(interval) - continue + return False self.assertEqual(len(global_rib), len(self.quaggas)) - for c in self.quaggas.values(): - for r in c.routes: - self.assertTrue(r in global_rib) - done = True - if done: - continue - # should not reach here - raise AssertionError + for route in expected_routes: + self.assertTrue(route in global_rib) + return True + + wait_for_completion(_has_expected_routes) def test_05_add_quagga(self): q4 = QuaggaBGPContainer(name='q4', asn=65004, router_id='192.168.0.5') @@ -202,23 +180,18 @@ def test_09_add_distant_relative(self): q2.wait_for(expected_state=BGP_FSM_ESTABLISHED, peer=q5) q3.wait_for(expected_state=BGP_FSM_ESTABLISHED, peer=q5) - timeout = 120 - interval = 1 - count = 0 - while True: + def _has_expected_nexthop(): paths = self.gobgp.get_adj_rib_out(q1, '10.0.6.0/24') - if len(paths) > 0: - path = paths[0] - print("{0}'s nexthop is {1}".format(path['nlri']['prefix'], - path['nexthop'])) - n_addrs = [i[1].split('/')[0] for i in self.gobgp.ip_addrs] - if path['nexthop'] in n_addrs: - break + if not paths: + return False - time.sleep(interval) - count += interval - if count >= timeout: - raise Exception('timeout') + path = paths[0] + print("{0}'s nexthop is {1}".format(path['nlri']['prefix'], + path['nexthop'])) + n_addrs = [i[1].split('/')[0] for i in self.gobgp.ip_addrs] + return path['nexthop'] in n_addrs + + wait_for_completion(_has_expected_nexthop) def test_10_originate_path(self): self.gobgp.add_route('10.10.0.0/24') @@ -574,4 +547,3 @@ def test_27_dynamic_peer_md5(self): # config doesn't explicitly "know about" the peer q9.wait_for(expected_state=BGP_FSM_ESTABLISHED, peer=g6, timeout=30) - diff --git a/test/scenario_test/bgp_zebra_test.py b/test/scenario_test/bgp_zebra_test.py index 0224f9ce3..ccee9354c 100644 --- a/test/scenario_test/bgp_zebra_test.py +++ b/test/scenario_test/bgp_zebra_test.py @@ -29,6 +29,7 @@ Bridge, BGP_FSM_ESTABLISHED, local, + wait_for, ) from lib.gobgp import GoBGPContainer from lib.quagga import QuaggaBGPContainer @@ -261,25 +262,32 @@ def nexthops(): return n def validate_nexthops(peers): - interval = 1 - count = 0 - timeout = 30 - while True: - valid = False - nhs = nexthops() - if len(nhs) == len(peers): - valid = True - for peer in peers: - if g1.peers[peer]['neigh_addr'].split('/')[0] not in nhs: - valid = False - break - if valid: - return - - time.sleep(interval) - count += interval - if count >= timeout: - raise Exception(nhs) + current_nexthops = [] + expected_nexthops = [ + g1.peers[peer]['neigh_addr'].split('/')[0] for peer in peers + ] + + def _valid_nexthops(): + nonlocal current_nexthops + current_nexthops = nexthops() + if len(current_nexthops) != len(expected_nexthops): + return False + for nexthop in expected_nexthops: + if nexthop not in current_nexthops: + return False + return True + + def _timeout_message(): + return 'timeout waiting for zebra nexthops: expected {0}, got {1}'.format( + expected_nexthops, + current_nexthops, + ) + + wait_for( + _valid_nexthops, + timeout=30, + timeout_message=_timeout_message, + ) validate_nexthops([g4, g5]) @@ -301,5 +309,3 @@ def validate_nexthops(peers): g2.local('gobgp g ri add 10.0.10.0/24 med 20') validate_nexthops([g3]) - - diff --git a/test/scenario_test/evpn_test.py b/test/scenario_test/evpn_test.py index 17e78c8e0..c6e662197 100644 --- a/test/scenario_test/evpn_test.py +++ b/test/scenario_test/evpn_test.py @@ -30,6 +30,7 @@ BGP_FSM_ESTABLISHED, BGP_ATTR_TYPE_EXTENDED_COMMUNITIES, local, + wait_for_completion, ) from lib.gobgp import GoBGPContainer @@ -92,17 +93,11 @@ def test_02_add_evpn_route(self): path = dst['paths'][0] self.assertEqual(path['nexthop'], '0.0.0.0') - interval = 1 - timeout = int(30 / interval) - done = False - for _ in range(timeout): - if done: - break + def _has_expected_route(): grib = self.g2.get_global_rib(rf='evpn') if len(grib) < 1: - time.sleep(interval) - continue + return False self.assertEqual(len(grib), 1) dst = grib[0] @@ -110,7 +105,9 @@ def test_02_add_evpn_route(self): path = dst['paths'][0] n_addrs = [i[1].split('/')[0] for i in self.g1.ip_addrs] self.assertTrue(path['nexthop'] in n_addrs) - done = True + return True + + wait_for_completion(_has_expected_route, timeout=30) def test_03_check_mac_mobility(self): self.g2.local('gobgp global rib add ' @@ -174,4 +171,3 @@ def test_05_check_mac_mobility_per_mac_vrf(self): # no mac mobility for this route self.assertEqual(get_mac_mobility_sequence(path['attrs']), -1) - diff --git a/test/scenario_test/graceful_restart_test.py b/test/scenario_test/graceful_restart_test.py index 2fb78c544..59759f260 100644 --- a/test/scenario_test/graceful_restart_test.py +++ b/test/scenario_test/graceful_restart_test.py @@ -31,6 +31,7 @@ BGP_FSM_ESTABLISHED, GRACEFUL_RESTART_TIME, local, + wait_for_completion, ) from lib.gobgp import GoBGPContainer @@ -239,15 +240,12 @@ def test_11_multineighbor_graceful_restart(self): g1.start_gobgp(graceful_restart=True) - count = 0 - while (g1.get_neighbor_state(g2) != BGP_FSM_ESTABLISHED - or g1.get_neighbor_state(g3) != BGP_FSM_ESTABLISHED): - count += 1 + def _neighbors_reestablished(): # assert connections are not refused self.assertTrue(g1.get_neighbor_state(g2) != BGP_FSM_IDLE) self.assertTrue(g1.get_neighbor_state(g3) != BGP_FSM_IDLE) - if count > 120: - raise Exception('timeout') - time.sleep(1) + return (g1.get_neighbor_state(g2) == BGP_FSM_ESTABLISHED + and g1.get_neighbor_state(g3) == BGP_FSM_ESTABLISHED) + wait_for_completion(_neighbors_reestablished) diff --git a/test/scenario_test/ibgp_router_test.py b/test/scenario_test/ibgp_router_test.py index 97a4d9acc..89d378d05 100644 --- a/test/scenario_test/ibgp_router_test.py +++ b/test/scenario_test/ibgp_router_test.py @@ -75,28 +75,15 @@ def test_01_neighbor_established(self): def test_02_check_gobgp_global_rib(self): for q in self.quaggas.values(): - # paths expected to exist in gobgp's global rib - routes = list(q.routes.keys()) - timeout = 120 - interval = 1 - count = 0 - while True: - # gobgp's global rib + expected_routes = set(q.routes.keys()) + + def _has_expected_routes(): state = self.gobgp.get_neighbor_state(q) self.assertEqual(state, BGP_FSM_ESTABLISHED) - global_rib = [p['prefix'] for p in self.gobgp.get_global_rib()] - - for p in global_rib: - if p in routes: - routes.remove(p) - - if len(routes) == 0: - break + global_rib = {p['prefix'] for p in self.gobgp.get_global_rib()} + return expected_routes.issubset(global_rib) - time.sleep(interval) - count += interval - if count >= timeout: - raise Exception('timeout') + wait_for_completion(_has_expected_routes) def test_03_check_gobgp_adj_rib_out(self): for q in self.quaggas.values(): @@ -126,19 +113,13 @@ def test_05_check_gobgp_adj_rib_out(self): # check routes are properly advertised to all BGP speaker def test_06_check_quagga_global_rib(self): - interval = 1 - timeout = int(120 / interval) for q in self.quaggas.values(): - done = False - for _ in range(timeout): - if done: - break + def _has_expected_routes(): global_rib = q.get_global_rib() # quagga's global_rib must have two routes at least, # a self-generated route and a gobgp-generated route if len(global_rib) < len(q.routes) + len(self.gobgp.routes): - time.sleep(interval) - continue + return False peer_info = self.gobgp.peers[q] local_addr = peer_info['local_addr'].split('/')[0] @@ -154,11 +135,9 @@ def test_06_check_quagga_global_rib(self): if rr['prefix'] == r: self.assertEqual(rr['nexthop'], '0.0.0.0') - done = True - if done: - continue - # should not reach here - raise AssertionError + return True + + wait_for_completion(_has_expected_routes) def test_07_add_ebgp_peer(self): q3 = QuaggaBGPContainer(name='q3', asn=65001, router_id='192.168.0.4') @@ -290,4 +269,3 @@ def f3(): return len(l) == 1 and len(l[0]['aspath']) == 0 wait_for_completion(f3) - diff --git a/test/scenario_test/mup_test.py b/test/scenario_test/mup_test.py index 49e02e6ab..8564bc16f 100644 --- a/test/scenario_test/mup_test.py +++ b/test/scenario_test/mup_test.py @@ -32,6 +32,7 @@ BGP_FSM_ESTABLISHED, BGP_ATTR_TYPE_EXTENDED_COMMUNITIES, local, + wait_for_completion, ) from lib.gobgp import GoBGPContainer @@ -105,38 +106,27 @@ def test_02_add_del_mup_route(self): path = dst['paths'][0] self.assertEqual(path['nexthop'], nh) - interval = 1 - timeout = int(30 / interval) - done = False - for _ in range(timeout): - if done: - break + def _has_expected_route(): grib = self.g2.get_global_rib(rf=rf) if len(grib) < 1: - time.sleep(interval) - continue + return False self.assertEqual(len(grib), 1) dst = grib[0] self.assertEqual(len(dst['paths']), 1) path = dst['paths'][0] - n_addrs = [i[1].split('/')[0] for i in self.g1.ip_addrs] self.assertEqual(path['nexthop'], nh) - done = True + return True + + wait_for_completion(_has_expected_route, timeout=30) self.g1.local('gobgp global rib del ' '-a {} {}'.format(rf, route)) - done = False - for _ in range(timeout): - if done: - break + def _route_removed(): grib = self.g2.get_global_rib(rf=rf) + return len(grib) == 0 - if len(grib) > 0: - time.sleep(interval) - continue - done = True - + wait_for_completion(_route_removed, timeout=30) diff --git a/test/scenario_test/route_reflector_test.py b/test/scenario_test/route_reflector_test.py index f2a8812c3..b0bee7e81 100644 --- a/test/scenario_test/route_reflector_test.py +++ b/test/scenario_test/route_reflector_test.py @@ -25,24 +25,11 @@ from lib.noseplugin import parser_option from lib import base -from lib.base import BGP_FSM_ESTABLISHED, local +from lib.base import BGP_FSM_ESTABLISHED, local, wait_for from lib.gobgp import GoBGPContainer from lib.quagga import QuaggaBGPContainer -def wait_for(f, timeout=120): - interval = 1 - count = 0 - while True: - if f(): - return - - time.sleep(interval) - count += interval - if count >= timeout: - raise Exception('timeout') - - class GoBGPTestBase(unittest.TestCase): def assert_adv_count(self, src, dst, rf, count): self.assertEqual(count, len(src.get_adj_rib_out(dst, rf=rf))) @@ -285,4 +272,3 @@ def test_12_routes_from_separate_rts_peers_are_isolated_by_rr(self): self.assertEqual(self.rr.peer_name(self.acme1), ar0['nexthop']) self.assertEqual(100, ar0['local-pref']) - diff --git a/test/scenario_test/route_server_as2_test.py b/test/scenario_test/route_server_as2_test.py index fc9f3b4e7..35758ddde 100644 --- a/test/scenario_test/route_server_as2_test.py +++ b/test/scenario_test/route_server_as2_test.py @@ -29,6 +29,7 @@ BGP_FSM_IDLE, BGP_FSM_ESTABLISHED, local, + wait_for, ) from lib.gobgp import GoBGPContainer from lib.exabgp import ExaBGPContainer @@ -82,29 +83,24 @@ def test_01_neighbor_established(self): def test_02_check_gobgp_local_rib(self): for rs_client in self.quaggas.values(): - done = False - for _ in range(self.retry_limit): - if done: - break - + def _has_expected_routes(): state = self.gobgp.get_neighbor_state(rs_client) self.assertEqual(state, BGP_FSM_ESTABLISHED) local_rib = self.gobgp.get_local_rib(rs_client) if len(local_rib) < (len(self.quaggas) - 1): - time.sleep(self.wait_per_retry) - continue + return False self.assertEqual(len(local_rib), 4) - done = True + return True - if done: - continue - # should not reach here - raise AssertionError + wait_for( + _has_expected_routes, + timeout=self.retry_limit * self.wait_per_retry, + interval=self.wait_per_retry, + ) def test_03_stop_q2_and_check_neighbor_status(self): q2 = self.quaggas['q2'] q2.remove() self.gobgp.wait_for(expected_state=BGP_FSM_IDLE, peer=q2) - diff --git a/test/scenario_test/route_server_ipv4_v6_test.py b/test/scenario_test/route_server_ipv4_v6_test.py index 0f45fda11..8792401d6 100644 --- a/test/scenario_test/route_server_ipv4_v6_test.py +++ b/test/scenario_test/route_server_ipv4_v6_test.py @@ -25,7 +25,7 @@ from lib.noseplugin import parser_option from lib import base -from lib.base import BGP_FSM_ESTABLISHED, local +from lib.base import BGP_FSM_ESTABLISHED, local, wait_for from lib.gobgp import GoBGPContainer from lib.quagga import QuaggaBGPContainer @@ -78,18 +78,13 @@ def setUpClass(cls): def check_gobgp_local_rib(self, ctns, rf): for rs_client in ctns.values(): - done = False - for _ in range(self.retry_limit): - if done: - break - + def _has_expected_routes(): state = self.gobgp.get_neighbor_state(rs_client) self.assertEqual(state, BGP_FSM_ESTABLISHED) local_rib = self.gobgp.get_local_rib(rs_client, rf=rf) local_rib = [p["prefix"] for p in local_rib] if len(local_rib) < (len(ctns) - 1): - time.sleep(self.wait_per_retry) - continue + return False self.assertEqual(len(local_rib), (len(ctns) - 1)) @@ -98,23 +93,21 @@ def check_gobgp_local_rib(self, ctns, rf): for r in c.routes: self.assertTrue(r in local_rib) - done = True - if done: - continue - # should not reach here - raise AssertionError + return True + + wait_for( + _has_expected_routes, + timeout=self.retry_limit * self.wait_per_retry, + interval=self.wait_per_retry, + ) def check_rs_client_rib(self, ctns, rf): for rs_client in ctns.values(): - done = False - for _ in range(self.retry_limit): - if done: - break + def _has_expected_routes(): global_rib = rs_client.get_global_rib(rf=rf) global_rib = [p['prefix'] for p in global_rib] if len(global_rib) < len(ctns): - time.sleep(self.wait_per_retry) - continue + return False self.assertEqual(len(global_rib), len(ctns)) @@ -122,11 +115,13 @@ def check_rs_client_rib(self, ctns, rf): for r in c.routes: self.assertTrue(r in global_rib) - done = True - if done: - continue - # should not reach here - raise AssertionError + return True + + wait_for( + _has_expected_routes, + timeout=self.retry_limit * self.wait_per_retry, + interval=self.wait_per_retry, + ) # test each neighbor state is turned establish def test_01_neighbor_established(self): @@ -166,4 +161,3 @@ def test_08_check_rib(self): self.assertEqual(len(self.gobgp.get_adj_rib_out(q, rf='ipv6')), 0) self.assertEqual(len(q.get_global_rib(rf='ipv6')), len(q.routes)) - diff --git a/test/scenario_test/route_server_malformed_test.py b/test/scenario_test/route_server_malformed_test.py index 26a67e961..7e5e1b049 100644 --- a/test/scenario_test/route_server_malformed_test.py +++ b/test/scenario_test/route_server_malformed_test.py @@ -26,7 +26,7 @@ from lib.noseplugin import parser_option from lib import base -from lib.base import BGP_FSM_ESTABLISHED, local +from lib.base import BGP_FSM_ESTABLISHED, local, wait_for from lib.gobgp import GoBGPContainer from lib.exabgp import ExaBGPContainer @@ -48,19 +48,6 @@ def lookup_scenario(name): return None -def wait_for(f, timeout=120): - interval = 1 - count = 0 - while True: - if f(): - return - - time.sleep(interval) - count += interval - if count >= timeout: - raise Exception('timeout') - - @register_scenario class MalformedMpReachNlri(object): """ @@ -497,4 +484,3 @@ def test(self): e(self) print('[PASS] %s' % e.__qualname__.split('.')[0], file=sys.stderr, flush=True) - diff --git a/test/scenario_test/route_server_policy_grpc_test.py b/test/scenario_test/route_server_policy_grpc_test.py index 2f453f682..2eb81d305 100644 --- a/test/scenario_test/route_server_policy_grpc_test.py +++ b/test/scenario_test/route_server_policy_grpc_test.py @@ -31,6 +31,7 @@ BGP_ATTR_TYPE_COMMUNITIES, BGP_ATTR_TYPE_EXTENDED_COMMUNITIES, local, + wait_for, ) from lib.gobgp import GoBGPContainer from lib.quagga import QuaggaBGPContainer @@ -54,19 +55,6 @@ def lookup_scenario(name): return None -def wait_for(f, timeout=120): - interval = 1 - count = 0 - while True: - if f(): - return - - time.sleep(interval) - count += interval - if count >= timeout: - raise Exception('timeout') - - @register_scenario class ImportPolicy(object): """ @@ -2709,4 +2697,3 @@ def test(self): e(self) print('[PASS] %s' % e.__qualname__.split('.')[0], file=sys.stderr, flush=True) - diff --git a/test/scenario_test/route_server_policy_test.py b/test/scenario_test/route_server_policy_test.py index 49c8ac464..ead947c1e 100644 --- a/test/scenario_test/route_server_policy_test.py +++ b/test/scenario_test/route_server_policy_test.py @@ -31,6 +31,7 @@ BGP_ATTR_TYPE_COMMUNITIES, BGP_ATTR_TYPE_EXTENDED_COMMUNITIES, local, + wait_for, ) from lib.gobgp import GoBGPContainer from lib.quagga import QuaggaBGPContainer @@ -54,19 +55,6 @@ def lookup_scenario(name): return None -def wait_for(f, timeout=120): - interval = 1 - count = 0 - while True: - if f(): - return - - time.sleep(interval) - count += interval - if count >= timeout: - raise Exception('timeout') - - @register_scenario class ImportPolicy(object): """ @@ -3184,4 +3172,3 @@ def test(self): e(self) print('[PASS] %s' % e.__qualname__.split('.')[0], file=sys.stderr, flush=True) - diff --git a/test/scenario_test/route_server_test.py b/test/scenario_test/route_server_test.py index b9d1ccf4c..3e20ee9c8 100644 --- a/test/scenario_test/route_server_test.py +++ b/test/scenario_test/route_server_test.py @@ -29,6 +29,7 @@ BGP_FSM_ACTIVE, BGP_FSM_ESTABLISHED, local, + wait_for, ) from lib.gobgp import GoBGPContainer from lib.quagga import QuaggaBGPContainer @@ -76,18 +77,14 @@ def setUpClass(cls): def check_gobgp_local_rib(self): for rs_client in self.quaggas.values(): - done = False - for _ in range(self.retry_limit): - if done: - break + def _has_expected_routes(): local_rib = self.gobgp.get_local_rib(rs_client) local_rib = [p['prefix'] for p in local_rib] state = self.gobgp.get_neighbor_state(rs_client) self.assertEqual(state, BGP_FSM_ESTABLISHED) if len(local_rib) < (len(self.quaggas) - 1): - time.sleep(self.wait_per_retry) - continue + return False self.assertEqual(len(local_rib), (len(self.quaggas) - 1)) @@ -96,23 +93,21 @@ def check_gobgp_local_rib(self): for r in c.routes: self.assertTrue(r in local_rib) - done = True - if done: - continue - # should not reach here - raise AssertionError + return True + + wait_for( + _has_expected_routes, + timeout=self.retry_limit * self.wait_per_retry, + interval=self.wait_per_retry, + ) def check_rs_client_rib(self): for rs_client in self.quaggas.values(): - done = False - for _ in range(self.retry_limit): - if done: - break + def _has_expected_routes(): global_rib = rs_client.get_global_rib() global_rib = [p['prefix'] for p in global_rib] if len(global_rib) < len(self.quaggas): - time.sleep(self.wait_per_retry) - continue + return False self.assertEqual(len(global_rib), len(self.quaggas)) @@ -120,11 +115,13 @@ def check_rs_client_rib(self): for r in c.routes: self.assertTrue(r in global_rib) - done = True - if done: - continue - # should not reach here - raise AssertionError + return True + + wait_for( + _has_expected_routes, + timeout=self.retry_limit * self.wait_per_retry, + interval=self.wait_per_retry, + ) # test each neighbor state is turned establish def test_01_neighbor_established(self): @@ -217,11 +214,7 @@ def test_10_add_distant_relative(self): self.gobgp.wait_for(expected_state=BGP_FSM_ESTABLISHED, peer=q3) def check_nexthop(target_prefix, expected_nexthop): - is_done = False - for _ in range(self.retry_limit): - if is_done: - break - time.sleep(self.wait_per_retry) + def _has_expected_nexthop(): for path in q1.get_global_rib(): if path['prefix'] == target_prefix: print("{0}'s nexthop is {1}".format(path['prefix'], @@ -229,9 +222,15 @@ def check_nexthop(target_prefix, expected_nexthop): n_addrs = [i[1].split('/')[0] for i in expected_nexthop.ip_addrs] if path['nexthop'] in n_addrs: - is_done = True - break - return is_done + return True + return False + + wait_for( + _has_expected_nexthop, + timeout=self.retry_limit * self.wait_per_retry, + interval=self.wait_per_retry, + ) + return True done = check_nexthop('10.0.6.0/24', q3) self.assertTrue(done) @@ -247,5 +246,3 @@ def check_nexthop(target_prefix, expected_nexthop): done = check_nexthop('10.0.6.0/24', q2) self.assertTrue(done) - - From 2fadaaa7fb6dbec1a229739fca0a609c796efdcb Mon Sep 17 00:00:00 2001 From: Ivan Pokhabov Date: Wed, 5 Aug 2026 16:02:50 +0300 Subject: [PATCH 2/2] test(scenario): add scalable local command timeouts and failure logs Add a --timeout-scale pytest option, backed by GOBGP_TEST_TIMEOUT_SCALE, to multiply scenario-test waits and retry counts for slower environments. Apply the scale to shared wait helpers, repeated assertion helpers, Docker client timeouts, and local command execution. Switch local command execution to subprocess.run so stderr is preserved in test logs and timeout failures include the Docker CLI error output. --- test/lib/base.py | 62 +++++++++++++++++++++++++++++----- test/lib/noseplugin.py | 1 + test/scenario_test/conftest.py | 20 +++++++++++ 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/test/lib/base.py b/test/lib/base.py index cbb7f9c98..eefdd3781 100644 --- a/test/lib/base.py +++ b/test/lib/base.py @@ -18,6 +18,7 @@ import subprocess import time import itertools +import math import textwrap from colored import fg, attr @@ -25,6 +26,8 @@ from docker import APIClient as Client import netaddr +from lib.noseplugin import parser_option + DEFAULT_TEST_PREFIX = '' DEFAULT_TEST_BASE_DIR = '/tmp/gobgp' @@ -84,10 +87,47 @@ TEST_NETWORK_LABEL = TEST_CONTAINER_LABEL -def local(s, capture=False): +def _timeout_scale(): + return max(float(getattr(parser_option, 'timeout_scale', 1.0)), 1.0) + + +def scale_timeout(timeout): + if timeout is None: + return None + return int(math.ceil(float(timeout) * _timeout_scale())) + + +def scale_count(count): + return int(math.ceil(float(count) * _timeout_scale())) + + +def local(s, capture=False, timeout=None): print('[localhost] local:', s) _env = {'NOSE_NOLOGCAPTURE': '1' if capture else '0'} - return subprocess.check_output(s, shell=True, env=_env).decode('utf-8').strip() + # Keep stderr visible in pytest logs. The Docker CLI often puts the useful + # failure reason there, while check_output only preserved stdout. + result = subprocess.run( + s, + shell=True, + env=_env, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + timeout=scale_timeout(timeout), + ) + stdout = result.stdout.decode('utf-8') + stderr = result.stderr.decode('utf-8') + if stderr: + print(stderr, end='') + if result.returncode != 0: + if stdout: + print(stdout, end='') + raise subprocess.CalledProcessError( + result.returncode, + s, + output=result.stdout, + stderr=result.stderr, + ) + return stdout.strip() def yellow(s): @@ -114,6 +154,7 @@ def community_str(i): def wait_for(f, timeout=DEFAULT_WAIT_TIMEOUT, interval=1, timeout_message='timeout'): + timeout = scale_timeout(timeout) count = 0 while True: if f(): @@ -133,7 +174,7 @@ def wait_for_completion(f, timeout=DEFAULT_WAIT_TIMEOUT): def try_several_times(f, t=3, s=1): - for _ in range(t): + for _ in range(scale_count(t)): try: r = f() except RuntimeError: @@ -145,7 +186,7 @@ def try_several_times(f, t=3, s=1): def assert_several_times(f, t=DEFAULT_ASSERT_RETRIES, s=1): e = AssertionError - for _ in range(t): + for _ in range(scale_count(t)): try: f() except AssertionError as ae: @@ -231,7 +272,7 @@ def addif(self, ctn, ip_addr=''): if self.subnet.version == 6: ip = '--ip6 {0}'.format(ip_addr) local("docker network connect {0} {1} {2}".format(ip, self.name, ctn.docker_name())) - i = [x for x in list(Client(timeout=60, version='auto').inspect_network(self.id)['Containers'].values()) if x['Name'] == ctn.docker_name()][0] + i = [x for x in list(Client(timeout=scale_timeout(60), version='auto').inspect_network(self.id)['Containers'].values()) if x['Name'] == ctn.docker_name()][0] if self.subnet.version == 4: eth = 'eth{0}'.format(len(ctn.ip_addrs)) addr = i['IPv4Address'] @@ -297,14 +338,19 @@ def remove(self): self.is_running = False return ret - def local(self, cmd, capture=False, stream=False, detach=False, tty=True): + def local(self, cmd, capture=False, stream=False, detach=False, tty=True, + timeout=None): if stream: - dckr = Client(timeout=120, version='auto') + dckr = Client(timeout=scale_timeout(120), version='auto') i = dckr.exec_create(container=self.docker_name(), cmd=cmd) return dckr.exec_start(i['Id'], tty=tty, stream=stream, detach=detach) else: flag = '-d' if detach else '' - return local('docker exec {0} {1} {2}'.format(flag, self.docker_name(), cmd), capture) + return local( + 'docker exec {0} {1} {2}'.format(flag, self.docker_name(), cmd), + capture, + timeout=timeout, + ) def get_pid(self): if self.is_running: diff --git a/test/lib/noseplugin.py b/test/lib/noseplugin.py index 3d9d9598a..03d1bd531 100644 --- a/test/lib/noseplugin.py +++ b/test/lib/noseplugin.py @@ -6,6 +6,7 @@ class _ParserOption: gobgp_log_level = "info" test_index = 0 config_format = "yaml" + timeout_scale = 1.0 parser_option = _ParserOption() diff --git a/test/scenario_test/conftest.py b/test/scenario_test/conftest.py index 0cd9245b4..935be897a 100644 --- a/test/scenario_test/conftest.py +++ b/test/scenario_test/conftest.py @@ -1,7 +1,21 @@ +import math +import os + import pytest from lib.noseplugin import parser_option +def _parse_timeout_scale(value): + try: + scale = float(value) + except (TypeError, ValueError): + raise pytest.UsageError('--timeout-scale must be a number') + + if not math.isfinite(scale) or scale < 1.0: + raise pytest.UsageError('--timeout-scale must be a finite number greater than or equal to 1.0') + return scale + + def pytest_addoption(parser): parser.addoption('--test-prefix', default='') parser.addoption('--gobgp-image', default='osrg/gobgp') @@ -10,6 +24,11 @@ def pytest_addoption(parser): parser.addoption('--gobgp-log-level', default='info') parser.addoption('--test-index', type=int, default=0) parser.addoption('--config-format', default='yaml') + parser.addoption( + '--timeout-scale', + default=os.environ.get('GOBGP_TEST_TIMEOUT_SCALE', '1.0'), + help='multiply default scenario-test wait timeouts by this factor', + ) def pytest_configure(config): @@ -20,3 +39,4 @@ def pytest_configure(config): parser_option.gobgp_log_level = config.getoption('--gobgp-log-level') parser_option.test_index = config.getoption('--test-index') parser_option.config_format = config.getoption('--config-format') + parser_option.timeout_scale = _parse_timeout_scale(config.getoption('--timeout-scale'))