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
[java] POC: Porting WebDriver classic command to BiDi #13190
base: trunk
Are you sure you want to change the base?
Conversation
579e69e
to
b1fdeea
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## trunk #13190 +/- ##
=======================================
Coverage 58.07% 58.07%
=======================================
Files 88 88
Lines 5340 5340
Branches 224 224
=======================================
Hits 3101 3101
Misses 2015 2015
Partials 224 224 ☔ View full report in Codecov by Sentry. |
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.
This is great. I love seeing all that extra code removed from the driver classes. What is the separation of concerns between Oh, this is when the user passes an existing |
BiDiDelegator knows where to delegate the commands and that's it. BiDiExecutor will have reference to methods/class that handles converting the Classic command to BiDiCommand and also maintain browsing context to identify where to run the command since almost all BiDi commands need the browsing context id to be sent to run commands. For atoms, it will have additional methods to convert arguments to how BiDi expects it to run scripts. I think we don't want to override the user's custom executor. I have considered that case. So we do a check that if the user passes an executor that is an instance of BiDiDelegator then we set the BiDiCommandExecutor for it. Otherwise, the executor passed in will be used as it is. |
Details or gaps found due to failing tests:
|
…nding BiDi command
d49c84c
to
ca99cd1
Compare
@pujagani is there a long therm plan to support classic w3c commands or is the plan to shift to BiDi at a certain time? I am currently playing with BiDi and noticed it is relying on a stable tcp-ip connection, what seems to be not the case in the company i am working at. Looks like our datacenters sometimes have short connection outtakes or at least some connections are dropped (might be the VPN, who knows). We did not notice them till now, as HTTP clients do automatically reconnect when a connection is stale. |
Thank you @joerg1985 for sharing this. The plan is to move to BiDi under the hood Selenium 5 onwards (whenever it happens). |
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
This POC demonstrates the changes in porting the WebDriver Classic command to use BiDi under the hood.
The changes port GET and PRINT command.
Motivation and Context
The next big step for Selenium towards Selenium 5 is porting WebDriver Classic commands to use WebDriver BiDi protocol. This is the first step towards it. The changes are not trivial, a lot of pieces need to be considered. Instead of moving the mountain, trying to make progress towards it in decent-sized chunks of work.
Reason for choosing GET command: demonstrate the need for requiring driver instance since capabilities like pageLoadStrategy are required
Reason for choosing the PRINT command: Required additional step to serialize parameters into PrintOptions. That piece of code can be moved to PrintOptions but the build kept breaking due to cycling dependency so didn't dig deep into it.
Includes the items that need to be considered/included:
TODO:
Types of changes
Checklist