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

test for align watermark flow with port configuration #1

Closed
wants to merge 1 commit into from

Conversation

dbarashinvd
Copy link
Owner

@dbarashinvd dbarashinvd commented Nov 16, 2022

Description of PR

Summary:
Fixes # (issue)

Type of change

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

Back port request

  • 201911
  • 202012
  • 202205

Approach

What is the motivation for this PR?

add a verification test for feature align watermark flow with port configuration

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

Copy link

@roy-sror roy-sror left a comment

Choose a reason for hiding this comment

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

Hi Doron,

  1. To cover the different scenarios, I suggest using pytest parametrization. It has some advantages comparing to the existing implementation.
  2. There's no usage of allure step, it would be much more readable to have allure step when running the test with allure reporter.
  3. Did you consider adding tests to cover the functional correctness of the counters?

@dbarashinvd
Copy link
Owner Author

Hi Doron,

  1. To cover the different scenarios, I suggest using pytest parametrization. It has some advantages comparing to the existing implementation.
  2. There's no usage of allure step, it would be much more readable to have allure step when running the test with allure reporter.
  3. Did you consider adding tests to cover the functional correctness of the counters?

since we agreed to use randomization, didn't use pytest parametrization
added alure steps
random choice of reload/reboot and queue/watermark/pg-drop minimized the test duration to ~ 500 seconds.

@JibinBao
Copy link

It seems there is missing a test: "Adding a port when watermark is enabled" which is defined in HLD.
Can we need to add it?

@dbarashinvd
Copy link
Owner Author

dbarashinvd commented Nov 28, 2022

It seems there is missing a test: "Adding a port when watermark is enabled" which is defined in HLD. Can we need to add it?

Hi Jibin,
this is a test defined in the feature implementation HLD, not the test plan HLD.
it's covered in a unit test and is part of the public PR for the implementation in sonic-swss 2525

Also note that it's not just adding a port now since there was an improvement adding counterpoll flex counters only for ports with queue or pg buffers: sonic-swss 2473

so now it's creating a buffer queue and buffer pg for a certain port and checks the flex counters created for it

if needed I can add such a test also for this test here but keep in mind it'll make the test longer than it's now, about 500 seconds after randomization requested by Roy.

@JibinBao
Copy link

JibinBao commented Nov 28, 2022

It seems there is missing a test: "Adding a port when watermark is enabled" which is defined in HLD. Can we need to add it?

Hi Jibin, this is a test defined in the feature implementation HLD, not the test plan HLD. it's covered in a unit test and is part of the public PR for the implementation: sonic-swss 2525

Also note that it's not just adding a port now since there was an improvement adding counterpoll flex counters only for ports with queue or pg buffers: sonic-swss 2473 so now it's creating a buffer queue and buffer pg for a certain port and checks the flex counters created for it

if needed I can add such a test also for this test here but keep in mind it'll make the test longer than it's now, about 500 seconds after randomization requested by Roy.

Hi dbarashinvd,
I am not sure if the unit test can cover this scenario or not. If yes, I think it is ok. Otherwise, we will lose coverage.

@dbarashinvd dbarashinvd force-pushed the dbarashi_align_watermark_tests branch 2 times, most recently from e34fd19 to 7b4a2b9 Compare November 28, 2022 11:37
@dbarashinvd dbarashinvd requested review from JibinBao and removed request for roy-sror November 28, 2022 15:03
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