refactor!: Migrate to Selenium 4 #1531

valfirst · 2021-10-13T14:23:56Z

Change list

Migrate to Selenium 4

Types of changes

What types of changes are you proposing/introducing to Java client?

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

BREAKING CHANGE:

interface io.appium.java_client.MobileDriver do not extend org.openqa.selenium.internal.FindsByClassName, org.openqa.selenium.internal.FindsByCssSelector, org.openqa.selenium.internal.FindsById, org.openqa.selenium.internal.FindsByLinkText, org.openqa.selenium.internal.FindsByName, org.openqa.selenium.internal.FindsByTagName, org.openqa.selenium.internal.FindsByXPath interfaces anymore because they were removed in Selenium Java client;
class io.appium.java_client.DefaultGenericMobileElement do not implement org.openqa.selenium.internal.FindsByClassName, org.openqa.selenium.internal.FindsByCssSelector, org.openqa.selenium.internal.FindsById, org.openqa.selenium.internal.FindsByLinkText, org.openqa.selenium.internal.FindsByName, org.openqa.selenium.internal.FindsByTagName, org.openqa.selenium.internal.FindsByXPath interfaces anymore because they were removed in Selenium Java client;
method String io.appium.java_client.remote.MobileOptions#getPlatformName() is removed in favor of Platform org.openqa.selenium.Capabilities#getPlatformName()
method io.appium.java_client.service.local.AppiumServiceBuilder#withStartUpTimeOut is removed in favor of org.openqa.selenium.remote.service.DriverService.Builder#withTimeout
drop Appium FindsBy* iterfaces in the same way it was done in Selenium java client. The removed intefraces are: io.appium.java_client.FindsByAccessibilityId, io.appium.java_client.FindsByAndroidDataMatcher, io.appium.java_client.FindsByAndroidUIAutomator, io.appium.java_client.FindsByAndroidViewMatcher, io.appium.java_client.FindsByAndroidViewTag, io.appium.java_client.FindsByCustom, io.appium.java_client.FindsByFluentSelector, io.appium.java_client.FindsByImage, io.appium.java_client.FindsByIosClassChain, io.appium.java_client.FindsByIosNSPredicate, io.appium.java_client.FindsByWindowsAutomation, io.appium.java_client.mac.FindsByClassChain, io.appium.java_client.mac.FindsByNsPredicate
remove methods findElements(String by, String using) and findElement(String by, String using) from io.appium.java_client.DefaultGenericMobileDriver and io.appium.java_client.DefaultGenericMobileElement because the originals of these methods are deprecated in Selenium RemoteWebDriver and RemoteWebElement and throw UnsupportedOperationException
remove io.appium.java_client.MobileSelector as it's used once, the string values from the enum are inlined in io.appium.java_client.MobileBy.java
drop deprecated method io.appium.java_client.AppiumDriver#substituteMobilePlatform

BUG FIXES:

The change made in Selenium 4 (SeleniumHQ/selenium@0aaa401#r58091435) broke Appium class name selector strategy. The workaround was implemented: MobileBy#className.

Closes #1536
Closes #1273
Closes #1245

linux-foundation-easycla · 2021-10-13T14:23:59Z

The committers are authorized under a signed CLA.

✅ Valery Yatsynovich (f44ead6)

SrinivasanTarget · 2021-10-13T14:38:42Z

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

+        return new HttpClient() {
+            @Override
+            public HttpResponse execute(HttpRequest request) {
+                request.setHeader(IDEMPOTENCY_KEY_HEADER, UUID.randomUUID().toString().toLowerCase());


You can also implement a Filter to add the header if it's missing. That avoids the need for subclassing the httpclient

src/main/java/io/appium/java_client/service/local/AppiumServiceBuilder.java

mykola-mokhnach · 2021-10-13T14:51:26Z

Please mention all breaking changes in the commit message according to conventional commits rules.

mykola-mokhnach · 2021-10-13T15:02:56Z

I'd like to establish a better connection between Appium and Selenium, thus kindly asking @shs96c to take a look at this PR and help us to optimize it better before we advertise the compatibility with Selenium4 and release a major lib update.

titusfortner · 2021-10-13T22:02:36Z

Would it make sense to remove/deprecate the Appium FindsElementBy* methods, so both Appium and Selenium are solely using the By.* and MobileBy.* approach?

titusfortner · 2021-10-13T22:10:05Z

Also, for what it's worth, @shs96c is going to be stepping down from Selenium project in a couple weeks. I don't know if he will or won't have the chance to review this before he departs, but as a Selenium dev, I would like to help facilitate our projects working together more going forward. It would be great if our users had a more seamless experience using both of our projects in each of the languages.

SrinivasanTarget · 2021-10-13T22:33:32Z

Would it make sense to remove/deprecate the Appium FindsElementBy* methods, so both Appium and Selenium are solely using the By.* and MobileBy.* approach?

Agree. I think it's right time to get rid of it and keep it inline with Se4 way.

mykola-mokhnach · 2021-10-14T07:08:27Z

src/main/java/org/openqa/selenium/WebDriver.java

@@ -21,14 +21,15 @@
 import org.openqa.selenium.logging.Logs;


not sure how much it would be possible now, but I'd really like to get rid of the package override in the client (e.g. remove everything under src/main/java/org/openqa/selenium from this lib)

Split packages are forbidden in Java 9+. Anything that uses the module system will refuse to load one.

It's not clear why this is being done. Perhaps with a better understanding of that, we can figure out a way to solve the problem that doesn't create a split package?

I believe this was done to "override" some Selenium interfaces, so Appium could create elements inherited from WebElement (e.g. AppiumElement). See SeleniumHQ/selenium#415 (which has never been merged) for more detalis.

for now I don't see any elegant way to fix this package override, if anyone has ideas, please share

This topic is currently being discussed with @shs96c
Perhaps, we could find a happy middle there before the next major release (ofc not necessarily as part of this PR, but this might still be a breaking change)
Feel free to follow the discussion https://seleniumhq.slack.com/archives/CBH302726/p1634562817364100

shs96c · 2021-10-15T08:54:49Z

In response to #1531 (comment), I'll certainly do my best to review this PR.

shs96c

Seems pretty much like we expected the migration to look. One question about the split package

shs96c · 2021-10-15T09:59:40Z

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

+        return new HttpClient() {
+            @Override
+            public HttpResponse execute(HttpRequest request) {
+                request.setHeader(IDEMPOTENCY_KEY_HEADER, UUID.randomUUID().toString().toLowerCase());


You can also implement a Filter to add the header if it's missing. That avoids the need for subclassing the httpclient

shs96c · 2021-10-15T10:01:48Z

src/main/java/io/appium/java_client/ws/StringWebSocketClient.java

-                .build();
-        client.newWebSocket(request, this);
-        client.dispatcher().executorService().shutdown();
+        ClientConfig clientConfig = ClientConfig.defaultConfig().readTimeout(Duration.ZERO);


I really hope that this disables the read timeout....

shs96c · 2021-10-15T10:03:43Z

src/main/java/org/openqa/selenium/WebDriver.java

@@ -21,14 +21,15 @@
 import org.openqa.selenium.logging.Logs;


Split packages are forbidden in Java 9+. Anything that uses the module system will refuse to load one.

shs96c · 2021-10-15T10:04:56Z

src/main/java/org/openqa/selenium/WebDriver.java

@@ -21,14 +21,15 @@
 import org.openqa.selenium.logging.Logs;


It's not clear why this is being done. Perhaps with a better understanding of that, we can figure out a way to solve the problem that doesn't create a split package?

valfirst · 2021-10-15T10:21:57Z

It's not clear why this is being done. Perhaps with a better understanding of that, we can figure out a way to solve the problem that doesn't create a split package?

@shs96c I think the answer is here: d28c76d

mykola-mokhnach · 2021-10-15T11:34:20Z

In response to #1531 (comment), I'll certainly do my best to review this PR.

Thank you Simon. We really appreciate your help

valfirst · 2021-10-15T12:20:46Z

SeleniumHQ/selenium@0aaa401#r58091435 - this change breaks the tests, I'm not sure whether it's expected and whether it worths fixing on Appium Java client side (e.g. by introducing MobileBy.className)

BREAKING CHANGE: - interface `io.appium.java_client.MobileDriver` do not extend `org.openqa.selenium.internal.FindsByClassName`, `org.openqa.selenium.internal.FindsByCssSelector`, `org.openqa.selenium.internal.FindsById`, `org.openqa.selenium.internal.FindsByLinkText`, `org.openqa.selenium.internal.FindsByName`, `org.openqa.selenium.internal.FindsByTagName`, `org.openqa.selenium.internal.FindsByXPath` interfaces anymore because they were removed in Selenium Java client; - class `io.appium.java_client.DefaultGenericMobileElement` do not implement `org.openqa.selenium.internal.FindsByClassName`, `org.openqa.selenium.internal.FindsByCssSelector`, `org.openqa.selenium.internal.FindsById`, `org.openqa.selenium.internal.FindsByLinkText`, `org.openqa.selenium.internal.FindsByName`, `org.openqa.selenium.internal.FindsByTagName`, `org.openqa.selenium.internal.FindsByXPath` interfaces anymore because they were removed in Selenium Java client; - method `String io.appium.java_client.remote.MobileOptions#getPlatformName()` is removed in favor of `Platform org.openqa.selenium.Capabilities#getPlatformName()` - method `io.appium.java_client.service.local.AppiumServiceBuilder#withStartUpTimeOut` is removed in favor of `org.openqa.selenium.remote.service.DriverService.Builder#withTimeout`

BREAKING CHANGE: - drop Appium `FindsBy*` iterfaces in the same way it was done in Selenium java client. The removed intefraces are: `io.appium.java_client.FindsByAccessibilityId`, `io.appium.java_client.FindsByAndroidDataMatcher`, `io.appium.java_client.FindsByAndroidUIAutomator`, `io.appium.java_client.FindsByAndroidViewMatcher`, `io.appium.java_client.FindsByAndroidViewTag`, `io.appium.java_client.FindsByCustom`, `io.appium.java_client.FindsByFluentSelector`, `io.appium.java_client.FindsByImage`, `io.appium.java_client.FindsByIosClassChain`, `io.appium.java_client.FindsByIosNSPredicate`, `io.appium.java_client.FindsByWindowsAutomation`, `io.appium.java_client.mac.FindsByClassChain`, `io.appium.java_client.mac.FindsByNsPredicate` - remove methods `findElements(String by, String using)` and `findElement(String by, String using)` from `io.appium.java_client.DefaultGenericMobileDriver` and `io.appium.java_client.DefaultGenericMobileElement` because the originals of these methods are deprecated in Selenium `RemoteWebDriver` and `RemoteWebElement` and throw `UnsupportedOperationException` - remove `io.appium.java_client.MobileSelector` as it's used once, the string values from the enum are inlined in `io.appium.java_client.MobileBy.java`

The change made in Selenium 4 (SeleniumHQ/selenium@0aaa401#r58091435) broke Appium `class name` selector strategy. The workaround was implemented: `MobileBy#className`.

…orm` BREAKING CHANGE: drop deprecated method `io.appium.java_client.AppiumDriver#substituteMobilePlatform`

mykola-mokhnach · 2021-10-20T20:12:14Z

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

-            throw formIllegalArgumentException(contextClass, FindsByAndroidViewTag.class,
-                    FindsByFluentSelector.class);
+        @Override public String toString() {
+            return "By.AndroidViewTag: " + getLocatorString();


Can we just define toString in the base class and make it to something like

return String.format("By.%s: %s", getClass().getSimpleName(), getLocatorString())

?

mykola-mokhnach

nicely done. Thanks Valery

SrinivasanTarget

Looks good. Thanks @valfirst

SrinivasanTarget · 2021-10-13T16:29:42Z

gradle.properties

@@ -7,4 +7,4 @@ signing.secretKeyRingFile=PathToYourKeyRingFile
 ossrhUsername=your-jira-id
 ossrhPassword=your-jira-password

-selenium.version=3.141.59
+selenium.version=4.0.0-rc-3


Let's update to stable version as it's out now

refactor!: Migrate to Selenium 4 (appium#1531)

valfirst marked this pull request as draft October 13, 2021 14:24

valfirst force-pushed the migrate-to-selenium-4 branch from b01b6ff to f44ead6 Compare October 13, 2021 14:25

valfirst mentioned this pull request Oct 13, 2021

Selenium v4 support #1530

Closed

4 tasks

SrinivasanTarget reviewed Oct 13, 2021

View reviewed changes

mykola-mokhnach reviewed Oct 13, 2021

View reviewed changes

src/main/java/io/appium/java_client/service/local/AppiumServiceBuilder.java Show resolved Hide resolved

mykola-mokhnach changed the title ~~Migrate to Selenium 4~~ refactor!: Migrate to Selenium 4 Oct 13, 2021

mykola-mokhnach mentioned this pull request Oct 13, 2021

refactor!: Update to work with Selenium 4 (with element factory) appium/dotnet-client#469

Merged

6 tasks

appium temporarily blocked BOYKHMER Oct 13, 2021

valfirst force-pushed the migrate-to-selenium-4 branch 2 times, most recently from 27bdc96 to 0be6ac3 Compare October 13, 2021 18:36

mykola-mokhnach reviewed Oct 14, 2021

View reviewed changes

shs96c reviewed Oct 15, 2021

View reviewed changes

valfirst force-pushed the migrate-to-selenium-4 branch from 9776116 to e4b8efe Compare October 15, 2021 11:27

valfirst force-pushed the migrate-to-selenium-4 branch from e4b8efe to 3d82e42 Compare October 15, 2021 13:44

valfirst force-pushed the migrate-to-selenium-4 branch 2 times, most recently from ae6ea41 to f1579d6 Compare October 19, 2021 12:00

valfirst added 2 commits October 19, 2021 17:23

fix: introduce MobileBy.className

5063baf

The change made in Selenium 4 (SeleniumHQ/selenium@0aaa401#r58091435) broke Appium `class name` selector strategy. The workaround was implemented: `MobileBy#className`.

valfirst force-pushed the migrate-to-selenium-4 branch from f1579d6 to 5063baf Compare October 19, 2021 14:23

refactor!: drop deprecated method `AppiumDriver#substituteMobilePlatf…

a836cc9

…orm` BREAKING CHANGE: drop deprecated method `io.appium.java_client.AppiumDriver#substituteMobilePlatform`

valfirst marked this pull request as ready for review October 20, 2021 18:53

valfirst requested review from mykola-mokhnach and SrinivasanTarget October 20, 2021 18:53

mykola-mokhnach reviewed Oct 20, 2021

View reviewed changes

mykola-mokhnach approved these changes Oct 20, 2021

View reviewed changes

SrinivasanTarget approved these changes Oct 21, 2021

View reviewed changes

SrinivasanTarget merged commit 8057ac6 into appium:master Oct 21, 2021

sakibguy added a commit to sakibguy/java-client that referenced this pull request Oct 21, 2021

Merge pull request #3 from appium/master

414f5ca

refactor!: Migrate to Selenium 4 (appium#1531)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: Migrate to Selenium 4 #1531

refactor!: Migrate to Selenium 4 #1531

valfirst commented Oct 13, 2021 •

edited

Loading

linux-foundation-easycla bot commented Oct 13, 2021 •

edited

Loading

SrinivasanTarget Oct 13, 2021

This comment was marked as off-topic.

shs96c Oct 15, 2021

mykola-mokhnach commented Oct 13, 2021

mykola-mokhnach commented Oct 13, 2021

titusfortner commented Oct 13, 2021

titusfortner commented Oct 13, 2021

SrinivasanTarget commented Oct 13, 2021

mykola-mokhnach Oct 14, 2021 •

edited

Loading

shs96c Oct 15, 2021

shs96c Oct 15, 2021

mykola-mokhnach Oct 15, 2021

valfirst Oct 15, 2021

mykola-mokhnach Oct 19, 2021

shs96c commented Oct 15, 2021

shs96c left a comment

shs96c Oct 15, 2021

shs96c Oct 15, 2021

shs96c Oct 15, 2021

shs96c Oct 15, 2021

valfirst commented Oct 15, 2021

mykola-mokhnach commented Oct 15, 2021

valfirst commented Oct 15, 2021

mykola-mokhnach Oct 20, 2021 •

edited

Loading

mykola-mokhnach left a comment

SrinivasanTarget left a comment

SrinivasanTarget Oct 13, 2021

refactor!: Migrate to Selenium 4 #1531

refactor!: Migrate to Selenium 4 #1531

Conversation

valfirst commented Oct 13, 2021 • edited Loading

Change list

Types of changes

Details

linux-foundation-easycla bot commented Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

mykola-mokhnach commented Oct 13, 2021

mykola-mokhnach commented Oct 13, 2021

titusfortner commented Oct 13, 2021

titusfortner commented Oct 13, 2021

SrinivasanTarget commented Oct 13, 2021

mykola-mokhnach Oct 14, 2021 • edited Loading

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

shs96c commented Oct 15, 2021

shs96c left a 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

valfirst commented Oct 15, 2021

mykola-mokhnach commented Oct 15, 2021

valfirst commented Oct 15, 2021

mykola-mokhnach Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

mykola-mokhnach left a comment

Choose a reason for hiding this comment

SrinivasanTarget left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valfirst commented Oct 13, 2021 •

edited

Loading

linux-foundation-easycla bot commented Oct 13, 2021 •

edited

Loading

mykola-mokhnach Oct 14, 2021 •

edited

Loading

mykola-mokhnach Oct 20, 2021 •

edited

Loading