Skip to content

tests: Fix wrong expectations in bgp_srv6_unicast topotest#21284

Merged
donaldsharp merged 2 commits intoFRRouting:masterfrom
cscarpitta:fix_bgp_grt_test
Mar 22, 2026
Merged

tests: Fix wrong expectations in bgp_srv6_unicast topotest#21284
donaldsharp merged 2 commits intoFRRouting:masterfrom
cscarpitta:fix_bgp_grt_test

Conversation

@cscarpitta
Copy link
Contributor

After removing sid export on R1, the test checks both 10.0.0.1/32 and 10.0.0.3/32 on R2 with expect_sid="", expecting neither to carry a SRv6 SID. This is wrong: 10.0.0.3/32 is originated by R3 which still has sid export configured, so it should still be
seen on R2 with r3_unicast_sid.

This wrong expectation was not caught because check_route() did not verify the absence of a SID when expect_sid="".

Fix the check for 10.0.0.3/32 to expect r3_unicast_sid on R2, while keeping 10.0.0.1/32 checked for absence of SID.

When check_route() is called with expect_sid="" to assert that a
route has no SRv6 SID, it silently succeeds even if a SID is
present.

Add an explicit check: when expect_sid is "", return an error if
a SRv6 SID is found on the route.

Signed-off-by: Carmine Scarpitta <[email protected]>
@frrbot frrbot bot added the tests Topotests, make check, etc label Mar 22, 2026
@cscarpitta cscarpitta changed the title tests: Fix wrong expectations in bgp_srv6_unicast` topotest tests: Fix wrong expectations in bgp_srv6_unicast topotest Mar 22, 2026
@greptile-apps
Copy link

greptile-apps bot commented Mar 22, 2026

Greptile Summary

This PR fixes two related bugs in the bgp_srv6_unicast topotest that caused incorrect route SID verification.

  • check_route() silent no-SID bypass (line 125–126): When expect_sid="", the original code relied on if expect_sid (which is falsy for an empty string) to drive the comparison, meaning the absence of a SRv6 SID was never actually asserted. The fix adds an explicit expect_sid == "" branch that returns an error if the route still carries a SID — closing a gap where incorrect SID presence would silently pass.
  • Wrong expectation in test_bgp_srv6_sid_unexport() (lines 271–278): After R1's SID export is removed, only R1's own prefixes lose their SID. The 10.0.0.3/32 prefix originates from R3 (which retains sid export auto) and should still carry r3_unicast_sid on R2. The prior code incorrectly expected no SID for both 10.0.0.1/32 and 10.0.0.3/32. The fix separates the two checks: 10.0.0.1/32 is verified without a SID, while 10.0.0.3/32 is verified with r3_unicast_sid.

Confidence Score: 5/5

  • This PR is safe to merge — it corrects two clearly identified test bugs with no production code impact.
  • The changes are confined to a single test file, fix a genuine logic gap in check_route() (silent no-SID skip), and correct a provably wrong expectation for 10.0.0.3/32. Both fixes are consistent with the rest of the test's design and topology description. No ambiguity remains.
  • No files require special attention.

Important Files Changed

Filename Overview
tests/topotests/bgp_srv6_unicast/test_bgp_srv6_unicast.py Two-part bug fix: (1) check_route() now correctly validates absence of a SRv6 SID when expect_sid="" was previously silently ignored; (2) test_bgp_srv6_sid_unexport() corrects the expectation for 10.0.0.3/32 to use r3_unicast_sid since R3 retains its SID export configuration.

Sequence Diagram

sequenceDiagram
    participant R1
    participant R2
    participant R3

    Note over R1: no sid export auto (unconfigured)
    Note over R3: sid export auto (still active)

    R1->>R2: Advertise 10.0.0.1/32 (no SRv6 SID)
    R2->>R2: Install 10.0.0.1/32 — expect_sid=""  ✓

    R3->>R1: Advertise 10.0.0.3/32 + r3_unicast_sid
    R1->>R2: Re-advertise 10.0.0.3/32 + r3_unicast_sid
    R2->>R2: Install 10.0.0.3/32 — expect_sid=r3_unicast_sid  ✓ (was wrongly "")
Loading

Reviews (1): Last reviewed commit: "tests: Fix test_bgp_srv6_sid_unexport ex..." | Re-trigger Greptile

After removing sid export on R1, the test checks both 10.0.0.1/32
and 10.0.0.3/32 on R2 with expect_sid="", expecting neither to
carry a SRv6 SID. This is wrong: 10.0.0.3/32 is originated by R3
which still has sid export configured, so it should still be
seen on R2 with r3_unicast_sid.

This wrong expectation was not caught because check_route() did
not verify the absence of a SID when expect_sid="".

Fix the check for 10.0.0.3/32 to expect r3_unicast_sid on R2,
while keeping 10.0.0.1/32 checked for absence of SID.

Signed-off-by: Carmine Scarpitta <[email protected]>
@donaldsharp donaldsharp merged commit 5b86c31 into FRRouting:master Mar 22, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

master size/S tests Topotests, make check, etc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants