tests: fix uptime check in test_bgp_default_originate_2links.py

[mwinter-osr]

In bgp_default_originate/test_bgp_default_originate_2links.py,
test_verify_bgp_default_originate_with_default_static_route_p1 uses
the uptime from "show ip route" (zebra) in two places to verify that
a config change to bgp default-originate on one link did not disturb
the bgp 0.0.0.0/0 route received on the other link. Both checks are
fundamentally broken: any change to a member of an ECMP set causes
zebra to reinstall the entire set, resetting the uptime regardless
of whether the monitored path itself changed.

• The first check verified that re-configuring default-originate on
  link-1 did not disturb the path received on link-2.

• The second check verified that removing redistribute static did not
  disturb the default-originate path received on link-1.

Replace both zebra-based uptime checks with two BGP-table checks that
together prove no spurious UPDATE was received on the monitored session:

1. Path attrs comparison: attrs {aspath, origin, metric, nexthop}
   for the peer's path in r2's BGP table (show bgp ipv4/ipv6
   unicast 0.0.0.0/0 json) before and after the config change. Any
   UPDATE carrying different attributes would be caught here.

2. Duplicate-count check: snapshot peer->pcount_dup[afi][safi]
   (exposed as "receivedPrefixDup" in show bgp neighbor PEER json)
   before and after. An UPDATE carrying identical attributes —
   which the attrs comparison would miss — increments this counter.

Original PR21480 by enkechen-panw

[greptile-apps]

Greptile Summary

Replaces two fundamentally broken zebra-uptime checks in test_verify_bgp_default_originate_with_default_static_route_p1 with a pair of BGP-table checks (path-attrs snapshot + receivedPrefixDup counter) that correctly prove no spurious UPDATE was sent on the monitored session after an ECMP-set modification. The fix is well-motivated and the two-check design (attrs catches attribute-change UPDATEs; dup-counter catches identical-attribute UPDATEs) is sound.

• P1: _get_dup_count returns None on any caught exception; all four dup-count assertions are bare == with no prior is not None guard. If both snapshots fail identically (None == None), the check passes vacuously and the duplicate-UPDATE detection is silently disabled.
• P2: except (json.JSONDecodeError, Exception) is redundant in both helpers — Exception already covers json.JSONDecodeError — and it swallows programming errors that should surface during test development.

Confidence Score: 4/5

• Good fix to a broken test, but the dup-count check has a vacuous-pass flaw that must be addressed before the verification is trustworthy.
• The core approach (path-attrs + dup-counter) is correct and strictly better than the broken uptime checks. Score is 4 rather than 5 because the P1 issue — _get_dup_count returning None causing None == None to pass — means the second line of defense (duplicate-UPDATE detection) can be silently disabled under error conditions, which undermines the stated purpose of this PR.
• tests/topotests/bgp_default_originate/test_bgp_default_originate_2links.py — _get_dup_count and all four dup-count assertion sites need is not None guards.

Important Files Changed

Filename	Overview
tests/topotests/bgp_default_originate/test_bgp_default_originate_2links.py	Replaces fundamentally broken zebra-uptime checks with two BGP-table checks (path-attrs comparison + receivedPrefixDup counter). The approach is correct, but _get_dup_count can return None on exception and all four dup-count assertions use == without a prior is-not-None guard, so both snapshots failing identically makes the check vacuously pass.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Snapshot BGP path attrs\nbefore/after check] --> B{_bgp_stable_attrs_from_peer}
    B -->|Returns dict| C[Compare before == after\nCatches attribute-change UPDATEs]
    B -->|Returns None| D[assert bgp_before is not None\nFails hard — path missing]

    E[Snapshot dup counter\nbefore/after check] --> F{_get_dup_count}
    F -->|Returns int 0+| G[assert dup_after == dup_before\nCatches duplicate UPDATEs]
    F -->|Returns None on Exception| H{Both before AND after None?}
    H -->|Yes — vacuous pass ⚠️| I[None == None → True\nDuplicate-UPDATE check silently disabled]
    H -->|No — one is int| J[None == int → False\nFails with misleading message]

    style I fill:#ff9999,stroke:#cc0000
    style D fill:#99ff99,stroke:#006600
    style C fill:#99ff99,stroke:#006600
    style G fill:#99ff99,stroke:#006600

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: tests/topotests/bgp_default_originate/test_bgp_default_originate_2links.py
Line: 381-386

Comment:
**`None == None` vacuously passes the dup-count assertions**

`_get_dup_count` returns `None` when the broad `except (json.JSONDecodeError, Exception)` fires — e.g. if vtysh returns a non-JSON error string for an unknown neighbor IP. Because all four dup-count assertions are `assert dup_after == dup_before`, if both snapshots fail the same way (`None == None` → `True`), the duplicate-UPDATE check silently passes and provides zero protection. Add `is not None` guards before each comparison:

```python
assert dup_before_ipv4 is not None, (
    "{}: could not read link-2 IPv4 dup counter before re-config".format(tc_name)
)
assert dup_after_ipv4 is not None, (
    "{}: could not read link-2 IPv4 dup counter after re-config".format(tc_name)
)
assert dup_after_ipv4 == dup_before_ipv4, (...)
```

The same guard is needed for all four dup-count assertions (ipv4/ipv6 × check-1/check-2).

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

---

This is a comment left during a code review.
Path: tests/topotests/bgp_default_originate/test_bgp_default_originate_2links.py
Line: 381-382

Comment:
**Redundant exception type in both helpers**

`json.JSONDecodeError` is a subclass of `ValueError`, which is itself a subclass of `Exception`, so listing it alongside `Exception` is entirely redundant. The broad `except Exception` also swallows programming errors (`AttributeError`, `NameError`, etc.) that should surface during test development. Tighten the catch in both `_get_dup_count` and `_bgp_stable_attrs_from_peer`:

```suggestion
    except (ValueError, TypeError):
```

This covers JSON parse failures and bad vtysh output types while letting real programming errors propagate.

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

_{Reviews (1): Last reviewed commit: "tests: fix uptime check in bgp_default_o..." | Re-trigger Greptile}

[greptile-apps]

None == None vacuously passes the dup-count assertions

_get_dup_count returns None when the broad except (json.JSONDecodeError, Exception) fires — e.g. if vtysh returns a non-JSON error string for an unknown neighbor IP. Because all four dup-count assertions are assert dup_after == dup_before, if both snapshots fail the same way (None == None → True), the duplicate-UPDATE check silently passes and provides zero protection. Add is not None guards before each comparison:

assert dup_before_ipv4 is not None, (
    "{}: could not read link-2 IPv4 dup counter before re-config".format(tc_name)
)
assert dup_after_ipv4 is not None, (
    "{}: could not read link-2 IPv4 dup counter after re-config".format(tc_name)
)
assert dup_after_ipv4 == dup_before_ipv4, (...)

The same guard is needed for all four dup-count assertions (ipv4/ipv6 × check-1/check-2).

Prompt To Fix With AI

This is a comment left during a code review.
Path: tests/topotests/bgp_default_originate/test_bgp_default_originate_2links.py
Line: 381-386

Comment:
**`None == None` vacuously passes the dup-count assertions**

`_get_dup_count` returns `None` when the broad `except (json.JSONDecodeError, Exception)` fires — e.g. if vtysh returns a non-JSON error string for an unknown neighbor IP. Because all four dup-count assertions are `assert dup_after == dup_before`, if both snapshots fail the same way (`None == None` → `True`), the duplicate-UPDATE check silently passes and provides zero protection. Add `is not None` guards before each comparison:

```python
assert dup_before_ipv4 is not None, (
    "{}: could not read link-2 IPv4 dup counter before re-config".format(tc_name)
)
assert dup_after_ipv4 is not None, (
    "{}: could not read link-2 IPv4 dup counter after re-config".format(tc_name)
)
assert dup_after_ipv4 == dup_before_ipv4, (...)
```

The same guard is needed for all four dup-count assertions (ipv4/ipv6 × check-1/check-2).

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

[greptile-apps]

Redundant exception type in both helpers

json.JSONDecodeError is a subclass of ValueError, which is itself a subclass of Exception, so listing it alongside Exception is entirely redundant. The broad except Exception also swallows programming errors (AttributeError, NameError, etc.) that should surface during test development. Tighten the catch in both _get_dup_count and _bgp_stable_attrs_from_peer:

Suggested change

	except (json.JSONDecodeError, Exception):
	return None
	except (ValueError, TypeError):

This covers JSON parse failures and bad vtysh output types while letting real programming errors propagate.

Prompt To Fix With AI

This is a comment left during a code review.
Path: tests/topotests/bgp_default_originate/test_bgp_default_originate_2links.py
Line: 381-382

Comment:
**Redundant exception type in both helpers**

`json.JSONDecodeError` is a subclass of `ValueError`, which is itself a subclass of `Exception`, so listing it alongside `Exception` is entirely redundant. The broad `except Exception` also swallows programming errors (`AttributeError`, `NameError`, etc.) that should surface during test development. Tighten the catch in both `_get_dup_count` and `_bgp_stable_attrs_from_peer`:

```suggestion
    except (ValueError, TypeError):
```

This covers JSON parse failures and bad vtysh output types while letting real programming errors propagate.

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