refactor(anta.tests)!: Update the pass/fail criteria for testcase VerifyBFD #504
Conversation
gmuloc
changed the title
gmuloc
changed the title
anta/tests/bfd.py
Outdated
|
|
||
| class VerifyBFDPeersHealth(AntaTest): | ||
| """ | ||
| Verifies there is no IPv4 BFD peer in the down state, remote disc is not zero and last down should be above the threshold for all VRF. |
There was a problem hiding this comment.
What does remote disc mean?
There was a problem hiding this comment.
is it ok to mention discriminator value of the remote system
anta/tests/bfd.py
Outdated
| This class defines the input parameters of the testcase. | ||
| """ | ||
|
|
||
| last_down: Optional[int] = None |
There was a problem hiding this comment.
| last_down: Optional[int] = None | |
| last_down: Optional[int] = Field(default=None, gt=0) |
There was a problem hiding this comment.
Also, I am not sure I like last_down as an input. It is not very descriptive.
There was a problem hiding this comment.
i can update the doc string as
Optional last down threshold in hours to check if a bfd peer was down before those hours or not.
There was a problem hiding this comment.
Maybe down_threshold?
anta/tests/bfd.py
Outdated
| """ | ||
|
|
||
| name = "VerifyBFDPeersHealth" | ||
| description = "Verifies there is no IPv4 BFD peer in the down state, remote disc is not zero and last down should be above the threshold for all VRF." |
There was a problem hiding this comment.
I think we should mention that the last down threshold value is optional somehow.
There was a problem hiding this comment.
description = "Verifies there is no IPv4 BFD peer in the down state and discriminator value of the remote system is not zero for all VRF. BFD peer last down in hours is optional check which should be above the threshold for all VRF."
There was a problem hiding this comment.
Looks good, you can modify depending on how you decide to rename the last_down variable.
tests/units/anta_tests/test_bfd.py
Outdated
| "expected": {"result": "success"}, | ||
| }, | ||
| { | ||
| "name": "success-no-peer", |
There was a problem hiding this comment.
Shouldn't we fail the test if there is no peer?
anta/tests/bfd.py
Outdated
| for peer_data in neighbor_data["peerStats"].values(): | ||
| peer_status = peer_data["status"] | ||
| remote_disc = peer_data["remoteDisc"] | ||
| peer_l3intf = peer_data.get("l3intf", "") |
There was a problem hiding this comment.
Do we need that l3intf key?
There was a problem hiding this comment.
yes, earlier it was used in failure message so kept it same. Let me know if need to remove
There was a problem hiding this comment.
Can you investigate if the test can be refactored with a later revision (see my other comment) and check if this is still needed? Further testing might be required on EOS to check if this is necessary.
There was a problem hiding this comment.
As discussed removed the l3intf details from failure message.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
LGTM |
Description
Fixes #503
Checklist:
pre-commit run)tox -e testenv)