#732 FIX by TikhomirovSergey · Pull Request #733 · appium/java-client

TikhomirovSergey · 2017-09-23T10:33:49Z

Change list

Types of changes

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

Also I found out that logic of Selenium Capabilities has changed since Jan'2017 and capabilities.getCapability("platformName") is not the same as the platform name that user gives to start Android/iOS/Windows session.

@SrinivasanTarget Could you run iOS tests with this change?
I am looking forward to improve tests to make them run on Sauce env (will be the next PR).

TikhomirovSergey · 2017-09-23T10:35:46Z

@welnanick you can can comment the PR if you have something to say about this :)

mykola-mokhnach · 2017-09-23T16:33:01Z

src/main/java/io/appium/java_client/HasSessionDetails.java

+    /**
+     * @return name of the current mobile platform.
+     */
    default String getPlatformName() {


I'd also mark these with Nullable annotation

same comment for the stuff below

mykola-mokhnach · 2017-09-23T16:34:40Z

src/main/java/io/appium/java_client/HasSessionDetails.java

-    boolean isBrowser();
+    default boolean isBrowser() {
+        return ofNullable(getSessionDetail("browserName"))
+                .map(browserName -> browserName.toString())


can be String::valueOf

mykola-mokhnach · 2017-09-23T16:35:52Z

src/main/java/io/appium/java_client/HasSessionDetails.java

        Response response = execute(GET_SESSION);
+        Map<String, Object> resultMap = Map.class.cast(response.getValue());
+
        return  ImmutableMap.<String, Object>builder()


It might be helpful to have a comment here about why additional filtering is necessary

mykola-mokhnach · 2017-09-23T16:36:59Z

src/main/java/io/appium/java_client/AppiumDriver.java

    @Override public boolean isBrowser() {
-        return !getContext().toLowerCase().contains("NATIVE_APP".toLowerCase());
+        return super.isBrowser()
+                && !getContext().toLowerCase().contains("NATIVE_APP".toLowerCase());


Apache's StringUtils has StringUtils.containsIgnoreCase method

mykola-mokhnach · 2017-09-23T16:39:00Z

Overall good, just several minor things

TikhomirovSergey · 2017-09-23T21:07:17Z

@mykola-mokhnach I made improvements.
@SrinivasanTarget I am waiting for your review and results of the test running.

mykola-mokhnach · 2017-09-24T05:59:23Z

src/main/java/io/appium/java_client/HasSessionDetails.java

        Response response = execute(GET_SESSION);
+        Map<String, Object> resultMap = Map.class.cast(response.getValue());
+
+        //this filtering was added to clear returned result.


-> was added to avoid unexpected NPE on putAll call if the response map contains null value(s)

@mykola-mokhnach Not only for this purpose. I think empty strings is not useful data and it increases complexity of further operations. So let there will be something useful or null-value when user tries

driver.getSessionDetail("someDetail")

of course, just mention the same thing in the comment

SrinivasanTarget

LGTM

TikhomirovSergey · 2017-09-24T11:59:34Z

@SrinivasanTarget have you run iOS test in legacy and xCUIT mode? What was the result? Could you try to run some browser/webview script in xCUIT mode and check class of elements that are found? What the result?

saikrishna321 · 2017-09-25T00:27:19Z

Srini, was on vacation. if he is not back today ... I can help run tests on iOS and close it today ....

SrinivasanTarget · 2017-09-25T05:08:07Z

Yup i did tested on XCUIT mode including native and webviews but i dont have old xcode to test in legacy mode. Everything looks perfectly fine in XCUIT mode. Will download xcode 7.0.1 and give a try today. Code looks good to me as well. Will keep you posted about legacy mode (including native and webviews)

welnanick · 2017-09-25T17:29:27Z

Is there a way I can build this locally to verify my issue is fixed? Do I just clone the repo and run a gradle build?

SrinivasanTarget · 2017-09-25T17:38:29Z

@welnanick You can try this,

allprojects {
		repositories {
			...
			maven { url 'https://jitpack.io' }
		}
	}

dependencies {
	        compile 'com.github.TikhomirovSergey:java-client:d2e5ff1a47'
	}

welnanick · 2017-09-25T17:39:40Z

@SrinivasanTarget Alright, once I get a chance here in a few minutes I'll try that and let you all know the results.

SrinivasanTarget · 2017-09-25T17:40:00Z

Thanks @welnanick

welnanick · 2017-09-25T18:18:07Z

What you suggested to try to get the build didn't work, I get this error through gradle:

* What went wrong:
Could not resolve all files for configuration ':testCompileClasspath'.
> Could not find com.github.TikhomirovSergey:java-client:d2e5ff1a47.
  Searched in the following locations:
      https://jcenter.bintray.com/com/github/TikhomirovSergey/java-client/d2e5ff1a47/java-client-d2e5ff1a47.pom
      https://jcenter.bintray.com/com/github/TikhomirovSergey/java-client/d2e5ff1a47/java-client-d2e5ff1a47.jar
      https://jitpack.io/com/github/TikhomirovSergey/java-client/d2e5ff1a47/java-client-d2e5ff1a47.pom
      https://jitpack.io/com/github/TikhomirovSergey/java-client/d2e5ff1a47/java-client-d2e5ff1a47.jar
  Required by:
      project :

SrinivasanTarget

Tested, everything looks fine on both legacy and XCUIT modes.

TikhomirovSergey added 2 commits September 23, 2017 01:57

#732 FIX

5fa74ae

code style issues were got fixed

d2e5ff1

TikhomirovSergey self-assigned this Sep 23, 2017

TikhomirovSergey requested review from SrinivasanTarget and mykola-mokhnach September 23, 2017 10:33

TikhomirovSergey added the bug label Sep 23, 2017

mykola-mokhnach reviewed Sep 23, 2017

View reviewed changes

some minor things that were found by reviewers were improved

89b22dd

appium deleted a comment Sep 23, 2017

mykola-mokhnach reviewed Sep 24, 2017

View reviewed changes

mykola-mokhnach approved these changes Sep 24, 2017

View reviewed changes

SrinivasanTarget approved these changes Sep 24, 2017

View reviewed changes

SrinivasanTarget approved these changes Sep 26, 2017

View reviewed changes

TikhomirovSergey merged commit 983c176 into appium:master Sep 27, 2017

FilipWennerdahlSopraSteria mentioned this pull request Jun 12, 2018

5.4.0 and forward causes excessive getSession() calls #937

Closed

Uh oh!

Conversation

TikhomirovSergey commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change list

Types of changes

Details

Uh oh!

TikhomirovSergey commented Sep 23, 2017

Uh oh!

mykola-mokhnach Sep 23, 2017

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Sep 23, 2017

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Sep 23, 2017

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Sep 23, 2017

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Sep 23, 2017

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach commented Sep 23, 2017

Uh oh!

TikhomirovSergey commented Sep 23, 2017

Uh oh!

mykola-mokhnach Sep 24, 2017

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Sep 24, 2017

Choose a reason for hiding this comment

Uh oh!

SrinivasanTarget left a comment

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey commented Sep 24, 2017

Uh oh!

saikrishna321 commented Sep 25, 2017

Uh oh!

SrinivasanTarget commented Sep 25, 2017

Uh oh!

welnanick commented Sep 25, 2017

Uh oh!

SrinivasanTarget commented Sep 25, 2017

Uh oh!

welnanick commented Sep 25, 2017

Uh oh!

SrinivasanTarget commented Sep 25, 2017

Uh oh!

welnanick commented Sep 25, 2017

Uh oh!

SrinivasanTarget left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TikhomirovSergey commented Sep 23, 2017 •

edited

Loading

TikhomirovSergey Sep 24, 2017 •

edited

Loading