-
Notifications
You must be signed in to change notification settings - Fork 47
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
removed segmentation faults, corrected commands and output need to fix o... #513
base: master
Are you sure you want to change the base?
removed segmentation faults, corrected commands and output need to fix o... #513
Conversation
…x only for battery jira issue: WP-829
Could you add some test cases for this module? DeviceStatus has caused problems in the past, so it would be useful to know that these issues were fixed. E.g., something that calls the native module and just checks for a valid result. I see there is a test already - you could make sure this is working, convert it to Jasmine, make sure that is covers the previous issue, and then add it to the test script at https://github.com/webinos/Webinos-Platform/blob/master/tools/travis/unit-tests.sh . If you need help, please let me know. |
Ok, will check with the tests. The problem with Device status Linux earlier was - the output when compared to a Virtual machine is different from the normal machines and other kernels of Linux(Ubuntu), some times the user might not have installed required system files that might be different from one machine to another for the Commands specified in the native code. This pull request consists of common packages used, If the required output is not shown then it returns an exception not able to retrieve it. Though the output is different from the specifications that's the most common output obtained. Will also Document the methods used that would be useful along with the tests. for Battery will push it soon. But Devicestatus seems to be stable enough for Linux now... |
Let me know if you find some errors will fix it |
Hi Krishna, I appreciate the issue with testing being that you can't predict the results. However, I'm sure you can predict things like a result being non-null, or within a certain range, or simply getting some kind of response (an error or otherwise). A simple test that invokes the device status code on a range of inputs would be something, just to show that no Segfault occurs. This will help if anyone else comes to modify your code in the future, or if our build environment changes. Assuming I have time, I will test and merge the pull request when it has at least some basic test cases for Linux that are invoked automatically in the Travis unit test script. Ideally tests work on Windows and Android, too. |
I fully agree with John. Even with unpredictable results, you can still write tests. If the tests fail when they shouldn't, we adapt the tests. The main goal IMO is to make sure we don't have regression later on and the API's are stable. BTW: this is an api for developers, if the output is completely unpredictable, we need to look at how we'll solve this. As a developer having an unreliable API is a complete nightmare. |
@@ -46,21 +46,21 @@ string Device::getPropertyValue(string * property, string * component) | |||
|
|||
string Device::imei(string deviceName) | |||
{ | |||
return "imei"; | |||
return "Can't Retrieve from OS"; |
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.
AFAIK, the API should respond false if the property is not supported. I have opened the jira issue http://jira.webinos.org/browse/WP-916
If we are calling OS functions instead of using c++ libraries, then why isn't this module a javascript one... I thought that the purpose of making this c++ is to make it faster and be os configuration independent (not rely on the user having the tools installed). In windows, this is true, but I see in Linux that the information is gathered from command line tools instead of using the libraries. |
segmentation fault removed, commands and outputs corrected...nned to only change for battery
jira issue: WP-829