tests: Convert lots of places to use run_and_expect

[donaldsharp]

There are a large number of places where the topotests are changing state via some sort of command and then immediately checking to see if that state is reflected, without giving time for that command to be processed. Modify these places that have been identified and switch the resulting show commands( of what ever ilk ) to use run_and_expect.

[greptile-apps]

Greptile Summary

This large PR converts ~30 topotest files from immediate state-checking (or fixed sleep() calls) to polling-based verification using topotest.run_and_expect(). This is a systematic improvement to test reliability — after issuing a configuration command, the tests now poll for the expected state change rather than assuming it happened instantaneously.

• The conversions are mechanically consistent across BGP, OSPF, LDP, RIP, RIPng, PIM, MSDP, BFD, SNMP, and Zebra test suites
• Fixed sleep() calls (e.g., sleep(7), sleep(17), sleep(21)) are replaced with active polling, which is both faster (early exit on success) and more reliable (longer effective timeout)
• Manual while loops with retry logic are standardized to run_and_expect
• New reusable helper functions are introduced where appropriate (e.g., check_label_table_state in bgp_l3vpn_label_export)
• Bug in test_ospf_p2mp_non_broadcast.py: The check_p2mp_non_broadcast_config helper returns shell exit codes (0/1) but run_and_expect compares against True, causing a type mismatch — the "config present" check will always time out

Confidence Score: 4/5

• This PR is safe to merge after fixing one bug in the OSPF P2MP non-broadcast test file
• 29 of 30 files apply a consistent, correct mechanical transformation from direct assertions to run_and_expect polling. The one issue in test_ospf_p2mp_non_broadcast.py involves a shell exit code vs. boolean comparison mismatch that will cause a test to always time out before passing. Since this only affects test infrastructure (no production code), the overall risk is low.
• tests/topotests/ospf_p2mp/test_ospf_p2mp_non_broadcast.py has a logic bug where run_and_expect compares a shell exit code (int 0) against True, which will never match

Important Files Changed

Filename	Overview
tests/topotests/bgp_l3vpn_label_export/test_bgp_l3vpn_label_export.py	Extracts a reusable check_label_table_state helper and converts all label-table assertions to run_and_expect. Uses re.search instead of re.match appropriately for unfiltered output. Correct.
tests/topotests/bgp_snmp_mplsl3vpn/test_bgp_snmp_mplsvpn.py	Replaces manual polling loops and direct SNMP assertions with run_and_expect. Inner helper functions return booleans for SNMP checks. Correct and cleaner.
tests/topotests/ldp_topo1/test_ldp_topo1.py	Major refactor: replaces manual timeout loop for LDP neighbor convergence with run_and_expect. Removes sleep(5) and sleep(15) calls. Correct.
tests/topotests/ospf_p2mp/test_ospf_p2mp_non_broadcast.py	Bug: first run_and_expect call expects True but grep -q returns 0 when config IS present (0 != True in Python), causing a guaranteed 30-second timeout. The second call (removal check) works by coincidence (grep returns 1, and 1 == True in Python).
tests/topotests/ospf_topo1/test_ospf_topo1.py	Extracts kernel route comparison helpers and replaces manual while-loop polling with run_and_expect. Correct.
tests/topotests/rip_topo1/test_rip_topo1.py	Replaces sleep(21) with active convergence polling. Converts RIP status and route checks from immediate assert to run_and_expect. Correct.
tests/topotests/ripng_topo1/test_ripng_topo1.py	Same pattern as rip_topo1: replaces sleep(11) with convergence polling and wraps RIPng status/route checks in run_and_expect. Correct.
tests/topotests/sbfd_topo1/test_sbfd_topo1.py	Wraps sbfd deletion check in run_and_expect(func, False). Mostly formatting/style changes (quotes, whitespace). Correct.
tests/topotests/simple_snmp_test/test_simple_snmp.py	Replaces sleep(17) with active SNMP readiness polling via bgpVersion OID check. Correct and much more robust.
tests/topotests/zebra_nhg_check/test_zebra_nhg.py	Replaces manual while-loop polling for BGP route count with run_and_expect. Mostly whitespace additions (blank lines before nested functions). Correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Test issues config command] --> B{Old pattern}
    A --> C{New pattern}
    
    B --> D["Direct check: result = router_json_cmp(...)"]
    D --> E["assert result is None"]
    E --> F["❌ Fails if state not yet propagated"]
    
    B --> G["sleep(N) then check"]
    G --> H["❌ Wastes time or still too short"]
    
    C --> I["Wrap check in partial/lambda"]
    I --> J["run_and_expect(func, expected, count, wait)"]
    J --> K{result == expected?}
    K -->|Yes| L["✅ Return (True, result) — early exit"]
    K -->|No| M["sleep(wait), retry"]
    M --> N{count > 0?}
    N -->|Yes| J
    N -->|No| O["Return (False, result) — timeout"]

Loading

_{Last reviewed commit: 7c03ee6}

[greptile-apps]

_{30 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Wrong expected value for cmd_status return code

check_p2mp_non_broadcast_config() returns the shell exit code from grep -q: 0 when the config IS present, 1 when it is NOT. run_and_expect uses result != what to compare, and in Python 0 != True is True (they are not equal), so this first call — which verifies the config is present — will never match and will always time out after 30 seconds.

The second usage at line 270 (verifying removal) works only by coincidence: grep -q returns 1 when config is absent, and 1 == True is True in Python due to bool being a subclass of int.

The expected value should be 0 for "config present" checks and 1 for "config absent" checks:

Suggested change

	_, rc = topotest.run_and_expect(
	check_p2mp_non_broadcast_config, True, count=30, wait=1
	)
	_, rc = topotest.run_and_expect(
	check_p2mp_non_broadcast_config, 0, count=30, wait=1
	)

And similarly at line 270, change the expected value to 1:

    _, rc = topotest.run_and_expect(
        check_p2mp_non_broadcast_config, 1, count=30, wait=1
    )

And at line 283:

    _, rc = topotest.run_and_expect(
        check_p2mp_non_broadcast_config, 0, count=30, wait=1
    )

Prompt To Fix With AI

This is a comment left during a code review.
Path: tests/topotests/ospf_p2mp/test_ospf_p2mp_non_broadcast.py
Line: 246-248

Comment:
**Wrong expected value for `cmd_status` return code**

`check_p2mp_non_broadcast_config()` returns the shell exit code from `grep -q`: **0** when the config IS present, **1** when it is NOT. `run_and_expect` uses `result != what` to compare, and in Python `0 != True` is `True` (they are not equal), so this first call — which verifies the config **is** present — will never match and will always time out after 30 seconds.

The second usage at line 270 (verifying removal) works only by coincidence: `grep -q` returns `1` when config is absent, and `1 == True` is `True` in Python due to `bool` being a subclass of `int`.

The expected value should be `0` for "config present" checks and `1` for "config absent" checks:

```suggestion
    _, rc = topotest.run_and_expect(
        check_p2mp_non_broadcast_config, 0, count=30, wait=1
    )
```

And similarly at line 270, change the expected value to `1`:
```python
    _, rc = topotest.run_and_expect(
        check_p2mp_non_broadcast_config, 1, count=30, wait=1
    )
```

And at line 283:
```python
    _, rc = topotest.run_and_expect(
        check_p2mp_non_broadcast_config, 0, count=30, wait=1
    )
```

How can I resolve this? If you propose a fix, please make it concise.

[donaldsharp]

fixed

[riw777]

looks good ... greptile's comments look valid, though (?)

[riw777]

looks good