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

updating pfc tests #5

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

updating pfc tests #5

wants to merge 7 commits into from

Conversation

ANISH-GOTTAPU
Copy link
Owner

@ANISH-GOTTAPU ANISH-GOTTAPU commented Apr 1, 2021

Description of PR

Migrating PFC/PFCWD/ECN tests from abstract to snappi

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • [x ] Test case(new/improvement)

Approach

What is the motivation for this PR?

The motivation for this PR is to migrate the tests that are written with abstract open traffic generator to snappi which is more readable

How did you do it?

updated the tests with snappi

How did you verify/test it?

verified all the tests in sonic-mgmt framework with DUT and IXIA connections using the below version
snappi[ixnetwork]==0.3.13

tests/ixia/pfc/files/helper.py Show resolved Hide resolved
tests/ixia/pfc/files/helper.py Show resolved Hide resolved
@@ -11,6 +11,7 @@
from tests.common.ixia.common_helpers import get_vlan_subnet, get_addrs_in_subnet,\
get_peer_ixia_chassis
from tests.common.ixia.ixia_helpers import IxiaFanoutManager, get_tgen_location
import snappi

try:
from abstract_open_traffic_generator.port import Port
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to get rid of abstract_open_traffic_generator everywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And statements using abstract classes

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added TODO as discussed

@@ -193,11 +194,31 @@ def ixia_api(ixia_api_serv_ip,
if api_session:
api_session.assistant.Session.remove()


@pytest.fixture(scope='module')
def snappi_api(ixia_api_serv_ip,
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't use ixia

Copy link
Owner Author

Choose a reason for hiding this comment

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

as discussed, we need a meeting with Wei

ixia_api_serv_port (pytest fixture): ixia_api_serv_port fixture.
"""
host = "https://" + ixia_api_serv_ip + ":" + str(ixia_api_serv_port)
api = snappi.api(host=host, ext="ixnetwork")
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a comment here mentioning:

TODO: Currently extension is defaulted to ixnetwork but it doesn't need to be that way. Going forward, we should be able to specify extension from command line while running pytest.

Copy link
Owner Author

Choose a reason for hiding this comment

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

updated


yield api

if api.assistant is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is specific to IxNetwork, so add a check here. i.e. execute this statement only if assistant is a valid attribute of api.

Copy link
Owner Author

Choose a reason for hiding this comment

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

updated

config = testbed_config
config.flows = flows
config.captures = capture_config
__gen_traffic(testbed_config=testbed_config,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please review description of this function. It doesn't return config anymore, rather mutates the one that is passed as an argument.

Copy link
Owner Author

Choose a reason for hiding this comment

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

updated

ip_filter.custom.value = '40'
ip_filter.custom.offset = 14
ip_filter.custom.mask = '0f'


def __run_traffic(api,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function is filled with API calls yet no asserts are used for error checking. We should check for errors and raise it, and if there's a warning, log it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

updated

request = api.capture_request()
request.port_name = capture_port_name
pcap_bytes = api.get_capture(request)
pcap_bytes = pcap_bytes.getvalue()
Copy link
Collaborator

Choose a reason for hiding this comment

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

pcap_bytes.getvalue()

Do this directly when writing to file.

Copy link
Owner Author

Choose a reason for hiding this comment

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

updated

Copy link
Collaborator

@ashutshkumr ashutshkumr left a comment

Choose a reason for hiding this comment

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

Following needs to be addressed:

  • All the comments provided for ecn/helper.py applies to other helper files as well (e.g. change comments to provide correct info on returned values, check errors/warnings for api calls, etc.)
  • Do not import ixia_api_serv_user and ixia_api_serv_passwd in multiple files since we're not using those anymore
  • There are multiple such comments spread across multiple files:
snappi_api (pytest fixture): IXIA session

Instead of IXIA session, we should mention snappi API session

Copy link
Collaborator

@ashutshkumr ashutshkumr left a comment

Choose a reason for hiding this comment

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

LGTM

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