Converting legacy platform tests for a T2 chassis, adding validation of values to tests, and refactor test_sfp into smaller tests by sanmalho-git · Pull Request #2965 · sonic-net/sonic-mgmt

sanmalho-git · 2021-02-09T23:51:28Z

Description of PR

Summary:
Fixes # (issue)

Type of change

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

Approach

What is the motivation for this PR?

Need to run the legacy platform tests against a T2 chassis. However, some of the tests would run only on supervisor card, while others would run only on frontend nodes (linecards).

So, need to convert these tests to use the enum_* fixtures for dut selection, rather than rand_one_dut_hostname.

Also, we need to add some validation of the commands based on some variables that are defined in the inventory file - similar to the new PMON API validation.

How did you do it?

The following modifications are made to some of the legacy platform tests:

conftest.py:
- Some of the platform tests run only on a supervisor card for a chassis. So, needed to change rand_one_dut_hostname to enum_dut_supervisor_hostname. However, in a multi-dut testbed that has no supervisor card (like DualTor), we would raise an exception that no supervisor card is present. Instead, we will now just return the first dut in the testbed.
- Modified generate_params_supervisor_hostname function for this behavior.
test_xcvr_info_in_db.py:
- Use enum_rand_one_per_hwsku_frontend_hostname to run test against frontend node instead of rand_one_dut_hostname that could give us a supervisor card.
test_sfp:
- Broke into 3 smaller test for the 3 modules being tested
  - sfpshow
  - sfputil
  - show interface transceiver
- These tests were added as a directory 'sfp' under platform_tests
- In these tests:
  - Use enum_rand_one_per_hwsku_frontend_hostname to run test against frontend node instead of rand_one_dut_hostname
  - Added validation of 'sfpshow presence' and 'sfpshow eeprom'.
test_show_platform.py:
- Use enum_rand_one_per_hwsku_frontend_hostname to run tests against frontend node instead of rand_one_dut_hostname
- test_show_platform_summary:
  - Added validation of fields in 'show platform summary' based on values in the inventory if defined.
    - hwsku - based on 'hwsku' variable in the inventory
    - platform - based on 'sonic_hw_platform' variable in the inventory
    - asic - based on 'asic_type' variable in the inventory
    - asic count - based on 'num_asics' variable in the inventory
- test_show_syseeprom:
  - Validate values against 'syseeprom_info' dictionary if defined in the inventory
- test_show_platform_psustatus:
  - Validate that atleast one of the PSU's shows status as OK.
- test_show_platform_fan:
  - Validate that atleast one of the fans shows status as OK.

How did you verify/test it?

Validated the modified tests against a chassis and also a pizza box.

Any platform specific information?

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

Documentation

lgtm-com · 2021-02-10T00:19:03Z

This pull request introduces 2 alerts when merging 16e71b816b8d56f01dc0c05046bbeaa125829d73 into 451d052 - view on LGTM.com

new alerts:

1 for Testing equality to None
1 for Except block handles 'BaseException'

tests/platform_tests/cli/test_show_platform.py

jleveque · 2021-02-10T02:26:51Z

tests/platform_tests/cli/test_show_platform.py

I think we should expect the syseeprom_info field to be present in the inventory file to prevent skipping this test and potentially missing an issue. Maybe we should assert here and fail the test if not present?

I was trying to keep it backward compatible. With your proposed change, it will enforce everybody to add syseeprom_info for all their devices into the inventory file. Is that what we want to do.

Yes, that's what I'm inferring. In order to have a full, comprehensive test, syseeprom_info data should be added to the inventory file. I think the test should fail if there's nothing to test against. We are currently in the process of improving test coverage, so I think this assumption in fine moving forward. Thank you for considering backward compatibility here, but when it comes to testing, we need to set some ground rules.

Also, the platform API tests are designed to fail if the expected data is not found.

tests/platform_tests/test_sfp.py

jleveque

I think it might be best to split tests/platform_tests/test_sfp.py into two files:

tests/platform_tests/test_sfputil.py
tests/platform_tests/test_sfpshow.py

Thoughts?

sanmalho-git · 2021-02-15T15:57:33Z

how about splitting each show command into its own test in the same test_sfp.py

jleveque · 2021-02-16T22:14:01Z

how about splitting each show command into its own test in the same test_sfp.py

I think it makes things cleaner to have one file dedicated to testing one utility.

The show platform ... subcommands will be tested in test_show_platform.py, sfputil subcommands will be tested in test_sfputil.py and sfpshow commands will be tested in test_sfpshow. It will help isolate the tests and also keep the file sizes down.

…on of values to some tests The following modifications are made to some of the legacy platform tests: - conftest.py: - Some of the platform tests run only on a supervisor card for a chassis. So, needed to change rand_one_dut_hostname to enum_dut_supervisor_hostname. However, in a multi-dut testbed that has no supervisor card (like DualTor), we would raise an exception that no supervisor card is present. Instead, we will now just return the first dut in the testbed. - Modified generate_params_supervisor_hostname function for this behavior. - test_xcvr_info_in_db.py: - Use enum_rand_one_per_hwsku_frontend_hostname to run test against frontend node instead of rand_one_dut_hostname that could give us a supervisor card. - test_sfp: - Use enum_rand_one_per_hwsku_frontend_hostname to run test against frontend node instead of rand_one_dut_hostname - Some of the new platforms don't support sfputils. So, added a check based on the presence of file /usr/share/sonic/device/<platform>/plugins/spfutil.py" If the file is not present, then we assume that sfputils is not supported. In this case, we don't do sfputils related steps in test_check_sfp_status_and_configure_sfp test. and skip test_check_sfp_low_power_mode test. - test_show_platform.py: - Use enum_rand_one_per_hwsku_frontend_hostname to run tests against frontend node instead of rand_one_dut_hostname - test_show_platform_summary: - Added validation of fields in 'show platform summary' based on values in the inventory if defined. - hwsku - based on 'hwsku' variable in the inventory - platform - based on 'sonic_hw_platform' variable in the inventory - asic - based on 'asic_type' variable in the inventory - asic count - based on 'num_asics' variable in the inventory - test_show_syseeprom: - Validate values against 'syseeprom_info' dictionary if defined in the inventory - test_show_platform_psustatus: - Validate that atleast one of the PSU's shows status as OK. - test_show_platform_fan: - Validate that atleast one of the fans shows status as OK.

test_sfp.py: - removed check for spfutils not supported test_show_platform.py: - Removed extra spaces as per review comments

Broke up test_sfp into 3 tests for the 3 different modules being tested - sfpshow - test_sfpshow.py - sfputil - test_sfputil.py - show interface transciever - test_show_intf_xvrc.py The files were added into 'sfp' directory under platform tests to allow for reuse of utilities and fixtures. Removed original test_sfp.py from platform_tests directory.

sanmalho-git · 2021-02-18T21:48:40Z

@jleveque Have refactored test_sfp into smaller tests under platform_tests/sfp. Please re-review.

tests/platform_tests/cli/test_show_platform.py

tests/platform_tests/sfp/test_sfpshow.py

tests/platform_tests/sfp/test_show_intf_xcvr.py

lgtm-com · 2021-02-19T01:34:12Z

This pull request introduces 1 alert when merging b722eb6 into 856c106 - view on LGTM.com

new alerts:

1 for Testing equality to None

jleveque

Please fix LGTM alert.

sanmalho-git · 2021-02-19T01:39:04Z

Done

sanmalho-git requested review from a team and jleveque as code owners February 9, 2021 23:51

jleveque suggested changes Feb 10, 2021

View reviewed changes

jleveque added Enhancement Platform labels Feb 10, 2021

jleveque suggested changes Feb 12, 2021

View reviewed changes

sanmalho-git added 5 commits February 18, 2021 16:28

Fixing lgtm warnings.

bd2bd63

Incorporating review comments

5c55487

test_sfp.py: - removed check for spfutils not supported test_show_platform.py: - Removed extra spaces as per review comments

Added sfpshow command validation to test_sfp.py

c546432

sanmalho-git force-pushed the platform branch from ad7bc58 to 3197fa4 Compare February 18, 2021 21:43

sanmalho-git changed the title ~~Converting legacy platform tests for a T2 chassis and adding validation of values to some tests~~ Converting legacy platform tests for a T2 chassis, adding validation of values to tests, and refactor test_sfp into smaller tests Feb 18, 2021

jleveque suggested changes Feb 18, 2021

View reviewed changes

Changes to use str.format syntax as per review comments

3ec552e

jleveque approved these changes Feb 19, 2021

View reviewed changes

Changes to use str.format

b722eb6

jleveque suggested changes Feb 19, 2021

View reviewed changes

Fixed lgtm alert

24fd7b3

jleveque approved these changes Feb 19, 2021

View reviewed changes

yxieca merged commit cdf78e3 into sonic-net:master Feb 26, 2021

sanmalho-git deleted the platform branch April 28, 2021 17:43

Conversation

sanmalho-git commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Approach

What is the motivation for this PR?

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

Uh oh!

lgtm-com bot commented Feb 10, 2021

Uh oh!

Uh oh!

Uh oh!

jleveque Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

sanmalho-git Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

jleveque Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jleveque left a comment

Choose a reason for hiding this comment

Uh oh!

sanmalho-git commented Feb 15, 2021

Uh oh!

jleveque commented Feb 16, 2021

Uh oh!

sanmalho-git commented Feb 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Feb 19, 2021

Uh oh!

jleveque left a comment

Choose a reason for hiding this comment

Uh oh!

sanmalho-git commented Feb 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sanmalho-git commented Feb 9, 2021 •

edited

Loading

jleveque Feb 12, 2021 •

edited

Loading