[java] WIP - proof of concept for using options with Selenium Manager

[titusfortner]

Description

I think this is the best way to get needed information from Options class to Selenium Manager.

• Ensure driver constructor uses createServiceWithConfig by default.
• new private withOptions() method that stores protected static options instance on DriverService
• Browser DriverService class can pull browser-specific functionality (getBinary()); though, I guess we *could implement this in all driver service classes and essentially avoid all subclasses for now.
• Couple things get cleaned up when we implement this in all browsers, but I wanted to show the idea with one to get feedback.

Questions:

• Does storing a protected static options instance make the most sense?
• Should we create getBinary() in all options classes and avoid subclassing requirements? (fwiw, Firefox's current implementation might complicate using Selenium Manager; Chrome was much easier to show the idea)
• Is there anything we can think of that we might be doing with Selenium Manager in the future that this implementation would complicate?

Motivation

• Provide support for [🚀 Feature]: Selenium Manager get Browser Version from Options classes #11372
• Provide support for [🚀 Feature]: Selenium Manager use binary from Browser Options #11351 when it is available
• It might be relevant to how we do [🚀 Feature]: Selenium Manager - allow to ignore build matching #11353

Previously, I was thinking about the need to specifically wrap --driver-version, which is why I brought up #11360
As of now, though, I think everything that we need to be supporting in Selenium Manager we can pull from the Options class, and this is the best way I came up with for doing that.

[codecov-commenter]

Codecov Report

Base: 52.83% // Head: 52.83% // No change to project coverage 👍

Coverage data is based on head (433bbee) compared to base (3832787).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #11376   +/-   ##
=======================================
  Coverage   52.83%   52.83%           
=======================================
  Files          82       82           
  Lines        5551     5551           
  Branches      198      198           
=======================================
  Hits         2933     2933           
  Misses       2420     2420           
  Partials      198      198           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[titusfortner]

(This is to address #11372)

[diemol]

I am OK when the PR template is ignored, but I think we should always provide the context of a PR (unless the code is extremely explicit). I understand why this PR was created and how the context was build through Slack discussions, but leaving this PR without a context will make future maintainers life very hard. This is why we have so much code in Java today that is doing magical stuff and we do not understand why.

As for the contents of the PR, they seem fine to me, but I do not recall all the details of the conversation.

[titusfortner]

This isn't production-ready code, so I moved it to draft status and added context with the specific questions I'm interested in getting feedback on. Thanks.

[titusfortner]

Some issues...

I think we need to deprecate either public static ChromeDriverService createDefaultService() or public ChromeDriver(ChromeDriverService service, ChromeOptions options) (make it private).

Since this code won't work as I have it implemented:

ChromeDriverService service = ChromeDriverService.createDefaultService();
ChromeOptions options = new ChromeOptions();
options.setBrowserVersion("100");
options.setBinary("/path/to/old/chrome");

ChromeDriver driver = new ChromeDriver(service, options);

[shs96c]

The coupling here is wrong: to integrate the SeleniumManager you shouldn't need to make changes to the DriverService.

Roles and responsibilities:

• SeleniumManager: finding the driver and browser
• DriverService: managing the lifetime of the driver and browser

The DriverService doesn't care how the driver and browser are found, just that it has them.

[shs96c]

This expansion won't be the same, so people won't be able to copy-and-paste it.

[titusfortner]

The DriverService doesn't care how the driver and browser are found

Except that is exactly what it is currently concerning itself with. If the location isn't passed in by the user (very rarely the case), it has logic for locating it.

We're currently putting the constraint on the Manager to only be used if the existing process would otherwise error.

We can:

1. Change that constraint and have the manager always execute and pass in the driver value to the DriverService
2. Change the existing DriverService class to insert logic between the finding and erroring steps (this implementation)
3. Separate out the Finding logic of existing DriverService code and insert Selenium Manager steps
4. Something else I'm not seeing?

@shs96c which of these are you suggesting ^^

[titusfortner]

Ok, I spent a lot more time inside the DriverService classes trying to figure out a better approach... We can definitely pull out the finding pieces for better separation of concerns.

I implemented a DriverFinder class, with a setDriverPath(options) method, that will set the driver path property that will then get used by DriverService. I hacked a way to get options to execute it, without the DriverService ever having to touch options. But there's no good way to ensure that the user didn't build their own Service before creating the options.

I think the Service class has to get passed in the options instance (withOptions(options)), but it can call out to the DriverFinder without needing to change anything in DriverService class directly.

User will need to have done one of these:

1. Not specified their own Service instance
2. Set the System Property before building their own Service instance
3. Built DriverService and used usingDriverExecutable()
4. Constructed DriverService with hard-coded path to driver
5. Explicitly called the new withOptions() to use SeleniumManager

So, we throw a warning if build() is called and none of those things have happened.

That is probably a better approach, but it's a lot to put together...
Let me try to polish it up again after having thought through it once more....

[titusfortner]

ok, I used my previous comment to rethink things and improve on where I was going with it.

I'm pretty sure #11411 is the superior implementation, so I'm going to close this.