#594 FIX by TikhomirovSergey · Pull Request #597 · appium/java-client

TikhomirovSergey · Mar 10, 2017

Change list

#594 FIX:

update to selenium 3.3.1
AppiumCommandExecutor was reimplemented. Now it implements CommandExecutor and doesn't extends HttpCommandExecutor. However the most part of implementation was taken from Selenium.
constructor changes.

Types of changes

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

Some changes may be considered as breaking changes.

AppiumProtocolHandShake was added. As it 99.9% repeats the old working version so the real authorship is used for this commit.

UPDATE to Selenium 3.3.0

- AppiumCommandInfo was added. - constructors of AndroidDriver were changed.

jsf-clabot · Mar 10, 2017

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ TikhomirovSergey
❌ shs96c

TikhomirovSergey · Mar 10, 2017

@mykola-mokhnach @saikrishna321 @KazuCocoa
You are invited too.

mykola-mokhnach · Mar 11, 2017

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

+import org.openqa.selenium.remote.http.HttpMethod;
+
+public class AppiumCommandInfo extends CommandInfo {
+    private final String url;


wouldn't it be more useful to keep url as an URL object instead of just a string?

@mykola-mokhnach
No. Take a look at the
https://github.com/appium/java-client/blob/master/src/main/java/io/appium/java_client/MobileCommand.java#L127

and

https://github.com/appium/java-client/blob/master/src/main/java/io/appium/java_client/MobileCommand.java#L197

I think that an URL like that can't be created by Java. It throws MalformedException at that case.

mykola-mokhnach · Mar 11, 2017

src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java

+            AppiumProtocolHandShake.Result result = handshake.createSession(client, command);
+            Dialect dialect = result.getDialect();
+            commandCodec = dialect.getCommandCodec();
+            for (Map.Entry<String, AppiumCommandInfo> entry : additionalCommands.entrySet()) {


can be simplified as

additionalCommands.forEach((key, value) -> { checkNotNull(key); checkNotNull(value); commandCodec.defineCommand(key, value.getMethod(), value.getUrl()); });

mykola-mokhnach · Mar 11, 2017

src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java

+
+            Response response = responseCodec.decode(httpResponse);
+            if (response.getSessionId() == null) {
+                if (httpResponse.getTargetHost() != null) {


do we assume, that httpResponse and response can never be equal to null?

mykola-mokhnach · Mar 11, 2017

src/main/java/io/appium/java_client/remote/AppiumProtocolHandShake.java

+        Optional<Result> result = createSession(client, parameters);
+
+        // Assume a fragile OSS webdriver implementation
+        if (!result.isPresent()) {


this all can be chained into orElseGet.orElseGet.orElseThrow

mykola-mokhnach · Mar 11, 2017

src/main/java/io/appium/java_client/remote/AppiumProtocolHandShake.java

+        String req = new BeanToJsonConverter().convert(required);
+
+        // Assume the remote end obeys the robustness principle.
+        StringBuilder parameters = new StringBuilder("{");


I'm thinking about using JSONObject here and JSONObject.toString() instead of StringBuilder, which can be potentially not so secure and stable.

TikhomirovSergey · Mar 11, 2017

@mykola-mokhnach yes. Thank you. I will fix it asap.

SrinivasanTarget · Mar 11, 2017

@TikhomirovSergey Works like a charm with all drivers 👍
Have a query, Our request looks like this now,

[
  {
    "app": "/Users/ssekar/workspace/java-client/src/test/java/io/appium/java_client/TestApp.app.zip",
    "platformVersion": "10.1",
    "automationName": "XCuiTest",
    "browserName": "",
    "platformName": "iOS",
    "deviceName": "iPhone 6",
    "launchTimeout": 500000
  },
  {
    
  },
  {
    "desiredCapabilities": {
      "app": "/Users/ssekar/workspace/java-client/src/test/java/io/appium/java_client/TestApp.app.zip",
      "platformVersion": "10.1",
      "automationName": "XCuiTest",
      "browserName": "",
      "platformName": "iOS",
      "deviceName": "iPhone 6",
      "launchTimeout": 500000
    },
    "requiredCapabilities": {
      
    }
  },
  null,
  null
]

But i didn't get the empty curly brackets in between comparing with #594 (comment).

TikhomirovSergey · Mar 16, 2017

@SrinivasanTarget @mykola-mokhnach
Some issues were fixed. I will commit them when possible. I am very busy on this week. Sorry for the delaying.

mykola-mokhnach · Mar 19, 2017

src/main/java/io/appium/java_client/remote/AppiumProtocolHandShake.java

+            throws IOException {
+
+        Capabilities desired = (Capabilities) command.getParameters().get("desiredCapabilities");
+        desired = desired == null ? new DesiredCapabilities() : desired;


I'm not 100% sure, but it would be better to use if condition here rather than ternary operator, because of http://stackoverflow.com/questions/12643973/java-calling-a-method-using-a-ternary-operator

This means the code will anyway create an extra DesiredCapabilities instance even if desired != null

Why not java Optional?

with Optional this might look like

Capabilities desired = Optional.ofNullable((Capabilities) command.getParameters().get("desiredCapabilities")) .orElseGet(DesiredCapabilities::new);

mykola-mokhnach · Mar 19, 2017

src/main/java/io/appium/java_client/remote/AppiumProtocolHandShake.java

+            result = createSession(client, jsonObject);
+        }
+
+        if (result.isPresent()) {


return result.orElseThrow(() -> new SessionNotCreatedException( String.format("Unable to create new remote session. desired capabilities = %s, required capabilities = %s", desired,required) ));

mykola-mokhnach · Mar 19, 2017

src/main/java/io/appium/java_client/remote/AppiumProtocolHandShake.java

+            // Fine. Handle that below
+        }
+
+        if (jsonBlob == null) {


wouldn't be simpler to assign jsonBlob to HashMap upon definition?

mykola-mokhnach · Mar 19, 2017

src/main/java/io/appium/java_client/remote/AppiumProtocolHandShake.java

+        required = required == null ? new DesiredCapabilities() : required;
+
+        JsonParser parser = new JsonParser();
+        JsonElement des = parser.parse(new BeanToJsonConverter().convert(desired));


TikhomirovSergey · Mar 19, 2017

@mykola-mokhnach Ok. Thank you. Will try to work it out toaday.

...also browser test improvement

TikhomirovSergey · Mar 19, 2017

@mykola-mokhnach @SrinivasanTarget Could you take a look at new changes ane more time?

mykola-mokhnach · Mar 19, 2017

src/main/java/io/appium/java_client/remote/AppiumProtocolHandShake.java

+            throws IOException, WebDriverException {
+
+        Capabilities desired = ofNullable((Capabilities) command.getParameters().get("desiredCapabilities"))
+                .orElse(new DesiredCapabilities());


this will also create an extra instance of DesiredCapabilities. One must use orElseGet here to avoid this (orElse is good for constants only).

hi please did you know when the new jar will be available?

@swimoAmin It will be available soon in couple of days.

TikhomirovSergey · Mar 20, 2017

@SrinivasanTarget @mykola-mokhnach There is one more commit. I think it is finally completed

mykola-mokhnach

LGTM

SrinivasanTarget · Mar 21, 2017

LGTM but request looks same with few null params and empty curly brackets. Any idea?

@TikhomirovSergey ping

SrinivasanTarget

LGTM

ghost · Mar 28, 2017

Is this fix included in 5.0.0-BETA6? I am still getting this error using that version of the Java Client with Appium 1.6.4-beta:

[debug] [MJSONWP] Bad parameters: BadParametersError: Parameters were incorrect. We wanted {"required":["desiredCapabilities"],"optional":["requiredCapabilities","capabilities","sessionId","id"]} and you sent ["desiredCapabilities","requiredCapabilities","capabilities","alwaysMatch","firstMatch"]

SrinivasanTarget · Mar 28, 2017

@szaluk Yes changes are there and works fine for me. Can you please log an issue with detailed description, code snippet and logs so that we can investigate further.

ghost · Mar 28, 2017

@SrinivasanTarget - I just created appium/appium#8118

Thanks,
Steve

shs96c and others added 5 commits March 10, 2017 00:01

#594 FIX:

84b5dbd

AppiumProtocolHandShake was added. As it 99.9% repeats the old working version so the real authorship is used for this commit.

#594 FIX:

b0ca0d9

UPDATE to Selenium 3.3.0

#594 FIX: AppiumCommandExecutor was reimplemented.

aaeee64

#594 FIX: The final commit

4d062a5

- AppiumCommandInfo was added. - constructors of AndroidDriver were changed.

checkstyle issue fixes

2be5498

TikhomirovSergey requested review from SrinivasanTarget and jlipps March 10, 2017 23:22

TikhomirovSergey self-assigned this Mar 10, 2017

TikhomirovSergey added the bug label Mar 10, 2017

TikhomirovSergey added this to the 5.0.0 milestone Mar 10, 2017

mykola-mokhnach reviewed Mar 11, 2017

View reviewed changes

KazuCocoa mentioned this pull request Mar 11, 2017

new webdriver createSession for W3C appium/ruby_lib#501

Closed

This was referenced Mar 13, 2017

BadParametersError with Selenium 3 appium/appium#8030

Closed

Ios Simulator not getting launched appium/appium#8034

Closed

This was referenced Mar 14, 2017

Bad parameters #602

Closed

ESOCKETTIMEDOUT Error #600

Closed

SrinivasanTarget mentioned this pull request Mar 17, 2017

Unable to create a new Session after upgrading to java-client 5.0.0-BETA5 #605

Closed

Work on remark.

0e6c04c

mykola-mokhnach reviewed Mar 19, 2017

View reviewed changes

AppiumProtocolHandShake code style improving (usage of Optional)

02374da

...also browser test improvement

mykola-mokhnach reviewed Mar 19, 2017

View reviewed changes

Build issue fix.

b8bc1f0

mykola-mokhnach approved these changes Mar 20, 2017

View reviewed changes

TikhomirovSergey mentioned this pull request Mar 21, 2017

org.openqa.selenium.remote.ProtocolHandshake createSession messages logging as errors #603

Closed

SrinivasanTarget approved these changes Mar 21, 2017

View reviewed changes

TikhomirovSergey merged commit 065d1a9 into appium:master Mar 21, 2017

saikrishna321 mentioned this pull request Mar 22, 2017

PageFactory.initElements throwing java.lang.NoSuchMethodError: com.google.common.base.Preconditions.checkArgument(ZLjava/lang/String;J)V #588

Closed

mykola-mokhnach mentioned this pull request Apr 4, 2017

BadParametersError while trying to connect to Appium server appium/python-client#158

Closed

sivasks19 mentioned this pull request Apr 11, 2017

Bad Parameters Error using Java-client 5.0.0-BETA6 and Appium 1.6.4 while creating AndroidDriver instance. appium/appium#8227

Closed

TikhomirovSergey mentioned this pull request Jun 19, 2017

Java-client will throw exceptions - GridException: Cannot extract a capabilities xxx with webdriver 3.x #655

Closed

This was referenced Jul 28, 2017

registering appium node with selenium server stopped working appium/appium#8916

Closed

#655 FIX #682

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

TikhomirovSergey commented Mar 10, 2017 • edited by SrinivasanTarget Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change list

Types of changes

Details

Uh oh!

jsf-clabot commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TikhomirovSergey commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey commented Mar 11, 2017

Uh oh!

SrinivasanTarget commented Mar 11, 2017

Uh oh!

TikhomirovSergey commented Mar 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey commented Mar 19, 2017

Uh oh!

TikhomirovSergey commented Mar 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TikhomirovSergey commented Mar 20, 2017

Uh oh!

mykola-mokhnach left a comment

Choose a reason for hiding this comment

Uh oh!

SrinivasanTarget commented Mar 21, 2017

Uh oh!

SrinivasanTarget left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 28, 2017

Uh oh!

SrinivasanTarget commented Mar 28, 2017

Uh oh!

ghost commented Mar 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

TikhomirovSergey commented Mar 10, 2017 •

edited by SrinivasanTarget

Loading

jsf-clabot commented Mar 10, 2017 •

edited

Loading