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

Modify tests to run on zero ports systems #3663

Merged
merged 21 commits into from
Sep 16, 2021

Conversation

slutati1536
Copy link
Contributor

@slutati1536 slutati1536 commented Jun 14, 2021

Signed-off-by: Sharon Lutati slutati@nvidia.com

Description of PR

Allow systems which starts with zero ports to run platform tests without failing even if the tests skip the interface checks.

  • Change the port verification to relay on config_db.json instead of minigraph.xml
  • Change places where test fail in case of zero ports on setup

Summary:
Modify tests so that they will pass also on setup with zero ports

Type of change

  • Add checks if PORT exists
  • Take port list from config_db.json instead of minigraph

Approach

What is the motivation for this PR?

Allow systems which starts with zero ports to run platform tests without failing even if the tests skip the interface checks.

How did you do it?

Rerun pre defined list of tests where system is defined with zero ports and modify tests one by one to pass but to keep functional as possible (taking into account that interfaces are not configured)

How did you verify/test it?

Rerun tests and ensure they are passing on a system with ports and with zero ports.

Any platform specific information?

No

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

T0 and T1-lag pre defined list of tests

Signed-off-by: Sharon Lutati <slutati@nvidia.com>
@slutati1536 slutati1536 requested review from jleveque and a team as code owners June 14, 2021 06:49
@ghost
Copy link

ghost commented Jun 14, 2021

CLA assistant check
All CLA requirements met.

@lgtm-com
Copy link

lgtm-com bot commented Jun 14, 2021

This pull request introduces 3 alerts and fixes 2 when merging 39f18e6 into a26f2d5 - view on LGTM.com

new alerts:

  • 3 for Unused import

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@liat-grozovik
Copy link
Collaborator

@slutati1536 please handle new LGTM errors

@liat-grozovik liat-grozovik changed the title tests fixes for zero ports setups Modify tests to run on zero ports systems Jun 15, 2021
@liat-grozovik
Copy link
Collaborator

@wangxin would you mind to help review the changes?

Signed-off-by: Sharon Lutati <slutati@nvidia.com>
Signed-off-by: Sharon Lutati <slutati@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2021

This pull request fixes 2 alerts when merging 4d061f5 into 7b4a9e1 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

Signed-off-by: Sharon Lutati <slutati@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jun 21, 2021

This pull request fixes 2 alerts when merging b96e0f4 into e6741d3 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

Signed-off-by: Sharon Lutati <slutati@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jun 23, 2021

This pull request fixes 2 alerts when merging 447d6fc into ab82dd1 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

Copy link
Collaborator

@liat-grozovik liat-grozovik left a comment

Choose a reason for hiding this comment

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

@slutati1536 please add for each test logic change the reason why it does not run when there are no interfaces and how the test behaves in this case: cont without failure, pass, ignore, etc.

tests/snmp/test_snmp_phy_entity.py Outdated Show resolved Hide resolved
tests/telemetry/test_telemetry.py Outdated Show resolved Hide resolved
tests/telemetry/test_telemetry.py Outdated Show resolved Hide resolved
Signed-off-by: Sharon Lutati <slutati@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jun 24, 2021

This pull request fixes 2 alerts when merging 136d9b9 into 428e44a - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@slutati1536 slutati1536 requested review from liat-grozovik and removed request for a team June 27, 2021 07:05
Signed-off-by: Sharon Lutati <slutati@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jun 27, 2021

This pull request fixes 2 alerts when merging ba0f3ec into 73c2d17 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

Signed-off-by: Sharon Lutati <slutati@nvidia.com>
Signed-off-by: Sharon Lutati <slutati@nvidia.com>
@lgtm-com
Copy link

lgtm-com bot commented Jun 28, 2021

This pull request fixes 2 alerts when merging b392c01 into 6cc2135 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

Copy link
Collaborator

@wangxin wangxin left a comment

Choose a reason for hiding this comment

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

LGTM except some minor comments. Can you resolve the merge conflict? @slutati1536

tests/common/platform/interface_utils.py Outdated Show resolved Hide resolved
@slutati1536 slutati1536 requested a review from sujinmkang as a code owner July 21, 2021 14:23
@sujinmkang sujinmkang requested a review from vdahiya12 August 6, 2021 17:10
@slutati1536
Copy link
Contributor Author

Hello All, I have a task in the works for handling this PR, which should be resolve by the end of agust.

@lgtm-com
Copy link

lgtm-com bot commented Aug 17, 2021

This pull request introduces 1 alert and fixes 2 when merging bc97af9 into 741c735 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@liat-grozovik
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

@sujinmkang can you please help to review?

@lgtm-com
Copy link

lgtm-com bot commented Aug 18, 2021

This pull request introduces 1 alert and fixes 2 when merging a9f9453 into f50ae15 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

Details: basing interfaces list on config_db (which is done by get_port_map)
should be done in all cases not only where asic is not None.
Also, added get_dev_conn to test_check_sfp_using_ethtool, as it is also used in that way
in other sfp tests.
@lgtm-com
Copy link

lgtm-com bot commented Aug 18, 2021

This pull request introduces 1 alert and fixes 2 when merging 998955e into b338a88 - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Sep 12, 2021

This pull request fixes 2 alerts when merging 15ad6f1 into eefc0e5 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@slutati1536 slutati1536 reopened this Sep 13, 2021
@liat-grozovik
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lgtm-com
Copy link

lgtm-com bot commented Sep 14, 2021

This pull request fixes 2 alerts when merging 4ce5ed0 into 171df86 - view on LGTM.com

fixed alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@wangxin
Copy link
Collaborator

wangxin commented Sep 15, 2021

@liat-grozovik Currently merging of this PR is blocked by your change request. If you have no concern with this PR, can you approve it? Then we can proceed with merging.

@wangxin wangxin merged commit 6f492c1 into sonic-net:master Sep 16, 2021
yxieca added a commit that referenced this pull request Sep 23, 2021
bingwang-ms pushed a commit that referenced this pull request Sep 23, 2021
vmittal-msft pushed a commit to vmittal-msft/sonic-mgmt that referenced this pull request Sep 28, 2021
Allow systems which starts with zero ports to run platform tests without failing even if the tests skip the interface checks.

Change the port verification to relay on config_db.json instead of minigraph.xml
Change places where test fail in case of zero ports on setup

What is the motivation for this PR?
Allow systems which starts with zero ports to run platform tests without failing even if the tests skip the interface checks.

How did you do it?
Rerun pre defined list of tests where system is defined with zero ports and modify tests one by one to pass but to keep functional as possible (taking into account that interfaces are not configured)

How did you verify/test it?
Rerun tests and ensure they are passing on a system with ports and with zero ports.

Signed-off-by: Sharon Lutati <slutati@nvidia.com>
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.

4 participants