Providing W3C compliance by TikhomirovSergey · Pull Request #829 · appium/java-client

TikhomirovSergey · 2018-02-11T00:12:03Z

Change list

added additional capability 'MobileCapabilityType.FORCE_MJSONWP'
added org.openqa.selenium.remote.NewSessionPayload
update of selenium to 3.9.1

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

I am not sure that this solution is 100% good but it seem the goal is achived.

   @BeforeClass public static void beforeClass() {
        service = AppiumDriverLocalService.buildDefaultService();
        service.start();

        if (service == null || !service.isRunning()) {
            throw new AppiumServerHasNotBeenStartedLocallyException(
                "An appium server node is not started!");
        }

        File appDir = new File("src/test/java/io/appium/java_client");
        File app = new File(appDir, "ApiDemos-debug.apk");
        DesiredCapabilities capabilities = new DesiredCapabilities();
        capabilities.setCapability(MobileCapabilityType.DEVICE_NAME, "Android Emulator");
        capabilities.setCapability(MobileCapabilityType.APP, app.getAbsolutePath());
        capabilities.setCapability(MobileCapabilityType.FORCE_MJSONWP, true);
        driver = new AndroidDriver<>(service.getUrl(), capabilities);
    }

https://gist.github.com/TikhomirovSergey/0bcaf79e96cd1ef6644e5e8c2b975596

    @BeforeClass public static void beforeClass() {
        service = AppiumDriverLocalService.buildDefaultService();
        service.start();

        if (service == null || !service.isRunning()) {
            throw new AppiumServerHasNotBeenStartedLocallyException(
                "An appium server node is not started!");
        }

        File appDir = new File("src/test/java/io/appium/java_client");
        File app = new File(appDir, "ApiDemos-debug.apk");
        DesiredCapabilities capabilities = new DesiredCapabilities();
        capabilities.setCapability(MobileCapabilityType.DEVICE_NAME, "Android Emulator");
        capabilities.setCapability(MobileCapabilityType.APP, app.getAbsolutePath());
        //capabilities.setCapability(MobileCapabilityType.FORCE_MJSONWP, true);
        driver = new AndroidDriver<>(service.getUrl(), capabilities);
    }

https://gist.github.com/TikhomirovSergey/5987f5c74e37110427b8f5651030aa3b

There's a lot of overlap between the ProtocolHandshake and the NewSessionPayload. Time to deal with this overlap between the two. The main change is to make the NewSessionPayload capable of writing to an Appendable. It does so by streaming the Capabilities used to create the payload as the OSS capabilities and those used by old geckodrivers, and then streaming every capability through transforms to generate spec compliant capabilities. There appears to be a bug where we always generate synthetic capabilities, but we can deal with that later.

…ties

Using the magic of Buck's "maven-importer" and the following maven coordinates: ``` 'org.seleniumhq.selenium:htmlunit-driver:jar:2.28.5' \ 'junit:junit:jar:4.12' \ 'net.bytebuddy:byte-buddy:jar:1.7.9' \ 'com.google.code.gson:gson:jar:2.8.2' \ 'com.google.guava:guava:jar:23.6-jre' \ 'org.apache.commons:commons-exec:jar:1.3' \ 'org.eclipse.jetty:jetty-security:jar:9.4.8.v20171121' \ 'org.testng:testng:jar:6.13.1' \ 'org.pantsbuild:jarjar:jar:1.6.5' \ 'org.eclipse.jetty:jetty-util:jar:9.4.8.v20171121' \ 'org.eclipse.jetty:jetty-server:jar:9.4.8.v20171121' \ 'org.eclipse.jetty:jetty-servlet:jar:9.4.8.v20171121' \ 'org.hamcrest:hamcrest-library:jar:1.3' \ 'com.github.javaparser:javaparser-core:jar:3.5.7' \ 'org.eclipse.jetty:jetty-jmx:jar:9.4.8.v20171121' \ 'net.jcip:jcip-annotations:jar:1.0' \ 'org.yaml:snakeyaml:jar:1.19' \ 'org.mockito:mockito-core:jar:2.13.0' \ 'io.netty:netty-all:jar:4.1.19.Final' \ 'org.eclipse.mylyn.github:org.eclipse.egit.github.core:jar:2.1.5' \ 'org.littleshoot:littleproxy:jar:1.1.2' \ 'org.slf4j:slf4j-jdk14:jar:1.7.25' ``` The version of LittleProxy we use is a snapshot and is the old version from previously with dependencies updated.

jsf-clabot · 2018-02-11T00:12:04Z

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 3 committers have signed the CLA.

✅ TikhomirovSergey
❌ shs96c
❌ barancev

mykola-mokhnach · 2018-02-11T07:33:57Z

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

+
+    /**
+     * This is the flag which forces server to switch to the mobile WSONWP.
+     * If {@code false} when it is switched to W3C mode.


mykola-mokhnach · 2018-02-11T07:36:57Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+    private final FileBackedOutputStream backingStore;
+    private final ImmutableSet<Dialect> dialects;
+
+    private static List<String> getAppiumCapabilities(Class<?> capabilityList) {


List<String> or List<Object>?

@mykola-mokhnach
List<String>

private static List<String> getAppiumCapabilities(Class<?> capabilityList) { return Arrays.stream(capabilityList.getDeclaredFields()).map(field -> { field.setAccessible(true); try { return field.get(capabilityList).toString(); //<-!!!! } catch (IllegalAccessException e) { throw new IllegalArgumentException(e); } }).filter(s -> !FORCE_MJSONWP.equals(s)).collect(toList()); }

mykola-mokhnach · 2018-02-11T07:39:22Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+        // We need to convert the capabilities into a new session payload. At this point we're dealing
+        // with references, so I'm Just Sure This Will Be Fine.
+        boolean forceMobileJSONWP = ofNullable(caps.getCapability(FORCE_MJSONWP))
+                .map(o -> {


you could use ternary operator here

mykola-mokhnach · 2018-02-11T07:39:52Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+    }
+
+    public static NewSessionPayload create(Capabilities caps) throws IOException {
+        // We need to convert the capabilities into a new session payload. At this point we're dealing


mykola-mokhnach · 2018-02-11T07:42:03Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+        return new NewSessionPayload(source, forceMobileJSONWP);
+    }
+
+    private NewSessionPayload(Reader source, boolean forceMobileJSONWP) throws IOException {


This copies a lot of stuff from the original selenium code. Is it somehow possible to avoid such duplication?

for example using reflection

mykola-mokhnach · 2018-02-11T07:43:16Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+        // Opera: operaOptions
+        // SafariDriver: safari.options
+        //
+        // We can't use the constants defined in the classes because it would introduce circular


Do we need all the commented stuff above?

mykola-mokhnach · 2018-02-11T07:52:35Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+                                public String getKey() {
+                                    String key = stringObjectEntry.getKey();
+                                    if (APPIUM_CAPABILITIES.contains(key) && !forceMobileJSONWP) {
+                                        return "appium:" + key;


appium: might be a constant

mykola-mokhnach · 2018-02-11T07:53:12Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+                    .map(this::applyTransforms)
+                    .map(map -> map.entrySet().stream()
+                            .filter(entry -> {
+                                if (forceMobileJSONWP) {


ternary operator might be more useful here

.filter(entry -> forceMobileJSONWP && ACCEPTED_W3C_PATTERNS.test(entry.getKey()) || !forceMobileJSONWP)

mykola-mokhnach · 2018-02-11T07:53:49Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+        return Stream.concat(fromOss, fromW3c).distinct();
+    }
+
+    private Map<String, Object> convertOssToW3C(Map<String, Object> capabilities) {


mark as Nullable

mykola-mokhnach · 2018-02-11T07:54:32Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+                        name = input.nextName();
+                        if ("alwaysMatch".equals(name)) {
+                            return input.read(MAP_TYPE);
+                        } else {


this else seems redundant

mykola-mokhnach · 2018-02-11T07:54:43Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+                        name = input.nextName();
+                        if ("firstMatch".equals(name)) {
+                            return input.read(LIST_OF_MAPS_TYPE);
+                        } else {


mykola-mokhnach · 2018-02-11T07:54:56Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+        return null;
+    }
+
+    private Collection<Map<String, Object>> getFirstMatches() throws IOException {


mykola-mokhnach · 2018-02-11T07:55:05Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+        return toReturn;
+    }
+
+    private Map<String, Object> getAlwaysMatch() throws IOException {


mykola-mokhnach · 2018-02-11T07:56:04Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+        backingStore.reset();
+    }
+
+    private Map<String, Object> getOss() throws IOException {


mykola-mokhnach · 2018-02-11T07:56:23Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+                String name = input.nextName();
+                if ("desiredCapabilities".equals(name)) {
+                    return input.read(MAP_TYPE);
+                } else {


else is redundant

mykola-mokhnach · 2018-02-11T12:09:21Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+                                "Illegal key values seen in w3c capabilities: " + illegalKeys);
+                    }
+                })
+                .forEach(map -> {});


what is the purpose of this operator?

mykola-mokhnach · 2018-02-11T12:27:40Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+            }
+
+            // Write the first capability we get as the desired capability.
+            json.name("desiredCapabilities");


I'd rather put all there magic strings into constants

- removal of redundant code from the original NewSessionPayload

TikhomirovSergey · 2018-02-11T22:19:24Z

@SrinivasanTarget Could you try it on iOS?

mykola-mokhnach · 2018-02-12T06:26:15Z

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

-    /**
-     * Which mobile OS platform to use.
-     */
-    String PLATFORM_NAME = "platformName";


Where this constant is present now?

in org.openqa.selenium.remote.CapabilityType

public interface MobileCapabilityType extends CapabilityType

mykola-mokhnach · 2018-02-12T06:28:22Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+
+    public static NewSessionPayload create(Capabilities caps) throws IOException {
+        boolean forceMobileJSONWP = ofNullable(caps.getCapability(FORCE_MJSONWP))
+                .map(o -> {


.map(o -> Boolean.class.isAssignableFrom(o.getClass()) && Boolean.class.cast(o))

mykola-mokhnach · 2018-02-12T06:31:20Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+                String name = input.nextName();
+                if (DESIRED_CAPABILITIES.equals(name)) {
+                    return input.read(MAP_TYPE);
+                } else {


else is redundant

TikhomirovSergey · 2018-02-12T23:00:41Z

Updated.
@SrinivasanTarget does it work fine on iOS?

SrinivasanTarget · 2018-02-13T03:49:54Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+        return null;
+    }
+
+    private @Nullable Collection<Map<String, Object>> getFirstMatches() throws IOException {


getFirstMatch

mykola-mokhnach · 2018-02-13T06:17:14Z

src/main/java/org/openqa/selenium/remote/NewSessionPayload.java

+                        name = input.nextName();
+                        if (FIRST_MATCH.equals(name)) {
+                            return input.read(LIST_OF_MAPS_TYPE);
+                        } else {


else is redundant. same comment to other similar blocks

mykola-mokhnach

minor comments

SrinivasanTarget

Everything looks fine on iOS on both W3C and JSONWP

shs96c and others added 10 commits February 9, 2018 22:43

Better docs for NewSessionPayload.stream

3687731

Fix the Protocol Handshake

b67cfdc

Rookie mistake: only include the w3c capabilities in the w3c capabili…

9a34f41

…ties

Fix tests failing because of ProtocolHandshake

f860f33

Ensure that we forward metadata in the new session payload

ed5fba8

Introducing new string constant for "platformName"

67588ec

Use Capabilities rather than ImmutableCapabilities in mutators

fbb6e58

Update to selenium 3.9.1. Changes of NewSessionPayload.

1855291

TikhomirovSergey requested review from SrinivasanTarget and mykola-mokhnach February 11, 2018 00:12

mykola-mokhnach reviewed Feb 11, 2018

View reviewed changes

TikhomirovSergey added 2 commits February 12, 2018 00:55

fixing out:

a79c624

- removal of redundant code from the original NewSessionPayload

Removal of "magic" strings

c23e05b

appium deleted a comment Feb 12, 2018

mykola-mokhnach reviewed Feb 12, 2018

View reviewed changes

mykola-mokhnach mentioned this pull request Feb 12, 2018

iOS getAlertText not behaving as expected (XCUITestDriver + Java Client) appium/appium#10111

Closed

TikhomirovSergey added 2 commits February 13, 2018 01:52

Improving set of not so major issues

b694856

convertOssToW3C marked by Nullable

afa1039

appium deleted a comment Feb 13, 2018

SrinivasanTarget reviewed Feb 13, 2018

View reviewed changes

mykola-mokhnach reviewed Feb 13, 2018

View reviewed changes

mykola-mokhnach approved these changes Feb 13, 2018

View reviewed changes

SrinivasanTarget approved these changes Feb 13, 2018

View reviewed changes

Fixing issuies before the merging

6ffd2ea

appium deleted a comment Feb 13, 2018

TikhomirovSergey merged commit 409e6f3 into appium:master Feb 13, 2018

Uh oh!

Conversation

TikhomirovSergey commented Feb 11, 2018

Change list

Types of changes

Details

Uh oh!

jsf-clabot commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Feb 11, 2018 • 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!

TikhomirovSergey Feb 11, 2018 • 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!

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!

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!

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 Feb 11, 2018

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 Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach left a comment

Choose a reason for hiding this comment

Uh oh!

SrinivasanTarget left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

jsf-clabot commented Feb 11, 2018 •

edited

Loading

mykola-mokhnach Feb 11, 2018 •

edited

Loading

TikhomirovSergey Feb 11, 2018 •

edited

Loading