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

Make PFC commands use a class #3057

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

Conversation

bktsim-arista
Copy link

@bktsim-arista bktsim-arista commented Nov 30, 2023

What I did

This change puts contents originally in pfc/main.py into a class, to support the usage of the multi-asic helper in a future change. This change is required, as multi-asic helper being used expects members self.config_db and self.db to exist when a function with the decorator run_on_multi_asic is called. The multi-asic class helper will be used to add multi-asic support to pfc commands in a following pull request.

This is a part of the set of changes being pushed for sonic-net/sonic-buildimage#15148

How I did it

Moved contents of PFC commands into a class. There are no functional changes.

@arista-nwolfe
Copy link

/azpw run Azure.sonic-utilities

@wenyiz2021
Copy link
Contributor

/azpw run

@arlakshm
Copy link
Contributor

/azpw run Azure.sonic-utilities

@arlakshm
Copy link
Contributor

/azp run Azure.sonic-utilities

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

vmittal-msft
vmittal-msft previously approved these changes Mar 22, 2024
@vmittal-msft
Copy link

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -1,160 +1,182 @@
#!/usr/bin/env python3

import os
Copy link
Contributor

Choose a reason for hiding this comment

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

please sort imports

Choose a reason for hiding this comment

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

Done


class TestPfc(TestPfcBase):
@classmethod
def setup_class(cls):
Copy link
Contributor

Choose a reason for hiding this comment

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

teardown needs to be called for TestPfc to set the UTILITIES_UNIT_TESTING to zero

Choose a reason for hiding this comment

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

teardown_class is defined in the base class TestPfcBase, it should inherit that method.

PFC handler to display asymmetric PFC information.
"""
header = ('Interface', 'Asymmetric')
if "UTILITIES_UNIT_TESTING" in os.environ and os.environ["UTILITIES_UNIT_TESTING"] == "2":
Copy link
Contributor

Choose a reason for hiding this comment

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

why do need this change?

Choose a reason for hiding this comment

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

I don't think we need it, it looks like it was just to make writing the test easier but we can just as easily call the config command followed by the show command in the test.


def test_pfc_config_priority(self):
self.executor(pfc.cli, ['config', 'priority', 'on', 'Ethernet0', '5'],
expected_output=pfc_config_priority_on)
Copy link
Contributor

Choose a reason for hiding this comment

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

add test for showPfcPrio ?

Choose a reason for hiding this comment

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

It looks like there already is one for showPfcPrio see test_pfc_show_priority_all, test_pfc_show_priority_intf and test_pfc_show_asymmetric_intf_fake

bktsim-arista and others added 3 commits May 13, 2024 14:08
This change puts contents originally in pfc/main.py into a class,
to support the usage of the multi-asic helper in a future change.
This change is required, as multi-asic helper relies on certain members
of the class to exist. The multi-asic class helper will be used to add
multi-asic support to pfc commands.
expected_output=pfc_config_asymmetric)

def test_pfc_config_priority(self):
self.executor(pfc.cli, ['config', 'priority', 'on', 'Ethernet0', '5'],

Choose a reason for hiding this comment

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

@bktsim-arista Can we add "show pfc counter" test as well ? Also, Could we test this change on pizza box as well as chassis ?

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

Successfully merging this pull request may close these issues.

None yet

6 participants