#594 FIX #597

TikhomirovSergey · 2017-03-10T23:21:42Z

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 · 2017-03-10T23:21:43Z

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 · 2017-03-10T23:23:55Z

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

mykola-mokhnach · 2017-03-11T07:51:34Z

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 · 2017-03-11T07:59:49Z

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 · 2017-03-11T08:01:06Z

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 · 2017-03-11T08:05:28Z

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 · 2017-03-11T08:13:34Z

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 · 2017-03-11T09:48:25Z

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

SrinivasanTarget · 2017-03-11T11:19:37Z

@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 · 2017-03-16T20:45:23Z

@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 · 2017-03-19T08:16:40Z

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 · 2017-03-19T08:20:25Z

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 · 2017-03-19T08:22:15Z

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 · 2017-03-19T08:48:10Z

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 · 2017-03-19T10:13:19Z

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

...also browser test improvement

TikhomirovSergey · 2017-03-19T20:51:53Z

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

mykola-mokhnach · 2017-03-19T21:27:54Z

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 · 2017-03-20T21:00:06Z

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

mykola-mokhnach

LGTM

SrinivasanTarget · 2017-03-21T08:53:38Z

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

@TikhomirovSergey ping

SrinivasanTarget

LGTM

szaluk · 2017-03-28T17:25:42Z

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 · 2017-03-28T17:43:28Z

@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.

szaluk · 2017-03-28T18:28:08Z

@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 jlipps and SrinivasanTarget 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#594 FIX #597

#594 FIX #597

TikhomirovSergey commented Mar 10, 2017 •

edited by SrinivasanTarget

Loading

jsf-clabot commented Mar 10, 2017 •

edited

Loading

TikhomirovSergey commented Mar 10, 2017

mykola-mokhnach Mar 11, 2017

TikhomirovSergey Mar 11, 2017

mykola-mokhnach Mar 11, 2017

SrinivasanTarget Mar 11, 2017

mykola-mokhnach Mar 11, 2017

mykola-mokhnach Mar 11, 2017

mykola-mokhnach Mar 11, 2017

TikhomirovSergey commented Mar 11, 2017

SrinivasanTarget commented Mar 11, 2017

TikhomirovSergey commented Mar 16, 2017

mykola-mokhnach Mar 19, 2017

SrinivasanTarget Mar 19, 2017

mykola-mokhnach Mar 19, 2017

mykola-mokhnach Mar 19, 2017

mykola-mokhnach Mar 19, 2017

mykola-mokhnach Mar 19, 2017

TikhomirovSergey commented Mar 19, 2017

TikhomirovSergey commented Mar 19, 2017

mykola-mokhnach Mar 19, 2017

swimoAmin Mar 20, 2017

SrinivasanTarget Mar 21, 2017

TikhomirovSergey commented Mar 20, 2017

mykola-mokhnach left a comment

SrinivasanTarget commented Mar 21, 2017

SrinivasanTarget left a comment

szaluk commented Mar 28, 2017

SrinivasanTarget commented Mar 28, 2017

szaluk commented Mar 28, 2017

#594 FIX #597

#594 FIX #597

Conversation

TikhomirovSergey commented Mar 10, 2017 • edited by SrinivasanTarget Loading

Change list

Types of changes

Details

jsf-clabot commented Mar 10, 2017 • edited Loading

TikhomirovSergey commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TikhomirovSergey commented Mar 11, 2017

SrinivasanTarget commented Mar 11, 2017

TikhomirovSergey commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TikhomirovSergey commented Mar 19, 2017

TikhomirovSergey commented Mar 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TikhomirovSergey commented Mar 20, 2017

mykola-mokhnach left a comment

Choose a reason for hiding this comment

SrinivasanTarget commented Mar 21, 2017

SrinivasanTarget left a comment

Choose a reason for hiding this comment

szaluk commented Mar 28, 2017

SrinivasanTarget commented Mar 28, 2017

szaluk commented Mar 28, 2017

TikhomirovSergey commented Mar 10, 2017 •

edited by SrinivasanTarget

Loading

jsf-clabot commented Mar 10, 2017 •

edited

Loading