Skip to content
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

add apis to read the performance data #562

Merged
merged 9 commits into from
Feb 25, 2017

Conversation

heeseon
Copy link
Contributor

@heeseon heeseon commented Jan 29, 2017

Change list

add apis to read the resource usage information of the application as like cpu, memory, network traffic, and battery

Types of changes

What types of changes are you proposing/introducing to Java client?
Put an x in the boxes that apply

  • No changes in production code.
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

The appium already has a way to gather the performance data by log.
but If the resulting data format is set to a log file, the user should conduct another parsing to show the data on the screen.
also I think user wants to decide the moment to read the data and associate the performance data and other information.
so I’d like to add apis to gather the performance data.

please refer below the commit code


List<String> supportedPerformanceDataTypes = driver.getSupportedPerformanceDataTypes();

for(int i = 0 ; i < supportedPerformanceDataTypes.size() ; ++ i){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not to use for .. in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i applied it.
but I think there is no reason to check the value of the table, because the values could be changed in appium-android-driver.
I have a plan to add some other values for example, heaphistogram....


int valueTableHeadLength = valueTable.get(0).size();

for(int j = 1 ; j < valueTable.size() ; ++ j){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. Use subList property to get a slice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about the difference between List.get(i) and List.subList(i,i).
but I applied it.

and one more,
in case of this method,
there is no reason to check the head value of the returned table, because the implementation is dependent for the platform that means the head value could be changed.

driver.startActivity("io.appium.android.apis", ".ApiDemos");

List<String> supportedPerformanceDataTypes = driver.getSupportedPerformanceDataTypes();
assert(supportedPerformanceDataTypes.size() == 4);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be more readable to use assertEquals instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@@ -26,6 +26,7 @@
import org.openqa.selenium.internal.HasIdentity;

import java.util.AbstractMap;
import java.util.List;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applied

@@ -140,4 +141,33 @@
assertNotNull(driver.getDisplayDensity());
assertNotEquals(0, driver.getSystemBars().size());
}

@Test public void getSupportedPerformanceDataTypesTest() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I used assertEquals

Copy link
Contributor

@TikhomirovSergey TikhomirovSergey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heeseon
It looks ok but I would like to make the same the following way

public interface HasSupportedPerformanceDataType extends ExecutesMethod {
    default List<String> getSupportedPerformanceDataTypes() {
         return CommandExecutionHelper.execute(this, getSupportedPerformanceDataTypesCommand());
    }

    default List<List<Object>> getPerformanceData(String packageName, String dataType, int dataReadTimeout) {
        return CommandExecutionHelper.execute(this,
                                                getPerformanceDataCommand(packageName, dataType, dataReadTimeout));
    }
}

and then you just need to declare

public class AndroidDriver<T extends WebElement>
    extends AppiumDriver<T>
    implements PressesKeyCode, HasNetworkConnection, PushesFiles, StartsActivity,
        FindsByAndroidUIAutomator<T>, LocksAndroidDevice, HasSettings, HasDeviceDetails, HasSupportedPerformanceDataType {
...
}

@heeseon
Copy link
Contributor Author

heeseon commented Feb 2, 2017

fixed all

@@ -153,6 +157,52 @@ public AndroidDriver(Capabilities desiredCapabilities) {
}

/**
* returns the information type of the system state which is supported to read
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @heeseon
Everything is ok but... could you remove the code below and move java doc to the HasSupportedPerformanceDataType? It is not necessary to duplicate code of API implemented by default. I think there is no specific details of the AndroidDriver behaviour.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heeseon ping

@heeseon
Copy link
Contributor Author

heeseon commented Feb 24, 2017

I deleted the duplicated code. Please check it.


/**
* Created by hwangheeseon on 2017. 2. 2..
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heeseon Can you remove these statements please?

@heeseon
Copy link
Contributor Author

heeseon commented Feb 25, 2017

Ok

@TikhomirovSergey
Copy link
Contributor

I have approved these changes. @SrinivasanTarget ?

@SrinivasanTarget SrinivasanTarget merged commit 92082bf into appium:master Feb 25, 2017
@SrinivasanTarget
Copy link
Member

Thanks @heeseon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants