-
Notifications
You must be signed in to change notification settings - Fork 716
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
Voltage and Current sensors CLI tests #10736
base: master
Are you sure you want to change the base?
Conversation
The pre-commit check detected issues in the files touched by this pull request. Detailed pre-commit check results: To run the pre-commit checks locally, you can follow below steps:
|
Hey @bmridul, could you sync this PR with the latest master as it's been here for quite a while, please? It's also a good chance to rerun the PR checks. Thanks! |
""" | ||
duthost = duthosts[enum_rand_one_per_hwsku_hostname] | ||
cmd = " ".join([CMD_SHOW_PLATFORM, "voltage"]) | ||
check_show_platform_sensor_output(cmd, duthost): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wha is this :
for? Is this a typo?
logging.info("Verifying output of '{}' on '{}'...".format(cmd, duthost.hostname)) | ||
raw_output_lines = duthost.command(cmd)["stdout_lines"] | ||
|
||
pytest_assert(len(raw_output_lines) > 0, "There must be at least one line of output on '{}'".format(hostname)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is hostname
here and the ones in below? Should them be duthost.hostname
?
@summary: Run and verify output of `show platform [voltage|current]`. Expected output | ||
is "Sensor Not detected" or a table of sensor status data with 8 columns. | ||
""" | ||
num_expected_clos = 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo here, should be num_expected_cols
""" | ||
duthost = duthosts[enum_rand_one_per_hwsku_hostname] | ||
cmd = " ".join([CMD_SHOW_PLATFORM, "current"]) | ||
check_show_platform_sensor_output(cmd, duthost): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto the extra :
|
||
pytest_assert(len(raw_output_lines) > 0, "There must be at least one line of output on '{}'".format(hostname)) | ||
if len(raw_output_lines) == 1: | ||
pytest_assert(raw_output_lines[0].strip() == "Sensor Not detected", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure it's Not
with a capital N
? I think it'd better if we can lowercase raw_output_lines[0].strip()
and then do the comparison, so we don't need to worry about the cases.
second_line = raw_output_lines[1] | ||
field_ranges = util.get_field_range(second_line) | ||
pytest_assert(len(field_ranges) == num_expected_clos, "Output should consist of {} columns on '{}'". | ||
format(num_expected_clos, hostname)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this also check that the alarm status for every line of sensor data reads false, indicating no alarm? Perhaps test_thermal.py
can instead be copied/expanded for coverage of sensors of other types (voltage, current, etc.)
Description of PR
Sonic-mgmt tests for CLI introduced as part of Sensormon. HLD - sonic-net/SONiC#1394
Summary:
Added tests for Sensormon supported CLIs for
show platform voltage
show platform current
Type of change
Back port request
Approach
What is the motivation for this PR?
Added first set of sonic mgmt tests for Sensormon feature.
How did you verify/test it?
Ran the tests on the DUT.
Any platform specific information?
Supported testbed topology if it's a new test case?
Any. Should be applicable to all.
Documentation
HLD link provided above.