-
Notifications
You must be signed in to change notification settings - Fork 44
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
ledctl: add error message for missing devices #216
Conversation
Fixes #213 |
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.
One comment and we need to test this and "not supported" errors beacuse they are most common negative scenarios for ledctl so:
- Please add test for this error.
- Please also add test for error in case if device is not supported.
Ledctl prints error message for not supported devices, but there is no such message for not existent or missing devices. Add error message for missing devices for consistency. Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
b58ba4e
to
9bb7923
Compare
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.
Few new test related comments.
The change looks good, just small adjustments.
Add universal_newlines=True and shell=True for subprocess.run() call in run_led_cmd(). With universal_newlines decoding to utf-8 is not needed. Without shell param returncodes and some output lines are omitted. Change run_ledctl_cmd_decode() to run_ledctl_cmd_valid(). It returns stdout only, raise exception if non zero status is returned. Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
Add tests for checking output for not existing and unsupported devices. Add run_ledctl_cmd_not_valid() function for checking negative scenarios. Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
9bb7923
to
342a1ea
Compare
Ledctl prints error message for not supported devices, but there is no such message for not existent or missing devices.
Add error message for missing devices for consistency.