-
-
Notifications
You must be signed in to change notification settings - Fork 764
fix: Fix Selenium 4.40.0+ compatibility #2383
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 Selenium 4.40.0+ compatibility #2383
Conversation
|
Ooops :) |
| } | ||
| var biDiConnection = new org.openqa.selenium.bidi.Connection(wsClient, biDiUri.toString()); | ||
| this.biDi = new BiDi(biDiConnection); | ||
| this.biDi = new BiDi(biDiConnection, wsConfig.wsTimeout()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this limit the minimum supported selenium version to 4.40?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
Well, we could restore constructor new BiDi(biDiConnection) in Selenium 4.41.0.
Then we do a switch in Appium:
if <new constructor exists>
return new BiDi(biDiConnection, timeout)
else
return new BiDi(biDiConnection)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense just to bump the minimum supported selenium version, otherwise we'll get a lot of complains and confusion. (It's not the first breaking change from Selenium)
PS: I've fixed README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use reflection to make it backward compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, do you want me to re-work the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but still the release will be needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Well, we could restore constructor
new BiDi(biDiConnection)in Selenium 4.41.0. Then we do a switch in Appium:if
return new BiDi(biDiConnection, timeout)
else
return new BiDi(biDiConnection)
this probably won't work. We'd need to load the class dynamically via Class.forName and also use an interface to access its methods, which will be a pain
I agree to @valfirst , it's not the first time we deal with such issues, yolo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the first breaking change from Selenium
I don't know about other cases, but this specific was done occasionally. Sorry for that. :(
Don't you want to run Appium build against the night Selenium build (4.41.0-SNAPSHOT)?
In Selenide, we do the following:
- Every PR and main branch is built against latest stable Selenium (4.40.0)
- Additionally, every night we run a build from
mainbranch against latest Selenium nightly build (4.41.0-SNAPSHOT)
It helps to detect backward incompatible changes on an early stage.
c326126 to
bab90bc
Compare
Change list
BiDiclass constructor requires additional input parameter: SeleniumHQ/selenium@4cd4b09#diff-f0ed6d655b8c0ff47563631639d01d8f7a5f8e83a40db48456a2e9137e8ab0e6R33Types of changes
What types of changes are you proposing/introducing to Java client?
Put an
xin the boxes that applyDetails
Note: new release is needed.