fix(anta.tests): First round of cleaning up BGP tests module #888
Conversation
CodSpeed Performance ReportMerging #888 will not alter performanceComparing Summary
|
vitthalmagadum
force-pushed
the
feat/bgp_modules
branch
from
d898a60
to
887c6a1
Compare
There was a problem hiding this comment.
This PR seems quite breaking for the tests as we are suddenly checking more things.
to get a success like Count is now dpeendent on the state of the peer when it did not used to be.
Ideally we could add a flag for this one to not be concerned with the the state (default being we are not) to be backward compatible
anta/input_models/__init__.py
Outdated
| # Copyright (c) 2023-2024 Arista Networks, Inc. | ||
| # Use of this source code is governed by the Apache License 2.0 | ||
| # that can be found in the LICENSE file. | ||
| """Package related to all ANTA tests input models.""" |
There was a problem hiding this comment.
Package or module starting with test could be confusing with pytest.
|
|
||
| Please refer to the Input class attributes below for details. | ||
| 1. Confirms that the specified VRF is configured. | ||
| 2. Counts the number of peers that are: |
There was a problem hiding this comment.
Hmm this is breaking right? before we were "just" counting
There was a problem hiding this comment.
Right. As discussed, let's add a flag like you mentionned in your requested changes.
There was a problem hiding this comment.
We should update this to specify that if check_peer_state: true we also check the Established state + AFI/SAFI
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
vitthalmagadum
force-pushed
the
feat/bgp_modules
branch
from
707d01a
to
44581a0
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
vitthalmagadum
force-pushed
the
feat/bgp_modules
branch
from
1e15c52
to
c9f240d
Compare
|
| check_peer_state: bool = True | ||
| """Flag to check if the peers are established with negotiated AFI/SAFI. Defaults to `True`. |
There was a problem hiding this comment.
Default should be False to follow the current behavior where we just check the number of peers.
| @@ -209,129 +145,80 @@ class VerifyBGPPeerCount(AntaTest): | |||
| ``` | |||
| """ | |||
|
|
|||
| description = "Verifies the count of BGP peers." | |||
| name = "VerifyBGPPeerCount" | |||
| description = "Verifies the count of established BGP peers with negotiated AFI/SAFI for given address families." | |||
There was a problem hiding this comment.
This too :)
| * Success: If the count of established BGP peers with negotiated AFI/SAFI matches the expected count for all specified address families. | ||
| * Failure: If any of the following occur: | ||
| - The specified VRF is not configured. | ||
| - The count of established peers with negotiated AFI/SAFI does not match the expected count for any address family. |
There was a problem hiding this comment.
Need to update this as well per the check_peer_state flag.
| peers_data = vrf_output.get("peers", {}).values() | ||
| if not address_family.check_peer_state: | ||
| # Count the number of peers without considering the state and negotiated AFI/SAFI check if the count matches the expected count | ||
| peer_count = sum(1 for peer_data in peers_data if address_family.eos_key in peer_data) |
There was a problem hiding this comment.
Nice use of sum(). Cleaner this way thanks.
Description
Refactoring BGP tests module to address the following issues:
VerifyBGPPeerCountorVerifyBGPPeersHealthtest to also checks for a minimum established time for BGP session #635Focusing on VerifyBGPPeerCount, VerifyBGPPeersHealth, VerifyBGPSpecificPeers
Task list:
check_tcp_queuesis Truecheck_bgp_neighbor_capability(IMP: Added unit tests intest_tools.)format_dataChanges
orevaluates lazily, meaning if the first part (InQ := value) != 0 is True, the second part (OutQ := value) != 0 will not be evaluated, results OutQ is never assigned, leading to a NameError._check_bgp_neighbor_capabilityto bgp module as intended to BGP module only.