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.

Sign up for GitHub

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

Jump to bottom

#732 FIX #733

Merged
merged 3 commits into from
Sep 27, 2017
Merged

#732 FIX #733

merged 3 commits into from
Sep 27, 2017

Conversation

TikhomirovSergey
Copy link
Contributor

@TikhomirovSergey TikhomirovSergey commented Sep 23, 2017 •
edited

Change list

#732 FIX

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 TikhomirovSergey self-assigned this Sep 23, 2017
@TikhomirovSergey
Copy link
Contributor Author

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

mykola-mokhnach
mykola-mokhnach reviewed Sep 23, 2017
View reviewed changes
src/main/java/io/appium/java_client/HasSessionDetails.java Outdated
}

default Object getSessionDetail(String detail) {
return getSessionDetails().get(detail);
}

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

Choose a reason for hiding this comment

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

I'd also mark these with Nullable annotation

Copy link
Contributor

Choose a reason for hiding this comment

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

same comment for the stuff below

mykola-mokhnach
mykola-mokhnach reviewed Sep 23, 2017
View reviewed changes
src/main/java/io/appium/java_client/HasSessionDetails.java Outdated
boolean isBrowser();
default boolean isBrowser() {
return ofNullable(getSessionDetail("browserName"))
.map(browserName -> browserName.toString())
Copy link
Contributor

Choose a reason for hiding this comment

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

can be String::valueOf

mykola-mokhnach
mykola-mokhnach reviewed Sep 23, 2017
View reviewed changes
src/main/java/io/appium/java_client/HasSessionDetails.java Outdated
@@ -34,26 +35,46 @@
@SuppressWarnings("unchecked")
default Map<String, Object> getSessionDetails() {
Response response = execute(GET_SESSION);
Map<String, Object> resultMap = Map.class.cast(response.getValue());

return ImmutableMap.<String, Object>builder()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

mykola-mokhnach
mykola-mokhnach reviewed Sep 23, 2017
View reviewed changes
src/main/java/io/appium/java_client/AppiumDriver.java Outdated
@Override public boolean isBrowser() {
return !getContext().toLowerCase().contains("NATIVE_APP".toLowerCase());
return super.isBrowser()
&& !getContext().toLowerCase().contains("NATIVE_APP".toLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

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

Apache's StringUtils has StringUtils.containsIgnoreCase method

@mykola-mokhnach
Copy link
Contributor

Overall good, just several minor things

@TikhomirovSergey
Copy link
Contributor Author

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

@appium appium deleted a comment Sep 23, 2017
mykola-mokhnach
mykola-mokhnach reviewed Sep 24, 2017
View reviewed changes
src/main/java/io/appium/java_client/HasSessionDetails.java
@@ -34,26 +36,47 @@
@SuppressWarnings("unchecked")
default Map<String, Object> getSessionDetails() {
Response response = execute(GET_SESSION);
Map<String, Object> resultMap = Map.class.cast(response.getValue());

//this filtering was added to clear returned result.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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")

Copy link
Contributor

Choose a reason for hiding this comment

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

of course, just mention the same thing in the comment

SrinivasanTarget
SrinivasanTarget approved these changes Sep 24, 2017
View reviewed changes
Copy link
Member

@SrinivasanTarget SrinivasanTarget left a comment

Choose a reason for hiding this comment

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

LGTM

@TikhomirovSergey
Copy link
Contributor Author

@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
Copy link
Member

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

@SrinivasanTarget
Copy link
Member

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
Copy link

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
Copy link
Member

@welnanick You can try this,

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

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

@welnanick
Copy link

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

@SrinivasanTarget
Copy link
Member

Thanks @welnanick

@welnanick
Copy link

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
SrinivasanTarget approved these changes Sep 26, 2017
View reviewed changes
Copy link
Member

@SrinivasanTarget SrinivasanTarget left a comment

Choose a reason for hiding this comment

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

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

@TikhomirovSergey TikhomirovSergey merged commit 983c176 into appium:master Sep 27, 2017
@FilipWennerdahlSopraSteria FilipWennerdahlSopraSteria mentioned this pull request Jun 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mykola-mokhnach mykola-mokhnach mykola-mokhnach approved these changes

@SrinivasanTarget SrinivasanTarget SrinivasanTarget approved these changes

Assignees

@TikhomirovSergey TikhomirovSergey

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@TikhomirovSergey @mykola-mokhnach @saikrishna321 @SrinivasanTarget @welnanick