Skip to content

Add script platform_tests/test_platform_info.py into T0 PR checker.#13353

Merged
wangxin merged 6 commits intosonic-net:masterfrom
yutongzhang-microsoft:yutongzhang/skip_thermal
Jun 26, 2024
Merged

Add script platform_tests/test_platform_info.py into T0 PR checker.#13353
wangxin merged 6 commits intosonic-net:masterfrom
yutongzhang-microsoft:yutongzhang/skip_thermal

Conversation

@yutongzhang-microsoft
Copy link
Copy Markdown
Contributor

@yutongzhang-microsoft yutongzhang-microsoft commented Jun 19, 2024 •
edited
Loading

Description of PR

In PR #13220, we add a batch of control plane test scripts into onboarding T0 PR checker, but some of them failed. In this PR, we fix the failed test script platform_tests/test_platform_info.py and let it run successfully in T0 PR checker.

Summary:
Fixes # (issue)

Type of change

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

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405

Approach

What is the motivation for this PR?

In PR #13220, we add a batch of control plane test scripts into onboarding T0 PR checker, but some of them failed. In this PR, we fix the failed test script platform_tests/test_platform_info.py and let it run successfully in T0 PR checker.

How did you do it?

How did you verify/test it?

=========================== short test summary info ============================
SKIPPED [1] common/helpers/assertions.py:16: No PSU controller for vlab-01, skip rest of the testing in this case
SKIPPED [3] platform_tests/thermal_control_test_helper.py:123: No mocker defined for this platform x86_64-kvm_x86_64-r0
============== 2 passed, 4 skipped, 1 warning in 64.33s (0:01:04) ==============

Any platform specific information?

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

Documentation

ZhaohuiS
ZhaohuiS reviewed Jun 19, 2024
View reviewed changes
Comment thread tests/platform_tests/test_platform_info.py Outdated
healthy_psus = 0
psuutil_status_output = duthost.command(PSUUTIL_CMD)
psuutil_status_output = duthost.command(PSUUTIL_CMD, module_ignore_errors=True)
if psuutil_status_output['rc'] != 0:
Copy link
Copy Markdown
Contributor

@ZhaohuiS ZhaohuiS Jun 19, 2024

Choose a reason for hiding this comment

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

@yutongzhang-microsoft if rc !=0, is there any possibility that the command doesn't work or is something wrong on physical testbed? If so, how to handle this scenario?

Copy link
Copy Markdown
Contributor Author

@yutongzhang-microsoft yutongzhang-microsoft Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I will add another condition here, duthost.facts["asic_type"] == "vs"

ZhaohuiS
ZhaohuiS reviewed Jun 19, 2024
View reviewed changes
Comment thread tests/platform_tests/thermal_control_test_helper.py Outdated
if dut.facts["asic_type"] == "vs" and output["rc"] == 0:
pytest.skip("Thermalctld doesn't support on vs testbed")
else:
assert output["rc"] == 0, "Run command '%s' failed" % find_thermalctld_pid_cmd
Copy link
Copy Markdown
Contributor

@ZhaohuiS ZhaohuiS Jun 19, 2024

Choose a reason for hiding this comment

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

@yutongzhang-microsoft if you prefer to use format() to concentrate string, do you mind keeping using a same format for whole script?

Copy link
Copy Markdown
Contributor Author

@yutongzhang-microsoft yutongzhang-microsoft Jun 19, 2024

Choose a reason for hiding this comment

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

Sure, I will keep the same format in this script

wangxin
wangxin requested changes Jun 20, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Maybe the better idea is to expect different rc and output when DUT is vs setup.
Use pytest.skip here does not make sense.
If there is no point running the platform related commands at all, then add device_type "vs" mark is a better idea.

@yutongzhang-microsoft
Copy link
Copy Markdown
Contributor Author

yutongzhang-microsoft commented Jun 21, 2024

Maybe the better idea is to expect different rc and output when DUT is vs setup. Use pytest.skip here does not make sense. If there is no point running the platform related commands at all, then add device_type "vs" mark is a better idea.

Hi, Xin, some part of the test such as running a command is worth to be tested as we discussed offline. So I use return instead of skip, to execute to left part.

xwjiang-ms
xwjiang-ms approved these changes Jun 21, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@xwjiang-ms xwjiang-ms left a comment

Choose a reason for hiding this comment

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

LGTM

@yutongzhang-microsoft yutongzhang-microsoft changed the title Skip thermal test cases on vs testbed. Add script test_platform_info.py into T0 PR checker. Jun 24, 2024
@yutongzhang-microsoft yutongzhang-microsoft changed the title Add script test_platform_info.py into T0 PR checker. Add script platform_tests/test_platform_info.py into T0 PR checker. Jun 26, 2024
@wangxin wangxin merged commit 8aa12f6 into sonic-net:master Jun 26, 2024
@yutongzhang-microsoft yutongzhang-microsoft deleted the yutongzhang/skip_thermal branch June 26, 2024 08:02
@Gfrom2016
Copy link
Copy Markdown
Contributor

Gfrom2016 commented Jul 10, 2024

@yutongzhang-microsoft Do we need to cherry-pick this to 202311 and 202405?

@yutongzhang-microsoft
Copy link
Copy Markdown
Contributor Author

yutongzhang-microsoft commented Jul 10, 2024

@yutongzhang-microsoft Do we need to cherry-pick this to 202311 and 202405?

These changes are aimed at fitting vs testbed, will have no influence on physical. You can cherry pick if you need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZhaohuiS ZhaohuiS ZhaohuiS left review comments

@wangxin wangxin wangxin approved these changes

@xwjiang-ms xwjiang-ms xwjiang-ms approved these changes

@prgeor prgeor Awaiting requested review from prgeor

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yutongzhang-microsoft @Gfrom2016 @wangxin @ZhaohuiS @xwjiang-ms