Skip to content

fix: Make unit tests compatible with Selenium 4.33.0+ #2293

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

Merged
merged 1 commit into from
May 19, 2025

Conversation

valfirst
Copy link
Collaborator

@valfirst valfirst commented May 19, 2025

Change list

The PR addresses the latest breaking changes in Selenium client: SeleniumHQ/selenium@1cad0ca.

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

  • Only unit tests are affected, no new release is required
  • The current changes keep compatibility with lower versions of Selenium client. Once the minimum Selenium versions are bumped, the deprecated methods can be gradually removed.

Copy link

@diemol diemol left a comment

Choose a reason for hiding this comment

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

I was coming here to do the same, thank you for doing this, @valfirst!

public Timeouts setScriptTimeout(Duration duration) {
return this;
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

we should remove override annotations to be compatible to older versions. It also does not make sense to keep Deprecates ones as the original interface already does that anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we should remove override annotations to be compatible to older versions.

Only stable non-deprecated methods are marked with override annotations. It is useful to track changes from Selenium client. Also the methods marked with with override annotations were introduced in Selenium around 4 years ago (SeleniumHQ/selenium@afa9187), so backward compatibility is maintained.

It also does not make sense to keep Deprecates ones as the original interface already does that anyway

This is for housekeeping purposes only: to ensure that we do not forget to remove them at some point in the future. I can use other approach, like, // TODO comment

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean a compilation error will be thrown if one wants to add Override annotation to a method that does not exist in superclass or an interface, which automatically makes this fix incompatible with older selenium versions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry, it seems I do not follow your thought, could you please elaborate a bit?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, for example let say we have libX v1, with the following interface:

public interface Parent {
 @Deprecated
 public void method();
}

Then libX v2 removes it

public interface Parent {
}

Then the following client declaration would fail to compile if it uses libX v2, but would pass if libX v1 is used

public class Child implements Parent {
  @Override
  public void method();  
}

The below declaration would fail if libX v1 is used, but would pass for v2:

public class Child implements Parent {
}

And the third one would pass for both libX v1 and libX v2:

public class Child implements Parent {
  public void method();  
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, thank you! At the current moment the methods marked with @Override are not deprecated and not scheduled to be removed from Selenium client. If it's needed, I can remove the annotation from them as well

Copy link
Contributor

@mykola-mokhnach mykola-mokhnach May 19, 2025

Choose a reason for hiding this comment

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

yes please remove the Override annotation from all methods

@valfirst valfirst force-pushed the fix-selenium-4-33-0-compatibility branch from 1a5da2e to 0ed9e14 Compare May 19, 2025 17:33
@valfirst valfirst requested a review from mykola-mokhnach May 19, 2025 19:04
@valfirst valfirst merged commit 5560a56 into appium:master May 19, 2025
7 checks passed
@valfirst valfirst deleted the fix-selenium-4-33-0-compatibility branch May 19, 2025 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants