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.

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

fix: Fix compatibility with latest Selenium snapshots #2249

Conversation

valfirst
Copy link
Collaborator

Change list

Fix compatibility with latest Selenium snapshots.

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

Recently Selenium team has introduced an update for Appium client adding ability to get additionalCommands in HttpCommandExecutor: SeleniumHQ/selenium@59aa1a0.
But since new method is public and the same Appium method in AppiumCommandExecutor.java is protected, this collision introduces an error at the build stage:

/Users/runner/work/java-client/java-client/src/main/java/io/appium/java_client/remote/AppiumCommandExecutor.java:136: error: getAdditionalCommands() in AppiumCommandExecutor cannot override getAdditionalCommands() in HttpCommandExecutor

    protected Map<String, CommandInfo> getAdditionalCommands() {
> Task :compileJava FAILED
                                       ^

@valfirst valfirst force-pushed the fix-compatibility-with-latest-selenium-snapshots branch from 0dae8da to 0ee114b Compare December 12, 2024 20:31
@@ -133,7 +133,7 @@ protected void setPrivateFieldValue(
ReflectionHelpers.setPrivateFieldValue(cls, this, fieldName, newValue);
}

protected Map<String, CommandInfo> getAdditionalCommands() {
public Map<String, CommandInfo> getAdditionalCommands() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume there we can safely delete this getter as it is public now in the parent class

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, we can, but it will bump the minimal Selenium client version to 4.28.0-SNAPSHOT, if it's ok, I'll do it

Copy link
Contributor

Choose a reason for hiding this comment

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

lets wait until 4.28 is released, so then we could also release a new minor version of java client and set the minimum version there

@valfirst valfirst force-pushed the fix-compatibility-with-latest-selenium-snapshots branch from 0ee114b to 713ea7c Compare December 12, 2024 21:38
@@ -192,7 +192,11 @@ private Response createSession(Command command) throws IOException {
}

public void refreshAdditionalCommands() {
Copy link
Contributor

Choose a reason for hiding this comment

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

if this method is not used anymore then it would probably make sense to mark it deprecated

@mykola-mokhnach mykola-mokhnach merged commit 7fc702e into appium:master Dec 15, 2024
6 of 7 checks passed
@valfirst valfirst deleted the fix-compatibility-with-latest-selenium-snapshots branch December 16, 2024 06:41
@KazuCocoa KazuCocoa added the size:S contribution size: S label Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:S contribution size: S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants