-
Notifications
You must be signed in to change notification settings - Fork 770
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
[Platform API]: Update test cases to make use of capabilities fields in platform.json #4332
[Platform API]: Update test cases to make use of capabilities fields in platform.json #4332
Conversation
…in platform.json Test cases modified: - Chassis: test_status_led - Chassis Fans: test_get_fans_target_speed, test_set_fans_speed, test_set_fans_led - FanDrawer: test_set_fan_drawers_led - FanDrawer Fans: test_get_fans_target_speed, test_set_fans_speed, test_set_fans_led - PSU: test_led - PSU Fans: test_get_fans_target_speed, test_set_fans_speed, test_set_fans_led - Thermal: test_set_low_threshold, test_set_high_threshold Signed-off-by: Arun Saravanan Balachandran <Arun_Saravanan_Balac@dell.com>
I am working on some test case changes to align with the capabilities present on one of our Nokia platforms. My changes leverage some of the changes made in this PR. Please review and merge this PR so that I can make progress on my PR. Thanks. |
The change looks good to me. Hi @sujinmkang , could you review this PR as well? Thanks! |
for i in range(self.num_fans): | ||
speed_target_val = 25 | ||
speed_controllable = self.get_fan_facts(duthost, i, True, "speed", "controllable") | ||
if not speed_controllable: |
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 happens with platforms that does not support the capability query? Wouldn't this cause those platforms to be skipped as well?
Am I missing something perhaps??
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.
never mind my comments... I see there is a default value involved with the call so it should be fine...
duthost = duthosts[enum_rand_one_per_hwsku_hostname] | ||
if duthost.facts["asic_type"] in ["cisco-8000"]: | ||
target_speed = random.randint(40, 60) | ||
else: | ||
target_speed = random.randint(1, 100) | ||
|
||
for i in range(self.num_fans): | ||
speed_controllable = self.get_fan_facts(duthost, i, True, "speed", "controllable") | ||
if not speed_controllable: |
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.
Same comment as my previous comment for platforms that does not support capability what happens? it will also be skipped? then that would be wrong right? it should check if platform does not support capability query yet should not be skipped??
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.
never mind my comments... I see there is a default value involved with the call so it should be fine...
|
||
for i in range(self.num_fans): | ||
led_controllable = self.get_fan_facts(duthost, i, True, "status_led", "controllable") | ||
if not led_controllable: |
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.
for platform that does not support capability query this check will cause their tests be skipped as well?
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.
never mind my comments... I see there is a default value involved with the call so it should be fine...
Signed-off-by: Arun Saravanan Balachandran Arun_Saravanan_Balac@dell.com
Description of PR
Update platform API test cases to make use of capabilities fields in platform.json
HLD: sonic-net/SONiC#768
Test cases modified:
Type of change
Back port request
Approach
What is the motivation for this PR?
To use different values/skip test cases on a given platform based on the capabilities provided in platform.json
How did you do it?
Use chassis facts available in duthosts to get the required parameters for the test case.
How did you verify/test it?
Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation