-
Notifications
You must be signed in to change notification settings - Fork 37
Modified configuration in lf_base_interop_profile.py , py-json/interop_connectivity.py and Added robo in Port reset test #219
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
base: master
Are you sure you want to change the base?
Conversation
…hanges according to 5.5.2 build. Signed-off-by: Sidartha-CT <neelapu.sidartha@candelatech.com>
Signed-off-by: Sidartha-CT <neelapu.sidartha@candelatech.com>
…elper. Signed-off-by: Sidartha-CT <neelapu.sidartha@candelatech.com>
…_report. -For the purpose of robo heatmap ,moved dewebgui code to above the generate_report. Signed-off-by: Sidartha-CT <neelapu.sidartha@candelatech.com>
memnochproxy
left a comment
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.
I would prefer you do this with f-strings, eg:
f'--es ssid "{cur_ssid}" '
Is there a reason you are using \" inside a single quote? Do the backslashes fix something that resulting command?
memnochproxy
left a comment
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.
Please do not mix multiple string formatting methods. You have some " %s "%foo strings in there. Let's standardise on f-strings, please. You also have some "{}".format() calls, change those to f-strings too.
memnochproxy
left a comment
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.
Please address inline structural comments.
I think the lines in this file are getting rather long, too.
PEP8 requires line length of 79 chars.
| if not self.robot_test: | ||
| reset_dict, test_duration = self.performing_resets(test_start_time=test_start_time) | ||
| return reset_dict, test_duration | ||
| else: |
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.
This else is an unnecessary level of indentation. Remove the else and go up one level of indentation.
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.
Yes, Jed. I initially added the else for readability, but you’re right it creates an unnecessary level of indentation since the if statement already returns. I’ve removed the else, added a comment for clarity, and made the changes in the same commit via rebasing.
|
|
||
| # creating the dataset | ||
| self.graph_image_name = "overall_graph" | ||
| self.graph_image_name = "overall_graph{}".format(graph_suffix) |
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.
use fstrings
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.
Sure Jed, I’ve replaced them with f-strings and will follow this as the standard style going forward.
| coord_key = self.coordinate_list[coordinate] | ||
| angle_key = self.rotation_list[angle] | ||
|
|
||
| if ( |
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.
you can rewrite this as a series of guard clauses to put your continue statement at the top of the loop, not in an else.
if (coord_key not in self.port_reset_data
or angle_key not in slef.port_reset_data[coord_key]
or .....):
continue
reset_dict = ...
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.
Sure Jed, I've added those specific changes, will improve and follow this same pattern when it's required.
| else: | ||
| coord_key = self.coordinate_list[coordinate] | ||
|
|
||
| if ( |
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.
you can rewrite this as a series of guard clauses to put your continue statement at the top of the loop, not in an else.
if (coord_key not in ...
or ...
or .....):
continue
reset_dict = ...
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.
Sure Jed, I've added those specific changes. Thank you for reviewing the code.
…test Verified CLI : python3 lf_interop_port_reset_test.py --host 192.168.207.78 --mgr_ip eth1 --dut AP --ssid "NETGEAR_2G_wpa2" --encryp psk2 --passwd Password@123 --reset 2 --time_int 5 --robot_test --coordinate 4,3 --robot_ip 192.168.200.169 --device_list ubuntu24 Signed-off-by: Sidartha-CT <neelapu.sidartha@candelatech.com>
Hi Jed (@memnochproxy ) , To maintain consistency, we have implemented the same handling in our scripts that include Android configurations. Please let me know if you have any concerns or if further changes are required. |
Yes, Jed (@memnochproxy ). The inline comments were added by previous developers during the initial development of this script. To add robot-specific logic for the port reset with bit priority, I moved that block into a separate helper so it can be reused. That said, we will clean up the inline structural comments and ensure the file adheres to PEP8 standards, including the 79-character line length, in the future commits. |
6863627 to
9f4c7ec
Compare
No description provided.