Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(anta.tests): VerifySpecificPath testcase with the latest changes #965

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

geetanjalimanegslab
Copy link
Collaborator

@geetanjalimanegslab geetanjalimanegslab commented Dec 13, 2024

Description

Refactoring PathSelection (VerifySpecificPath) tests module to address the following issues:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have run pre-commit for code linting and typing (pre-commit run)
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes (tox -e testenv)

Copy link

codspeed-hq bot commented Dec 13, 2024

CodSpeed Performance Report

Merging #965 will not alter performance

Comparing geetanjalimanegslab:feat_verify_route_path (ce300b8) with main (164b736)

Summary

✅ 22 untouched benchmarks

anta/tests/path_selection.py Outdated Show resolved Hide resolved
anta/tests/path_selection.py Outdated Show resolved Hide resolved
anta/tests/path_selection.py Outdated Show resolved Hide resolved
anta/tests/path_selection.py Outdated Show resolved Hide resolved
anta/tests/path_selection.py Outdated Show resolved Hide resolved
@vitthalmagadum vitthalmagadum marked this pull request as ready for review January 7, 2025 09:11

The expected states are 'IPsec established', 'Resolved' for path and 'active' for telemetry.
This test performs the following checks for each specified router path:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This test performs the following checks for each specified router path:
This test performs the following checks for each specified DPS path:


1. Verifies that the specified peer is configured.
2. Verifies that the specified path group is found.
3. Verifies that the expected source and destination address match the path group.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?


# If the dpsPeers details are not found in the command output, the test fails.
if not (dps_peers_details := get_value(command_output, "dpsPeers")):
self.result.is_failure("Router path not configured")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.result.is_failure("Router path not configured")
self.result.is_failure("Router path-selection not configured")

self.result.is_failure("Router path not configured")
return

# Iterating on each router path mentioned in the inputs.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Iterating on each router path mentioned in the inputs.
# Iterating on each DPS peer mentioned in the inputs.

commands: ClassVar[list[AntaCommand | AntaTemplate]] = [
AntaTemplate(template="show path-selection paths peer {peer} path-group {group} source {source} destination {destination}", revision=1)
]
commands: ClassVar[list[AntaCommand | AntaTemplate]] = [AntaCommand(command="show path-selection paths", revision=1)]

class Input(AntaTest.Input):
"""Input model for the VerifySpecificPath test."""

paths: list[RouterPath]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
paths: list[RouterPath]
paths: list[DpsPath]

path_data = next((path for path in path_group_details.values() if (path.get("source") == source and path.get("destination") == destination)), None)
# If the expected and actual source and destination address of the path group are not matched, test fails.
if not path_data:
self.result.is_failure(f"{router_path} - Path not found")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.result.is_failure(f"{router_path} - Path not found")
self.result.is_failure(f"{router_path} - No path matching the source and destination found.")


def __str__(self) -> str:
"""Return a human-readable string representation of the RouterPath for reporting."""
return f"Peer: {self.peer} PathGroup: {self.path_group} Source: {self.source_address} Destination: {self.destination_address}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return f"Peer: {self.peer} PathGroup: {self.path_group} Source: {self.source_address} Destination: {self.destination_address}"
return f"Peer: {self.peer}, PathGroup: {self.path_group}, Source: {self.source_address}, Destination: {self.destination_address}"


path_state = path_data.get("state")
session = get_value(path_data, "dpsSessions.0.active")
expected_state = ["ipsecEstablished", "routeResolved"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need this variable? why not just put it directly in line 153?

if path_state not in ["ipsecEstablished", "routeResolved"]:
self.result.is_failure(f"Path state for `peer: {peer} source: {source} destination: {destination}` in path-group {path_group} is `{path_state}`.")
if path_state not in expected_state:
self.result.is_failure(f"{router_path} - State is not in ipsecEstablished, routeResolved - Actual: {path_state}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.result.is_failure(f"{router_path} - State is not in ipsecEstablished, routeResolved - Actual: {path_state}")
self.result.is_failure(f"{router_path} - State is not in ipsecEstablished, routeResolved. Actual: {path_state}")

self.result.is_failure(
f"Telemetry state for path `peer: {peer} source: {source} destination: {destination}` in path-group {path_group} is `inactive`."
)
self.result.is_failure(f"{router_path} - Telemetry state inactive")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.result.is_failure(f"{router_path} - Telemetry state inactive")
self.result.is_failure(f"{router_path} - Telemetry state inactive for this path")

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@geetanjalimanegslab geetanjalimanegslab marked this pull request as draft January 15, 2025 14:48
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@vitthalmagadum vitthalmagadum marked this pull request as ready for review January 15, 2025 16:21

Expected Results
----------------
* Success: The test will pass if the path state under router path-selection is either 'IPsec established' or 'Resolved'
* Success: The test will pass if the path state under router path selection is either 'IPsecEstablished' or 'Resolved'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Success: The test will pass if the path state under router path selection is either 'IPsecEstablished' or 'Resolved'
* Success: The test will pass if the path state under router path-selection is either 'IPsecEstablished' or 'Resolved'

and telemetry state as 'active'.
* Failure: The test will fail if router path-selection is not configured or if the path state is not 'IPsec established' or 'Resolved',
or if the telemetry state is 'inactive'.
* Failure: The test will fail if router path selection is not configured, the path state is not 'IPsec established' or 'Resolved',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Failure: The test will fail if router path selection is not configured, the path state is not 'IPsec established' or 'Resolved',
* Failure: The test will fail if router path selection or the peer is not configured or if the path state is not 'IPsec established' or 'Resolved',

return [
template.render(peer=path.peer, group=path.path_group, source=path.source_address, destination=path.destination_address) for path in self.inputs.paths
]
DpsPath: ClassVar[type[DpsPath]] = DpsPath
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DpsPath: ClassVar[type[DpsPath]] = DpsPath
RouterPath: ClassVar[type[DpsPath]] = DpsPath
"""To maintain backward compatibility."""

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants